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

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