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
