Libguestfs - Jan 2023 - [PATCH] inspect: check presence of BIOS boot partition to handle BIOS+GPT setup

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 at virtuozzo.com>
> ---
>  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_specification/
 
https://www.freedesktop.org/software/systemd/man/systemd-gpt-auto-generator.html

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_2%E2%80%9333)>
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
Laszlo

On 12/16/22 12: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 at
virtuozzo.com>
>> ---
>>  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_specification/
>  
https://www.freedesktop.org/software/systemd/man/systemd-gpt-auto-generator.html
> 
> 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_2%E2%80%9333)>
> 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
BTW I think this can be done purely functionally too (without reference
cells), with Array.fold_left. The folder would map each partition to
tuple, the first component being None or Some part, dependent on whether
it's and ESP or not; the second component would be true or false,
dependent on whether "part" is a BIOS boot partition or not. Then the
first component would be accumulated such that None is thrown away and
Some part prepends part to the list (the accumulators first component is
a list), and the second component would just be OR'd into the
accumulator's second component (which would be a bool). Quite doubtful
if this would look better than the above with reference cells though.

Laszlo

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 at
virtuozzo.com>
>> ---
>>  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

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_specification/
>  
https://www.freedesktop.org/software/systemd/man/systemd-gpt-auto-generator.html
> 
> 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_2%E2%80%9333)>
> 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
>

Hi Andrey,

Just looking through the patches posted vs what went upstream:

https://github.com/libguestfs/virt-v2v/commit/73614313114a7a3f3dfdd857d5c0307b34d0599c
https://github.com/libguestfs/virt-v2v/commit/d04b9a05b2a5350b55e1558a91e3477c35c16976
https://github.com/libguestfs/virt-v2v/commit/d04b9a05b2a5350b55e1558a91e3477c35c16976

I think we're all good here now?  If there are any more patches that
you're waiting on then please let me know.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org