curtin-dev team mailing list archive

[Paride Legovini <paride.legovini@xxxxxxxxxxxxx>]

Review: Needs Fixing

LGTM, the needs-fixing is just for the typo (see inline comment).

In another inline comment I suggest documenting the reason for the BootOrder reordering a bit more, but take it with "importance: nit".

I fixed a typo in the MP commit message s/perform/performance/. It's still present in the commit message for 3e1a8, but that will be squashed by CI.

Diff comments:

> diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
> index 458eb9d..5fbd5b8 100644
> --- a/curtin/commands/curthooks.py
> +++ b/curtin/commands/curthooks.py
> @@ -456,6 +464,40 @@ def uefi_reorder_loaders(grubcfg, target):
>              LOG.debug(
>                  "Setting currently booted %s as the first "
>                  "UEFI loader.", currently_booted)
> +        else:
> +            reason = (
> +                "config 'reorder_uefi_force_fallback' is True" if
> +                force_fallback_reorder else "missing 'BootCurrent' value")
> +            LOG.debug("Using fallback UEFI reordering: " + reason)
> +            if len(boot_order) < 2:
> +                LOG.debug(
> +                    'UEFI BootOrder has less than 2 entriesi, cannot reorder')

typo

> +                return
> +            # look at efi entries before we added one to find the new addition
> +            if efi_orig is not None:
> +                LOG.debug("Finding newly install entry and moving it back one")
> +                # find the newly installed entry, and move it back one
> +                # spot, (new entry == 0004)
> +                # orig = [0002, 0001, 0003, 0000]
> +                # new  = [0002, 0004, 0001, 0003, 0000]
> +                #  \
> +                #   ->   [0002, 0001, 0004, 0003, 0000]
> +                new_entries = set(boot_order).difference(
> +                    set(efi_orig['order']))
> +                for entry in new_entries:
> +                    # insert a copy on the other side of the next
> +                    new_spot = boot_order.index(entry) + 2
> +                    boot_order.insert(new_spot, entry)
> +                    boot_order.remove(entry)
> +            else:
> +                LOG.debug("Blindly moving first entry back one")
> +                # swap the first and second entry in order list
> +                # [0000, 0002, 0001] -> [0002, 0000, 0001]
> +                boot_order.insert(2, boot_order[0])
> +                boot_order = [bootnum for bootnum in boot_order[1:]]
> +            new_boot_order = ','.join(boot_order)
> +
> +        if new_boot_order:
>              LOG.debug(
>                  "New UEFI boot order: %s", new_boot_order)
>              with util.ChrootableTarget(target) as in_chroot:
> diff --git a/doc/topics/config.rst b/doc/topics/config.rst
> index 7f8396e..5ce1a4b 100644
> --- a/doc/topics/config.rst
> +++ b/doc/topics/config.rst
> @@ -226,6 +226,27 @@ not provided, Curtin will set the value to 'console'.  If the ``terminal``
>  value is 'unmodified' then Curtin will not set any value at all and will
>  use Grub defaults.
>  
> +**reorder_uefi**: *<boolean: default True>*
> +
> +On UEFI systems curtin will modify the UEFI BootOrder settings to place
> +the currently booted entry (BootCurrent) to the first option after installing
> +the new target OS into the UEFI boot menu.  The result is that the system
> +will boot from the same device that it booted to run curtin.
> +

I'd add a sentence or two here explaining why we want to give precedence to the currently booted device/entry over the newly installed system (install media is normally ejected but if it's not then the installer should start again + the PXE boot thing + other reasons I may have missed), as at first glance the most logical thing to do seems to be giving precedence to the new system.

> +This setting is ignored if *update_nvram* is False.
> +
> +
> +**reorder_uefi_force_fallback**: *<boolean: default False>*
> +
> +On some UEFI systems the BootCurrent entry may not be present.  This can
> +cause a system to not boot to the same device that it was previously booting.
> +If BootCurrent is not present, curtin will attempt to move the newly added
> +boot entry after the previous first entry in BootOrder.  If
> +*reorder_uefi_force_fallback* is True, curtin will ignore BootCurrent value and
> +use the fallback reordering method.
> +
> +This setting is ignored if *update_nvram* or *reorder_uefi* are False.
> +
>  
>  **Example**::
>  


-- 
https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/387981
Your team curtin developers is subscribed to branch curtin:master.