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

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".

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 ....

>>>> (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 :)

Laszlo