From 80aaf22373c9b0b63f596f0ea08b24830145b21d Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Mon, 7 Nov 2022 09:34:40 -0500 Subject: [PATCH 1/5] cmd-buildextend-live: declare `vendor_id` var to avoid indexing It's more legible that way. --- src/cmd-buildextend-live | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/cmd-buildextend-live b/src/cmd-buildextend-live index 9f20fa1b06..9e7cb27c57 100755 --- a/src/cmd-buildextend-live +++ b/src/cmd-buildextend-live @@ -539,14 +539,15 @@ def generate_iso(): vendor_ids = [n for n in os.listdir(tmpimageefidir) if n != "BOOT"] if len(vendor_ids) != 1: raise Exception(f"did not find exactly one EFI vendor ID: {vendor_ids}") + vendor_id = vendor_ids[0] - # Always replace live/EFI/{vendor} to actual live/EFI/{vendor_id[0]} + # Always replace live/EFI/{vendor} to actual live/EFI/{vendor_id} # https://github.com/openshift/os/issues/954 grubfilepath = ensure_glob(os.path.join(tmpdir, 'live/EFI/*/grub.cfg')) if len(grubfilepath) != 1: raise Exception(f'Found != 1 grub.cfg files: {grubfilepath}') srcpath = os.path.dirname(grubfilepath[0]) - os.renames(srcpath, os.path.join(os.path.dirname(srcpath), vendor_ids[0])) + os.renames(srcpath, os.path.join(os.path.dirname(srcpath), vendor_id)) # Delete fallback and its CSV file. Its purpose is to create # EFI boot variables, which we don't want when booting from @@ -557,19 +558,19 @@ def generate_iso(): # exists. But for now, fail if fallback.efi is missing. for path in ensure_glob(os.path.join(tmpimageefidir, "BOOT", "fb*.efi")): os.unlink(path) - for path in ensure_glob(os.path.join(tmpimageefidir, vendor_ids[0], "BOOT*.CSV")): + for path in ensure_glob(os.path.join(tmpimageefidir, vendor_id, "BOOT*.CSV")): os.unlink(path) # Drop vendor copies of shim; we already have it in BOOT*.EFI in # BOOT - for path in ensure_glob(os.path.join(tmpimageefidir, vendor_ids[0], "shim*.efi")): + for path in ensure_glob(os.path.join(tmpimageefidir, vendor_id, "shim*.efi")): os.unlink(path) # Consolidate remaining files into BOOT. shim needs GRUB to be # there, and the rest doesn't hurt. - for path in ensure_glob(os.path.join(tmpimageefidir, vendor_ids[0], "*")): + for path in ensure_glob(os.path.join(tmpimageefidir, vendor_id, "*")): shutil.move(path, os.path.join(tmpimageefidir, "BOOT")) - os.rmdir(os.path.join(tmpimageefidir, vendor_ids[0])) + os.rmdir(os.path.join(tmpimageefidir, vendor_id)) # Inject a stub grub.cfg pointing to the one in the main ISO image. # @@ -587,7 +588,7 @@ def generate_iso(): # pointing to efiboot.img) and needs a grub.cfg there. with open(os.path.join(tmpimageefidir, "BOOT", "grub.cfg"), "w") as fh: fh.write(f'''search --label "{volid}" --set root --no-floppy -set prefix=($root)/EFI/{vendor_ids[0]} +set prefix=($root)/EFI/{vendor_id} echo "Booting via ESP..." configfile $prefix/grub.cfg boot From c15e5876c9fe17622ac4e530377e81f1e42032ec Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Mon, 7 Nov 2022 09:34:41 -0500 Subject: [PATCH 2/5] cmd-buildextend-live: use dfd-relative for grub.cfg rename This allows the glob to be relative to the live ISO root. Prep for future patch which relies on this. --- src/cmd-buildextend-live | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/cmd-buildextend-live b/src/cmd-buildextend-live index 9e7cb27c57..39249d81f0 100755 --- a/src/cmd-buildextend-live +++ b/src/cmd-buildextend-live @@ -543,11 +543,13 @@ def generate_iso(): # Always replace live/EFI/{vendor} to actual live/EFI/{vendor_id} # https://github.com/openshift/os/issues/954 - grubfilepath = ensure_glob(os.path.join(tmpdir, 'live/EFI/*/grub.cfg')) + dfd = os.open(tmpisoroot, os.O_RDONLY) + grubfilepath = ensure_glob('EFI/*/grub.cfg', dir_fd=dfd) if len(grubfilepath) != 1: raise Exception(f'Found != 1 grub.cfg files: {grubfilepath}') srcpath = os.path.dirname(grubfilepath[0]) - os.renames(srcpath, os.path.join(os.path.dirname(srcpath), vendor_id)) + os.rename(srcpath, f"EFI/{vendor_id}", src_dir_fd=dfd, dst_dir_fd=dfd) + os.close(dfd) # Delete fallback and its CSV file. Its purpose is to create # EFI boot variables, which we don't want when booting from From d5cdce73ab3f9bc2241214b4f5df96b90897234c Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Mon, 7 Nov 2022 09:34:42 -0500 Subject: [PATCH 3/5] cmd-buildextend-live: update `kargs_json` when moving EFI `grub.cfg` Otherwise, code lower down will remove the that `grub.cfg` from the list of kargs files. This in turn will prevent `coreos-installer iso kargs` from affecting kargs on UEFI boots. Fixes 3991e6f06 ("cmd-buildextend-live: always change dir `EFI/{vendor}/` to correct vendor id"). --- src/cmd-buildextend-live | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/cmd-buildextend-live b/src/cmd-buildextend-live index 39249d81f0..e4d23ec567 100755 --- a/src/cmd-buildextend-live +++ b/src/cmd-buildextend-live @@ -551,6 +551,11 @@ def generate_iso(): os.rename(srcpath, f"EFI/{vendor_id}", src_dir_fd=dfd, dst_dir_fd=dfd) os.close(dfd) + # And update kargs.json + for file in kargs_json['files']: + if file['path'] == grubfilepath[0]: + file['path'] = f'EFI/{vendor_id}/grub.cfg' + # Delete fallback and its CSV file. Its purpose is to create # EFI boot variables, which we don't want when booting from # removable media. From 37b7f46be4d15fd9d844ee84fa91f565afe69750 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Mon, 7 Nov 2022 09:34:44 -0500 Subject: [PATCH 4/5] cmd-buildextend-live: ensure files exist before writing `kargs.json` Rather than silently deleting files that no longer exist, flip this around so that we assert they all exist. This will then ensure that new code moving `grub.cfg` will also have to update `kargs_json`. --- src/cmd-buildextend-live | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/cmd-buildextend-live b/src/cmd-buildextend-live index e4d23ec567..c95064f810 100755 --- a/src/cmd-buildextend-live +++ b/src/cmd-buildextend-live @@ -625,10 +625,11 @@ boot '-efi-boot', 'images/efiboot.img', '-no-emul-boot'] - # We've done everything that might affect kargs, so filter out any files - # that no longer exist and write out the kargs JSON if it lists any files - kargs_json['files'] = [f for f in kargs_json['files'] - if os.path.exists(os.path.join(tmpisoroot, f['path']))] + # Sanity-check that all kargs files that we found earlier still exist. This + # ensures that any modifications made also updated kargs_json as needed. + for f in kargs_json['files']: + fn = os.path.join(tmpisoroot, f['path']) + assert os.path.exists(fn), f"{fn} no longer exists" kargs_json['files'].sort(key=lambda f: f['path']) if kargs_json['files']: # Store the location of "karg embed areas" for use by From 0bd4e723f50f0e7e8e79719bb231064cdec67ea4 Mon Sep 17 00:00:00 2001 From: Jonathan Lebon Date: Mon, 7 Nov 2022 12:29:43 -0500 Subject: [PATCH 5/5] cmd-buildextend-live: avoid no-op `renameat(2)` for FCOS On FCOS, the EFI directory is already correctly named in the src config repo. So there's no need to do any renames at all. The previous code still worked because `renameat(2)` with the src and dest being the same file is a no-op. This is a bit too implicit though. Let's just add a check to be more clear. This avoids us also unnecessarily iterating over `kargs_json['files']`. While here, add a print statement when this logic kicks in. --- src/cmd-buildextend-live | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/cmd-buildextend-live b/src/cmd-buildextend-live index c95064f810..c913cf667e 100755 --- a/src/cmd-buildextend-live +++ b/src/cmd-buildextend-live @@ -548,14 +548,15 @@ def generate_iso(): if len(grubfilepath) != 1: raise Exception(f'Found != 1 grub.cfg files: {grubfilepath}') srcpath = os.path.dirname(grubfilepath[0]) - os.rename(srcpath, f"EFI/{vendor_id}", src_dir_fd=dfd, dst_dir_fd=dfd) + if srcpath != f'EFI/{vendor_id}': + print(f"Renaming '{srcpath}' to 'EFI/{vendor_id}'") + os.rename(srcpath, f"EFI/{vendor_id}", src_dir_fd=dfd, dst_dir_fd=dfd) + # And update kargs.json + for file in kargs_json['files']: + if file['path'] == grubfilepath[0]: + file['path'] = f'EFI/{vendor_id}/grub.cfg' os.close(dfd) - # And update kargs.json - for file in kargs_json['files']: - if file['path'] == grubfilepath[0]: - file['path'] = f'EFI/{vendor_id}/grub.cfg' - # Delete fallback and its CSV file. Its purpose is to create # EFI boot variables, which we don't want when booting from # removable media.