Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Restore on UEFI hosts (#150) #151

Open
wants to merge 2 commits into
base: release/xs8
Choose a base branch
from
Open

Fix Restore on UEFI hosts (#150) #151

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2701315
restore: read from backup inventory before unmounting (#150)
ydirson Jul 3, 2024
9b63a88
restore: fix access to ESP partition
ydirson Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions restore.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,11 @@ def restore_partitions():
util.assertDir(os.path.dirname(dst_file))
boot_config.commit(dst_file)

# prepare for boot loader restoration, before unmounting backup_fs
if boot_config.src_fmt == 'grub2' and efi_boot:
branding = util.readKeyValueFile(os.path.join(backup_fs.mount_point, constants.INVENTORY_FILE))
branding['product-brand'] = branding['PRODUCT_BRAND']

# repartition if needed
backup_fs.unmount()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just move this line inside the below if ?
We are restoring MBR so we won't have EFI to restore.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possibly, but that rather looks like some improvement that can wait for later, here I'm solely focused on unbreaking the nominal case

if restore_partitions:
Expand All @@ -209,8 +214,7 @@ def restore_partitions():
# restore boot loader
if boot_config.src_fmt == 'grub2':
if efi_boot:
branding = util.readKeyValueFile(os.path.join(backup_fs.mount_point, constants.INVENTORY_FILE))
branding['product-brand'] = branding['PRODUCT_BRAND']
mounts['esp'] = esp
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you pointed out on https://github.com/xenserver/host-installer/pull/161/files, these solve the same issue
I think adding esp to mounts should happen when the mounting occurs in case any future changes need to inspect what is mounted and rely on that mounts dictionary
With your change here, it would incorrectly appear as if esp is not mounted until this code is run
(Another improvement would be to use 'esp' in mounts instead of if efi_mounted but that's for another PR)

backend.setEfiBootEntry(mounts, disk_device, boot_partnum, constants.INSTALL_TYPE_RESTORE, branding)
else:
if location == constants.BOOT_LOCATION_MBR:
Expand Down