On 12/15/22 20:29, Andrey Drobyshev wrote: > If the guest uses BIOS firmware but GPT partitioned disk, and in > addition has "bios, esp" flag enabled on its "/boot" partition, then > inspection wrongly detects UEFI firmware and v2v ends up with the error: > "error: no bootloader detected". > > Let's fix this by checking for the presence of BIOS boot partition (0xef02 > type for gdisk, "bios_grub" flag for parted), which is used to store a > bootloader code in BIOS+GPT configurations. If such a partition is > present, then it's likely a BIOS+GPT setup. > > Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com&gt; > --- > convert/inspect_source.ml | 27 +++++++++++++++++++++++---- > 1 file changed, 23 insertions(+), 4 deletions(-) > > diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml > index 056d0bca..6650e136 100644 > --- a/convert/inspect_source.ml > +++ b/convert/inspect_source.ml > @@ -219,6 +219,9 @@ and list_applications g root = function > (* See if this guest could use UEFI to boot. It should use GPT and > * it should have an EFI System Partition (ESP). > * > + * If it has a BIOS boot partition (BIOS+GPT setup), then [BIOS] is > + * returned. > + * > * If it has ESP(s), then [UEFI devs] is returned where [devs] is the > * list of at least one ESP. > * > @@ -226,9 +229,13 @@ and list_applications g root = function > *) > and get_firmware_bootable_device g = > let rec uefi_ESP_guid = "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" > + and bios_boot_guid = "21686148-6449-6E6F-744E-656564454649" > and is_uefi_ESP dev part = > let partnum = g#part_to_partnum part in > g#part_get_gpt_type dev partnum = uefi_ESP_guid > + and is_bios_boot dev part = > + let partnum = g#part_to_partnum part in > + g#part_get_gpt_type dev partnum = bios_boot_guid > and parttype_is_gpt dev = > try g#part_get_parttype dev = "gpt" > with G.Error msg as exn -> > @@ -239,14 +246,26 @@ and get_firmware_bootable_device g = > and is_uefi_bootable_part part = > let dev = g#part_to_dev part in > parttype_is_gpt dev && is_uefi_ESP dev part > + and is_bios_gpt_part part = > + let dev = g#part_to_dev part in > + parttype_is_gpt dev && is_bios_boot dev part > in > > let partitions = Array.to_list (g#list_partitions ()) in > - let partitions = List.filter is_uefi_bootable_part partitions in > + let esp_partitions = List.filter is_uefi_bootable_part partitions in > > - match partitions with > - | [] -> I_BIOS > - | partitions -> I_UEFI partitions > + (* If there's a BIOS boot partition present (0xef02 type for gdisk, > + * "bios_grub" flag for parted), then this is likely a BIOS+GPT setup. > + * Note that if a source VM is using UEFI firmware and has a secondary > + * non-bootable disk attached which contains such a partition, the > + * firmware detection will detect I_BIOS wrongly. But this can only be > + * done manually, and we assume that there's no point doing it on purpose. > + *) > + if List.exists is_bios_gpt_part partitions then I_BIOS > + else > + match esp_partitions with > + | [] -> I_BIOS > + | esp_partitions -> I_UEFI esp_partitions > > (* If some inspection fields are "unknown", then that indicates a > * failure in inspection, and we shouldn't continue. For an example I've now re-read: https://en.wikipedia.org/wiki/GUID_Partition_Table https://en.wikipedia.org/wiki/BIOS_boot_partition and the proposed change seems to make sense. Effectively it adds an exception: in case we have at leasts one UEFI system partition (GPT partition entry with type GUID C12A7328-F81F-11D2-BA4B-00A0C93EC93B), but also a BIOS boot partition (GPT partition entry with type GUID 21686148-6449-6E6F-744E-656564454649), then state that the system under conversion uses BIOS and not UEFI. What I don't understand is what kind of system exposes such a partition scheme, requiring the above exception, for conversion. Can you describe that? Furthermore, the commit message says, has "bios, esp" flag enabled on its "/boot" partition and I totally don't understand that. First, the /boot partition is orthogonal to both the UEFI system partition and the BIOS boot partition. The above wikipedia article, and various *new* specs: https://uapi-group.org/specifications/specs/discoverable_partitions_speci... https://www.freedesktop.org/software/systemd/man/systemd-gpt-auto-generat... try to make us believe that /boot is supposed to be described with type GUID bc13c2ff-59e6-4262-a352-b275fd6f7172. But in fact, on my just-installed RHEL-9.1 system, the /boot partition has type GUID 0FC63DAF-8483-4772-8E79-3D69D8477DE4 "Linux filesystem data". Second, I don't understand the "bios" and "esp" references as *flags*. The Partition attributes at <https://en.wikipedia.org/wiki/GUID_Partition_Table#Partition_entries_(LBA... includes "Legacy BIOS bootable", but "esp" does not seem like a flag. Third, while the commit message speaks about these flags, the code does not appear to deal with them at all. So here's what I suggest: (1) In the commit message, describe *precisely* a GPT partition table that currently breaks inspection. Best provide an actual example dump, with partition type GUIDs and partition attributes. (2) Furthermore, please describe what concrete system or partitioning utility the partition table comes from. (3) The "bios, esp" paragraph looks like a distraction, so I suggest dropping it. (4) Please clarify the second paragraph by saying what the code will do -- if the GPT has a partition entry with type GUID 21686148-6449-6E6F-744E-656564454649 ("BIOS boot partition"), then we'll give priority to that and mark the system as BIOS, even if there's another partition entry with type GUID C12A7328-F81F-11D2-BA4B-00A0C93EC93B ("EFI system partition"). (5) I don't really like the runtime logic duplication that seems to be introduced by the patch. By iterating over the list of partitions twice, we make twice as many libguestfs calls. I think we should iterate only once over the list of partitions. I recommend a refactoring patch first: > diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml > index 056d0bca62e6..0dad9fc56572 100644 > --- a/convert/inspect_source.ml > +++ b/convert/inspect_source.ml > @@ -224,30 +224,32 @@ and list_applications g root = function > * > * Otherwise, [BIOS] is returned. > *) > and get_firmware_bootable_device g = > - let rec uefi_ESP_guid = "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" > - and is_uefi_ESP dev part = > - let partnum = g#part_to_partnum part in > - g#part_get_gpt_type dev partnum = uefi_ESP_guid > + let esp_partitions = ref [] > and parttype_is_gpt dev = > try g#part_get_parttype dev = "gpt" > with G.Error msg as exn -> > (* If it's _not_ "unrecognised disk label" then re-raise it. *) > if g#last_errno () <> G.Errno.errno_EINVAL then raise exn; > debug "%s (ignored)" msg; > false > - and is_uefi_bootable_part part = > - let dev = g#part_to_dev part in > - parttype_is_gpt dev && is_uefi_ESP dev part > in > > - let partitions = Array.to_list (g#list_partitions ()) in > - let partitions = List.filter is_uefi_bootable_part partitions in > + Array.iter (fun part -> > + let dev = g#part_to_dev part in > + if (parttype_is_gpt dev) then > + let partnum = g#part_to_partnum part in > + let part_type_guid = g#part_get_gpt_type dev partnum in > + match part_type_guid with > + | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> > + esp_partitions := part :: !esp_partitions > + | _ -> () > + ) (g#list_partitions ()); > > - match partitions with > + match !esp_partitions with > | [] -> I_BIOS > - | partitions -> I_UEFI partitions > + | esp_partitions -> I_UEFI esp_partitions > > (* If some inspection fields are "unknown", then that indicates a > * failure in inspection, and we shouldn't continue. For an example > * of this, see RHBZ#1278371. However don't "assert" here, since and then a simpler extension: > diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml > index 0dad9fc56572..a8dece3d4b5f 100644 > --- a/convert/inspect_source.ml > +++ b/convert/inspect_source.ml > @@ -226,6 +226,7 @@ and list_applications g root = function > *) > and get_firmware_bootable_device g = > let esp_partitions = ref [] > + and bios_boot = ref false > and parttype_is_gpt dev = > try g#part_get_parttype dev = "gpt" > with G.Error msg as exn -> > @@ -243,12 +244,14 @@ and get_firmware_bootable_device g = > match part_type_guid with > | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> > esp_partitions := part :: !esp_partitions > + | "21686148-6449-6E6F-744E-656564454649" -> > + bios_boot := true > | _ -> () > ) (g#list_partitions ()); > > - match !esp_partitions with > - | [] -> I_BIOS > - | esp_partitions -> I_UEFI esp_partitions > + match !esp_partitions, !bios_boot with > + | _ :: _ as esp_partitions, false -> I_UEFI esp_partitions > + | _ -> I_BIOS > > (* If some inspection fields are "unknown", then that indicates a > * failure in inspection, and we shouldn't continue. For an example

On 12/16/22 13:13, Laszlo Ersek wrote: > On 12/15/22 20:29, Andrey Drobyshev wrote: >> If the guest uses BIOS firmware but GPT partitioned disk, and in >> addition has "bios, esp" flag enabled on its "/boot" partition, then >> inspection wrongly detects UEFI firmware and v2v ends up with the error: >> "error: no bootloader detected". >> >> Let's fix this by checking for the presence of BIOS boot partition (0xef02 >> type for gdisk, "bios_grub" flag for parted), which is used to store a >> bootloader code in BIOS+GPT configurations. If such a partition is >> present, then it's likely a BIOS+GPT setup. >> >> Signed-off-by: Andrey Drobyshev <andrey.drobyshev(a)virtuozzo.com&gt; >> --- >> convert/inspect_source.ml | 27 +++++++++++++++++++++++---- >> 1 file changed, 23 insertions(+), 4 deletions(-) >> >> diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml >> index 056d0bca..6650e136 100644 >> --- a/convert/inspect_source.ml >> +++ b/convert/inspect_source.ml >> @@ -219,6 +219,9 @@ and list_applications g root = function >> (* See if this guest could use UEFI to boot. It should use GPT and >> * it should have an EFI System Partition (ESP). >> * >> + * If it has a BIOS boot partition (BIOS+GPT setup), then [BIOS] is >> + * returned. >> + * >> * If it has ESP(s), then [UEFI devs] is returned where [devs] is the >> * list of at least one ESP. >> * >> @@ -226,9 +229,13 @@ and list_applications g root = function >> *) >> and get_firmware_bootable_device g = >> let rec uefi_ESP_guid = "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" >> + and bios_boot_guid = "21686148-6449-6E6F-744E-656564454649" >> and is_uefi_ESP dev part = >> let partnum = g#part_to_partnum part in >> g#part_get_gpt_type dev partnum = uefi_ESP_guid >> + and is_bios_boot dev part = >> + let partnum = g#part_to_partnum part in >> + g#part_get_gpt_type dev partnum = bios_boot_guid >> and parttype_is_gpt dev = >> try g#part_get_parttype dev = "gpt" >> with G.Error msg as exn -> >> @@ -239,14 +246,26 @@ and get_firmware_bootable_device g = >> and is_uefi_bootable_part part = >> let dev = g#part_to_dev part in >> parttype_is_gpt dev && is_uefi_ESP dev part >> + and is_bios_gpt_part part = >> + let dev = g#part_to_dev part in >> + parttype_is_gpt dev && is_bios_boot dev part >> in >> >> let partitions = Array.to_list (g#list_partitions ()) in >> - let partitions = List.filter is_uefi_bootable_part partitions in >> + let esp_partitions = List.filter is_uefi_bootable_part partitions in >> >> - match partitions with >> - | [] -> I_BIOS >> - | partitions -> I_UEFI partitions >> + (* If there's a BIOS boot partition present (0xef02 type for gdisk, >> + * "bios_grub" flag for parted), then this is likely a BIOS+GPT setup. >> + * Note that if a source VM is using UEFI firmware and has a secondary >> + * non-bootable disk attached which contains such a partition, the >> + * firmware detection will detect I_BIOS wrongly. But this can only be >> + * done manually, and we assume that there's no point doing it on purpose. >> + *) >> + if List.exists is_bios_gpt_part partitions then I_BIOS >> + else >> + match esp_partitions with >> + | [] -> I_BIOS >> + | esp_partitions -> I_UEFI esp_partitions >> >> (* If some inspection fields are "unknown", then that indicates a >> * failure in inspection, and we shouldn't continue. For an example > > I've now re-read: > > https://en.wikipedia.org/wiki/GUID_Partition_Table > https://en.wikipedia.org/wiki/BIOS_boot_partition > > and the proposed change seems to make sense. Effectively it adds an > exception: in case we have at leasts one UEFI system partition (GPT > partition entry with type GUID C12A7328-F81F-11D2-BA4B-00A0C93EC93B), > but also a BIOS boot partition (GPT partition entry with type GUID > 21686148-6449-6E6F-744E-656564454649), then state that the system > under conversion uses BIOS and not UEFI. > > What I don't understand is what kind of system exposes such a > partition scheme, requiring the above exception, for conversion. Can > you describe that? Actually this is somewhat of a mystery for me as well, cause this fix originated from a customer bug. There was a Debian 8 guest with the following layout: # parted -l Model: ATA harddisk (scsi) Disk /dev/sda: 1288GB Sector size (logical/physical): 512B/4096B Partition Table: gpt Disk Flags: pmbr_boot Number Start End Size File system Name Flags 4 1049kB 2097kB 1049kB primary bios_grub 1 2097kB 4297MB 4295MB linux-swap(v1) primary 2 4297MB 4559MB 262MB ext4 primary boot, esp 3 4559MB 1288GB 1284GB ext4 primary

‘boot’ (Mac, MS-DOS, PC98) - should be enabled if you want to boot off the partition. The semantics vary between disk labels. For MS-DOS disk labels, only one partition can be bootable. If you are installing LILO on a partition that partition must be bootable. For PC98 disk labels, all ext2 partitions must be bootable (this is enforced by Parted). ‘esp’ (MS-DOS, GPT) - this flag identifies a UEFI System Partition. On GPT it is an alias for boot.

AFAIU an OS installer wouldn't create a partition scheme that way, even when dealing with GPT: there is either a BIOS boot partition (BIOS firmware) or a EFI system partition (UEFI firmware). So I can't really tell how we ended up with a scheme like this. But this article: https://en.wikipedia.org/wiki/Boot_flag claims that > Some BIOSes test if the boot flag of at least one partition is set, otherwise they ignore the device in boot-order.

So could it be that this boot flag was manually set "just in case"? Another *possible* scenario to consider: - This disk image is initially used in an UEFI powered VM, hence partition 2 has type "EFI system partition". No dedicated /boot partition; - The very same disk is then reused in a BIOS powered VM (without OS installation); - To be used in such a fashion, disk is re-partitioned so that * Partition 2 is reused as /boot now, but parttype isn't changed; * Swap partition is shrinked and BIOS boot partition is created in the beginning of the disk (the fact that it's numbered 4 advocates that version).

In any case, I think it's safe to assume that firmware detection was manually and artificially "broken" in this case. Moreover, even with this patch applied it can be manually broken as well -- just as I've mentioned in the code comments, by adding a secondary non-bootable disk with BIOS boot partition on it. But IMO it's also safe to assume that the latter scenario is less probable to happen accidentally.

> > Furthermore, the commit message says, > > has "bios, esp" flag enabled on its "/boot" partition > > and I totally don't understand that. > > First, the /boot partition is orthogonal to both the UEFI system > partition and the BIOS boot partition. The above wikipedia article, > and various *new* specs: > > https://uapi-group.org/specifications/specs/discoverable_partitions_speci... > https://www.freedesktop.org/software/systemd/man/systemd-gpt-auto-generat... > > try to make us believe that /boot is supposed to be described with > type GUID bc13c2ff-59e6-4262-a352-b275fd6f7172. But in fact, on my > just-installed RHEL-9.1 system, the /boot partition has type GUID > 0FC63DAF-8483-4772-8E79-3D69D8477DE4 "Linux filesystem data". You're right, "bios" and "esp" have nothing to do with each other, that is a typo. I meant to say "boot, esp" (see below). > > Second, I don't understand the "bios" and "esp" references as > *flags*. The Partition attributes at > <https://en.wikipedia.org/wiki/GUID_Partition_Table#Partition_entries_(LBA... > includes "Legacy BIOS bootable", but "esp" does not seem like a flag. This is the terminology used by parted: https://www.gnu.org/software/parted/manual/html_node/set.html Namely, the docs say: > ‘esp’ > (MS-DOS, GPT) - this flag identifies a UEFI System Partition. On GPT it is an alias for boot. So in case of a GPT partitioned disk it treats those as complete synonyms, as such: # parted -l /dev/sda | egrep '^\s*2' 2 2097kB 271MB 268MB ext4 # parted /dev/sda "set 2 esp on" # parted -l /dev/sda | egrep '^\s*2' 2 2097kB 271MB 268MB ext4 boot, esp # parted /dev/sda "set 2 boot off" # parted -l /dev/sda | egrep '^\s*2' 2 2097kB 271MB 268MB ext4 But then this flags aren't being treated by partition attributes, but rather being interpreted by the partition tool to decide which parttype to write in the partition table for that particular entry: # parted -l /dev/sda | egrep '^\s*2' 2 2097kB 271MB 268MB ext4 <-- (no flags) # lsblk -p -o NAME,PARTTYPE /dev/sda2 NAME PARTTYPE /dev/sda2 0fc63daf-8483-4772-8e79-3d69d8477de4 <-- (linux filesystem) # parted /dev/sda "set 2 boot on" # lsblk -p -o NAME,PARTTYPE /dev/sda2 NAME PARTTYPE /dev/sda2 c12a7328-f81f-11d2-ba4b-00a0c93ec93b <-- (EFI partition) > > Third, while the commit message speaks about these flags, the code > does not appear to deal with them at all. Again, in this context, as you can see, flags and parttypes are pretty much interchangeable, and we actually are dealing with them. But I agree that it could've been stated more clearly.

> > So here's what I suggest: > > (1) In the commit message, describe *precisely* a GPT partition table > that currently breaks inspection. Best provide an actual example > dump, with partition type GUIDs and partition attributes. Can specify the partition scheme from above. > > (2) Furthermore, please describe what concrete system or partitioning > utility the partition table comes from. > > (3) The "bios, esp" paragraph looks like a distraction, so I suggest > dropping it. Should probably substitute that with the particular parttype GUIDs. > > (4) Please clarify the second paragraph by saying what the code will > do -- if the GPT has a partition entry with type GUID > 21686148-6449-6E6F-744E-656564454649 ("BIOS boot partition"), then > we'll give priority to that and mark the system as BIOS, even if > there's another partition entry with type GUID > C12A7328-F81F-11D2-BA4B-00A0C93EC93B ("EFI system partition"). Sure.

> > (5) I don't really like the runtime logic duplication that seems to > be introduced by the patch. By iterating over the list of partitions > twice, we make twice as many libguestfs calls. I think we should > iterate only once over the list of partitions. Agreed, iterating just once is obviously better. I'll try to come up with the code which is less obscure. For instance, List.map with a custom function, taking partition as its argument and returning a tuple (Some part * bool). This way we'd get a list of tuples which you referred to in your other comment.

> I recommend a refactoring patch first: > >> diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml >> index 056d0bca62e6..0dad9fc56572 100644 >> --- a/convert/inspect_source.ml >> +++ b/convert/inspect_source.ml >> @@ -224,30 +224,32 @@ and list_applications g root = function >> * >> * Otherwise, [BIOS] is returned. >> *) >> and get_firmware_bootable_device g = >> - let rec uefi_ESP_guid = "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" >> - and is_uefi_ESP dev part = >> - let partnum = g#part_to_partnum part in >> - g#part_get_gpt_type dev partnum = uefi_ESP_guid >> + let esp_partitions = ref [] >> and parttype_is_gpt dev = >> try g#part_get_parttype dev = "gpt" >> with G.Error msg as exn -> >> (* If it's _not_ "unrecognised disk label" then re-raise it. *) >> if g#last_errno () <> G.Errno.errno_EINVAL then raise exn; >> debug "%s (ignored)" msg; >> false >> - and is_uefi_bootable_part part = >> - let dev = g#part_to_dev part in >> - parttype_is_gpt dev && is_uefi_ESP dev part >> in >> >> - let partitions = Array.to_list (g#list_partitions ()) in >> - let partitions = List.filter is_uefi_bootable_part partitions in >> + Array.iter (fun part -> >> + let dev = g#part_to_dev part in >> + if (parttype_is_gpt dev) then >> + let partnum = g#part_to_partnum part in >> + let part_type_guid = g#part_get_gpt_type dev partnum in >> + match part_type_guid with >> + | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> >> + esp_partitions := part :: !esp_partitions >> + | _ -> () >> + ) (g#list_partitions ()); >> >> - match partitions with >> + match !esp_partitions with >> | [] -> I_BIOS >> - | partitions -> I_UEFI partitions >> + | esp_partitions -> I_UEFI esp_partitions >> >> (* If some inspection fields are "unknown", then that indicates a >> * failure in inspection, and we shouldn't continue. For an example >> * of this, see RHBZ#1278371. However don't "assert" here, since > > and then a simpler extension: > >> diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml >> index 0dad9fc56572..a8dece3d4b5f 100644 >> --- a/convert/inspect_source.ml >> +++ b/convert/inspect_source.ml >> @@ -226,6 +226,7 @@ and list_applications g root = function >> *) >> and get_firmware_bootable_device g = >> let esp_partitions = ref [] >> + and bios_boot = ref false >> and parttype_is_gpt dev = >> try g#part_get_parttype dev = "gpt" >> with G.Error msg as exn -> >> @@ -243,12 +244,14 @@ and get_firmware_bootable_device g = >> match part_type_guid with >> | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> >> esp_partitions := part :: !esp_partitions >> + | "21686148-6449-6E6F-744E-656564454649" -> >> + bios_boot := true >> | _ -> () >> ) (g#list_partitions ()); >> >> - match !esp_partitions with >> - | [] -> I_BIOS >> - | esp_partitions -> I_UEFI esp_partitions >> + match !esp_partitions, !bios_boot with >> + | _ :: _ as esp_partitions, false -> I_UEFI esp_partitions >> + | _ -> I_BIOS >> >> (* If some inspection fields are "unknown", then that indicates a >> * failure in inspection, and we shouldn't continue. For an example > > Laszlo >

Now I'm a bit confused myself as a result of our discussion: should we really give priority to BIOS Boot partition? Because I tried to replay the opposite scenario -- i.e. to convert a disk previously used by BIOS machine to be used by UEFI machine, but leaving BIOS Boot partition with its parttype unchanged. And it failed to no surprise with this patch applied. Do you have a clearer understanding of what to prioritize and why?

>>> (5) I don't really like the runtime logic duplication that seems to >>> be introduced by the patch. By iterating over the list of >>> partitions twice, we make twice as many libguestfs calls. I think >>> we should iterate only once over the list of partitions. >> >> Agreed, iterating just once is obviously better. I'll try to come >> up with the code which is less obscure. For instance, List.map with >> a custom function, taking partition as its argument and returning a >> tuple (Some part * bool). This way we'd get a list of tuples which >> you referred to in your other comment. > > Can you try out the patches I proposed below? > > I agree that List.map + List.fold would work, but then (a) you don't > need a separate mapping step just so you can fold the mapped list: > you can push the mapping down into the folder callback; (b) I > honestly doubt in this case that the purely functional implementation > would be easier to read. Anyway, if you prefer that, I'm certainly OK > with it. Well, to my view the necessity to use ref's make it a bit clumsy, sort of like using global variables where they could've been avoided. Would you consider this (functional) use of List.exists, List.filter_map?

diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml index 056d0bca62e6..9686f9fff8d8 100644 --- a/convert/inspect_source.ml +++ b/convert/inspect_source.ml @@ -225,26 +225,29 @@ and list_applications g root = function * Otherwise, [BIOS] is returned. *) and get_firmware_bootable_device g = - let rec uefi_ESP_guid = "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" - and is_uefi_ESP dev part = - let partnum = g#part_to_partnum part in - g#part_get_gpt_type dev partnum = uefi_ESP_guid - and parttype_is_gpt dev = + let parttype_is_gpt dev = try g#part_get_parttype dev = "gpt" with G.Error msg as exn -> (* If it's _not_ "unrecognised disk label" then re-raise it. *) if g#last_errno () <> G.Errno.errno_EINVAL then raise exn; debug "%s (ignored)" msg; false - and is_uefi_bootable_part part = + in + let accumulate_partition esp_parts part = let dev = g#part_to_dev part in - parttype_is_gpt dev && is_uefi_ESP dev part + if parttype_is_gpt dev then + let partnum = g#part_to_partnum part in + let part_type_guid = g#part_get_gpt_type dev partnum in + match part_type_guid with + | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> part :: esp_parts + | _ -> esp_parts + else esp_parts in - let partitions = Array.to_list (g#list_partitions ()) in - let partitions = List.filter is_uefi_bootable_part partitions in + let esp_partitions = + Array.fold_left accumulate_partition [] (g#list_partitions ()) in - match partitions with + match esp_partitions with | [] -> I_BIOS | partitions -> I_UEFI partitions

diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml index 9686f9fff8d8..8e8f5c45f1eb 100644 --- a/convert/inspect_source.ml +++ b/convert/inspect_source.ml @@ -233,23 +233,24 @@ and get_firmware_bootable_device g = debug "%s (ignored)" msg; false in - let accumulate_partition esp_parts part = + let accumulate_partition (esp_parts, bboot) part = let dev = g#part_to_dev part in if parttype_is_gpt dev then let partnum = g#part_to_partnum part in let part_type_guid = g#part_get_gpt_type dev partnum in match part_type_guid with - | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> part :: esp_parts - | _ -> esp_parts - else esp_parts + | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> part :: esp_parts, bboot + | "21686148-6449-6E6F-744E-656564454649" -> esp_parts, true + | _ -> esp_parts, bboot + else esp_parts, bboot in - let esp_partitions = - Array.fold_left accumulate_partition [] (g#list_partitions ()) in + let esp_partitions, bios_boot = + Array.fold_left accumulate_partition ([], false) (g#list_partitions ()) in - match esp_partitions with - | [] -> I_BIOS - | partitions -> I_UEFI partitions + match esp_partitions, bios_boot with + | _ :: _, false -> I_UEFI esp_partitions + | _ -> I_BIOS (* If some inspection fields are "unknown", then that indicates a * failure in inspection, and we shouldn't continue. For an example

On 12/20/22 11:51, Laszlo Ersek wrote: > On 12/19/22 23:16, Andrey Drobyshev wrote: > >> Now I'm a bit confused myself as a result of our discussion: should we >> really give priority to BIOS Boot partition? Because I tried to >> replay the opposite scenario -- i.e. to convert a disk previously used >> by BIOS machine to be used by UEFI machine, but leaving BIOS Boot >> partition with its parttype unchanged. And it failed to no surprise >> with this patch applied. Do you have a clearer understanding of what >> to prioritize and why? > > In fact I do have a preference here. I consider this partition table > busted (inconsistent). If not necessarily for booting then for virt-v2v > purposes certainly. As you've showed yourself, if we apply this patch, > then we make things worse for the "inverse busting" of the partition > table. > > So I think we should leave this as-is, as far as *automatic* detection > is concerned. We could introduce a command line option for forcing the > source firmware type, but that's something I'd like Rich to comment upon > as well (especially because it is a user interface change), and Rich is > on PTO. > > I understand this patch is motivated by a preexistent customer issue > report; I figure the answer we can give, thus far, is "your source > system is inconsistent enough that we can't safely/robustly judge what > to do with it in an automated fashion". I see your point. However, here's my final argument on robustness. Take a look at [convert/linux_bootloaders.ml]: > let detect_bootloader (g : G.guestfs) inspect = > (* Where to start searching for bootloaders. *) > let mp = > match inspect.i_firmware with > | I_BIOS -> "/boot" > | I_UEFI _ -> "/boot/efi/EFI" in > > (* Find all paths below the mountpoint, then filter them to find > * the grub config file. > *) > let paths = > try List.map ((^) mp) (Array.to_list (g#find mp)) > with G.Error msg -> > error (f_"could not find bootloader mount point (%s): %s") mp msg in > > (* We can determine if the bootloader config file is grub 1 or > * grub 2 just by looking at the filename. > *) > let bootloader_type_of_filename path = > match last_part_of path '/' with > | Some "grub.cfg" -> Some Grub2 > | Some ("grub.conf" | "menu.lst") -> Some Grub1 > | Some _ > | None -> None > in > .... Here we're looping over files in the alleged boot/ESP partition trying to find a file which resembles a bootloader config. This code is of course Linux-specific and runs after the inspection phase. But what if we managed to apply the same kind of logic earlier on during inspection, namely: UEFI disk AND EFI system partition AND this partition contains grub.cfg -> I_UEFI otherwise -> I_BIOS This would probably require some rewriting/reordering of the calls as we wouldn't want to duplicate this logic both during inspection and later during conversion, but what do you think of the idea in general?

Now, we can surely "bust" the partition scheme as well here by having an "abandoned" ESP with grub.cfg remaining on it, but nevertheless this additional check would make it more robust overall, as I see it.

> >>>>> (5) I don't really like the runtime logic duplication that seems to >>>>> be introduced by the patch. By iterating over the list of >>>>> partitions twice, we make twice as many libguestfs calls. I think >>>>> we should iterate only once over the list of partitions. >>>> >>>> Agreed, iterating just once is obviously better. I'll try to come >>>> up with the code which is less obscure. For instance, List.map with >>>> a custom function, taking partition as its argument and returning a >>>> tuple (Some part * bool). This way we'd get a list of tuples which >>>> you referred to in your other comment. >>> >>> Can you try out the patches I proposed below? >>> >>> I agree that List.map + List.fold would work, but then (a) you don't >>> need a separate mapping step just so you can fold the mapped list: >>> you can push the mapping down into the folder callback; (b) I >>> honestly doubt in this case that the purely functional implementation >>> would be easier to read. Anyway, if you prefer that, I'm certainly OK >>> with it. >> >> Well, to my view the necessity to use ref's make it a bit clumsy, sort >> of like using global variables where they could've been avoided. Would >> you consider this (functional) use of List.exists, List.filter_map? > > I do think that (regarless of real life utility) this is a very > interesting discussion. So you've now prompted me to rewrite the patches > without refs, just for my own interest. > > I think we can do it more compactly than List.exists (for the bios boot > part) *plus* List.filter_map (for the ESP part list). > > First patch (refactoring, purely functional style, using > "Array.fold_left"): > >> diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml >> index 056d0bca62e6..9686f9fff8d8 100644 >> --- a/convert/inspect_source.ml >> +++ b/convert/inspect_source.ml >> @@ -225,26 +225,29 @@ and list_applications g root = function >> * Otherwise, [BIOS] is returned. >> *) >> and get_firmware_bootable_device g = >> - let rec uefi_ESP_guid = "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" >> - and is_uefi_ESP dev part = >> - let partnum = g#part_to_partnum part in >> - g#part_get_gpt_type dev partnum = uefi_ESP_guid >> - and parttype_is_gpt dev = >> + let parttype_is_gpt dev = >> try g#part_get_parttype dev = "gpt" >> with G.Error msg as exn -> >> (* If it's _not_ "unrecognised disk label" then re-raise it. *) >> if g#last_errno () <> G.Errno.errno_EINVAL then raise exn; >> debug "%s (ignored)" msg; >> false >> - and is_uefi_bootable_part part = >> + in >> + let accumulate_partition esp_parts part = >> let dev = g#part_to_dev part in >> - parttype_is_gpt dev && is_uefi_ESP dev part >> + if parttype_is_gpt dev then >> + let partnum = g#part_to_partnum part in >> + let part_type_guid = g#part_get_gpt_type dev partnum in >> + match part_type_guid with >> + | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> part :: esp_parts >> + | _ -> esp_parts >> + else esp_parts >> in >> >> - let partitions = Array.to_list (g#list_partitions ()) in >> - let partitions = List.filter is_uefi_bootable_part partitions in >> + let esp_partitions = >> + Array.fold_left accumulate_partition [] (g#list_partitions ()) in >> >> - match partitions with >> + match esp_partitions with >> | [] -> I_BIOS >> | partitions -> I_UEFI partitions >> > > Second patch (adding bios_boot): > >> diff --git a/convert/inspect_source.ml b/convert/inspect_source.ml >> index 9686f9fff8d8..8e8f5c45f1eb 100644 >> --- a/convert/inspect_source.ml >> +++ b/convert/inspect_source.ml >> @@ -233,23 +233,24 @@ and get_firmware_bootable_device g = >> debug "%s (ignored)" msg; >> false >> in >> - let accumulate_partition esp_parts part = >> + let accumulate_partition (esp_parts, bboot) part = >> let dev = g#part_to_dev part in >> if parttype_is_gpt dev then >> let partnum = g#part_to_partnum part in >> let part_type_guid = g#part_get_gpt_type dev partnum in >> match part_type_guid with >> - | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> part :: esp_parts >> - | _ -> esp_parts >> - else esp_parts >> + | "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" -> part :: esp_parts, bboot >> + | "21686148-6449-6E6F-744E-656564454649" -> esp_parts, true >> + | _ -> esp_parts, bboot >> + else esp_parts, bboot >> in >> >> - let esp_partitions = >> - Array.fold_left accumulate_partition [] (g#list_partitions ()) in >> + let esp_partitions, bios_boot = >> + Array.fold_left accumulate_partition ([], false) (g#list_partitions ()) in >> >> - match esp_partitions with >> - | [] -> I_BIOS >> - | partitions -> I_UEFI partitions >> + match esp_partitions, bios_boot with >> + | _ :: _, false -> I_UEFI esp_partitions >> + | _ -> I_BIOS >> >> (* If some inspection fields are "unknown", then that indicates a >> * failure in inspection, and we shouldn't continue. For an example > > You be the judge whether this is easier to read then the reference cells > version :) Yep, your code is once again more minimalistic and easier to comprehend. Especially cool that there're no intermediate list as in my version, we only have two: the original partition list and the one which is being constructed on the fly.