Re: [Libguestfs] [PATCH 1/2] v2v: fill the list of the EFI system partitions

On Fri, Jun 10, 2016 at 05:07:02PM +0300, Pavel Butsykin wrote: > Store the list of EFI system partitions on the inspect object in order to be > able to tune their contents later in the process. > > Signed-off-by: Pavel Butsykin <pbutsykin(a)virtuozzo.com&gt; > --- > v2v/convert_linux.ml | 2 +- > v2v/inspect_source.ml | 29 +++++++++++++++++++++-------- > v2v/types.ml | 4 ++-- > v2v/types.mli | 3 ++- > v2v/v2v.ml | 2 +- > v2v/v2v_unit_tests.ml | 2 +- > 6 files changed, 28 insertions(+), 14 deletions(-) > > diff --git a/v2v/convert_linux.ml b/v2v/convert_linux.ml > index 08b27d6..2de5e42 100644 > --- a/v2v/convert_linux.ml > +++ b/v2v/convert_linux.ml > @@ -97,7 +97,7 @@ let rec convert ~keep_serial_console (g : G.guestfs) inspect source rcaps = > "/boot/grub/grub.conf", `Grub1; > ] in > let locations = > - if inspect.i_uefi then > + if inspect.i_uefi <> None then > ("/boot/efi/EFI/redhat/grub.cfg", `Grub2) :: locations > else > locations in > diff --git a/v2v/inspect_source.ml b/v2v/inspect_source.ml > index 65dcb88..7a7674a 100644 > --- a/v2v/inspect_source.ml > +++ b/v2v/inspect_source.ml > @@ -68,7 +68,7 @@ let rec inspect_source root_choice g = > (* See if this guest could use UEFI to boot. It should use GPT and > * it should have an EFI System Partition (ESP). > *) > - let uefi = has_uefi_bootable_device g in > + let uefi = get_uefi_bootable_device g in > > let inspect = { > i_root = root; > @@ -153,10 +153,12 @@ and reject_if_not_installed_image g root = > if fmt <> "installed" then > error (f_"libguestfs thinks this is not an installed operating system (it might be, for example, an installer disk or live CD). If this is wrong, it is probably a bug in libguestfs. root=%s fmt=%s") root fmt > > -and has_uefi_bootable_device g = > +and get_uefi_bootable_device g = > let rec uefi_ESP_guid = "C12A7328-F81F-11D2-BA4B-00A0C93EC93B" > and is_uefi_ESP dev { G.part_num = partnum } = > g#part_get_gpt_type dev (Int32.to_int partnum) = uefi_ESP_guid > + and part_dev_name dev { G.part_num = partnum } = > + sprintf "%s%d" dev (Int32.to_int partnum) > and parttype_is_gpt dev = > try g#part_get_parttype dev = "gpt" > with G.Error msg as exn -> > @@ -164,14 +166,25 @@ and has_uefi_bootable_device g = > if g#last_errno () <> G.Errno.errno_EINVAL then raise exn; > debug "%s (ignored)" msg; > false > - and is_uefi_bootable_device dev = > - parttype_is_gpt dev && ( > - let partitions = Array.to_list (g#part_list dev) in > - List.exists (is_uefi_ESP dev) partitions > - ) > + and is_uefi_bootable_part dev part = > + parttype_is_gpt dev && is_uefi_ESP dev part > in > let devices = Array.to_list (g#list_devices ()) in > - List.exists is_uefi_bootable_device devices > + let uefi_list = ref [] in > + > + List.iter ( > + fun dev -> > + Array.iter ( > + fun part -> > + if (is_uefi_bootable_part dev part) then ( > + uefi_list := (part_dev_name dev part) :: !uefi_list > + ) > + ) (g#part_list dev) > + ) devices; > + > + match !uefi_list with > + | [] -> None > + | list -> Some list > > (* If some inspection fields are "unknown", then that indicates a > * failure in inspection, and we shouldn't continue. For an example > diff --git a/v2v/types.ml b/v2v/types.ml > index 08e1631..7f8a9b3 100644 > --- a/v2v/types.ml > +++ b/v2v/types.ml > @@ -315,7 +315,7 @@ type inspect = { > i_mountpoints : (string * string) list; > i_apps : Guestfs.application2 list; > i_apps_map : Guestfs.application2 list StringMap.t; > - i_uefi : bool; > + i_uefi : string list option; I think what you really want is for the type to be "string list" (not option), and for empty list to mean BIOS. (Assuming that UEFI cannot have zero system partitions -- as far as I understand UEFI, that's true).

Alternately you could write something like: type i_firmware = | BIOS | UEFI of string list ... i_firmware : i_firmware; which is quite nice, I think, because it's explicit about the meaning. > } > > let string_of_inspect inspect = > @@ -341,7 +341,7 @@ i_uefi = %b > inspect.i_package_management > inspect.i_product_name > inspect.i_product_variant > - inspect.i_uefi > + (inspect.i_uefi <> None) We should print the list here, so: "...i_uefi = [%s]" ... (String.concat ", " inspect.i_uefi) or if using the i_firmware type above: "...i_firmware = %s" ... (match inspect.i_firmware with | BIOS -> "BIOS" | UEFI devices -> sprintf "UEFI [%s]" (String.concat ", " devices) ) > type mpstat = { > mp_dev : string; > diff --git a/v2v/types.mli b/v2v/types.mli > index dacc991..a0a8399 100644 > --- a/v2v/types.mli > +++ b/v2v/types.mli > @@ -221,7 +221,8 @@ type inspect = { > (** This is a map from the app name to the application object. > Since RPM allows multiple packages with the same name to be > installed, the value is a list. *) > - i_uefi : bool; (** True if the guest could boot with UEFI. *) > + i_uefi : string list option; > + (** Some list if the guest could boot with UEFI. *) This comment needs to document what are the strings in this list. > } > (** Inspection information. *) > > diff --git a/v2v/v2v.ml b/v2v/v2v.ml > index fe81df5..2527633 100644 > --- a/v2v/v2v.ml > +++ b/v2v/v2v.ml > @@ -555,7 +555,7 @@ and get_target_firmware inspect guestcaps source output = > | BIOS -> TargetBIOS > | UEFI -> TargetUEFI > | UnknownFirmware -> > - if inspect.i_uefi then TargetUEFI else TargetBIOS in > + if inspect.i_uefi = None then TargetBIOS else TargetUEFI in > let supported_firmware = output#supported_firmware in > if not (List.mem target_firmware supported_firmware) then > error (f_"this guest cannot run on the target, because the target does not support %s firmware (supported firmware on target: %s)") > diff --git a/v2v/v2v_unit_tests.ml b/v2v/v2v_unit_tests.ml > index 95d8430..874cd48 100644 > --- a/v2v/v2v_unit_tests.ml > +++ b/v2v/v2v_unit_tests.ml > @@ -30,7 +30,7 @@ let inspect_defaults = { > i_major_version = 0; i_minor_version = 0; > i_root = ""; i_package_format = ""; i_package_management = ""; > i_product_name = ""; i_product_variant = ""; i_mountpoints = []; > - i_apps = []; i_apps_map = StringMap.empty; i_uefi = false > + i_apps = []; i_apps_map = StringMap.empty; i_uefi = None > } Rich.