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

On 12/20/22 21:57, Andrey Drobyshev wrote: > 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? Hmm, not sure. It seems to reverse the information flow / data dependencies a bit. Currently we first try to determine the firmware, and then (keying off of that) figure out the boot loader. Because, we need the boot loader (config) anyways, for further (firmware-unrelated, IIUC) purposes. Hooking the boot loader detection into firmware determination is a bit cart-before-the-horse, to me. (These doubts we're having here are not random BTW, they just reflect how fraught and brittle the firmware/bootloader space genuinely is.) > > 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. I see it quite differently; just yesterday I had to edit a RHEL8 system's kernel cmdline. I went to /etc/default/grub as usual, and whatever I did there didn't take effect. Turns out in RHEL8 we defer to "blscfg" unless disabled, so we need to use "grubby" for updating all (loosely) related config files together. This is absolutely horrible, because now you need to *execute* code in order to get in config changes, rather than just edit files. (And of course there was no comment in /etc/default/grub that it was a generated file!) My example may be a bit tangential; to me it does show that firmware comes first (reflecting in what order a computer actually boots), and then the boot loader is its own whole chaos.

So, I guess you've just showed me a greater bad :) Also, if I remember correctly, in virt-v2v we've been quite open to "policy changes" and not especially worried about regressions existing temporarily. Can you submit the two patches then? (The refactoring and the BIOS Boot partition recognition -- ref-cells style, or functional style, whichever you prefer.) I assume we can be listed on those again as collaborators, if you think that's appropriate.

I believe, after this discussion, we can give priority to a BIOS boot partition, whenever we see one. > >> >>>>>> (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. Thanks! Laszlo