Hey Pascal, and thanks for the review!

Overall comment: I'm not trying to make the heuristics 100% reliable
here, as I don't think that's actually possible. Instead, I'm trying
to tread the fine line of:

 * minimising false negatives - let's try to pick up on the most
   common cases where people are dual-booting with other systems and
   might not understand the issues here. That's 99%+ going to be
   people with Windows installed

 * minimising false positives - the issue that angered Cyril in
   particular, with an incomplete LVM setup triggering the "bios
   bootable OS" warning

On Mon, Apr 10, 2023 at 01:01:01PM +0200, Pascal Hambourg wrote:
>partman-efi "Fix detection of BIOS-bootable systems" provides a significant
>improvement over previous behaviour. However I have a few comments.
>
>1a) The patch assumes that a GPT disk may be BIOS-bootable only if it has a
>BIOS boot partition. But a GPT disk can be BIOS-bootable even without a BIOS
>boot partition:
>- GRUB may be installed without a BIOS boot partition if /boot is a plain
>partition (using blocklists), even though it is less reliable so a BIOS boot
>partition is strongly recommended.

Yeah, GRUB installed using blocklists is so much *not* a thing anybody
should be doing these days.

>- Other BIOS boot loaders such as syslinux/extlinux do not need or use a BIOS
>boot partition.

Also not a use case I'm particularly caring about, I'll be
honest. They're also *really* not likely to work well without another
filesystem in use, which I expect we'll detect anyway.

>1b) IIUC the patch fixes #1033913 because the disk selected for installation
>has received a new GPT disklabel without a BIOS boot partition, so further
>checking is skipped. But IMO the root cause of #1033913 is that changes are
>not committed to disk after setting the 'boot' and 'esp' flags to the newly
>created ESP partition before stopping parted_server.
>This can be seen in /var/log/partman:
>
>/bin/autopartition-lvm
>NEW_LABEL sda gpt
>NEW_PARTITION 1 sda ext2 538MB (future ESP)
>NEW_PARTITION 2 sda ext2 512MB (future /boot)
>NEW_PARTITION 3 sda ext3 159GB (future LVM)
>SET_FLAGS sda3 lvm
>(user prompt to write changes to the disk)
>COMMIT sda
>...
>/lib/partman/update.d/21efi_sync_flag
>SET_FLAGS sda1 boot esp
>...
>/bin/perform_recipe_by_lvm
>QUIT <- esp and boot flags have not been committed yet so are lost
>...
>/lib/partman/init.d/50efi
>GET_FLAGS sda1 -> none
>
>2) The patch considers the 'esp' and 'boot' flags to be equal. But this is
>true only with GPT. With MSDOS, they have totally different meanings:
>- 'esp' means that the partition has the ESP type identifier.
>- 'boot' means that the partition has the active/boot indicator set. The UEFI
>specification says that this indicator is ignored by EFI boot.

ACK, I think you're correct here. Yay parted and its inconsistent
"flags" concept. :-(

>3) The patch considers LVM and RAID partitions not bootable. But both LVM and
>RAID superblocks can have a boot loader reserved area. Also, GRUB may boot
>them directly without a /boot partition.

Hmmm, maybe.

>4) It appears that partman fails to detect the specially crafted partition
>table on the installation media created with a debian image. Is it intended
>or fortunately unintentional ? If partman could see the EFI partition on the
>installation media, the detection of BIOS-bootable systems would fail.

That's not a worry for today... :-)

-- 
Steve McIntyre, Cambridge, UK.                                st...@einval.com
You lock the door
And throw away the key
There's someone in my head but it's not me 

Reply via email to