Libguestfs - Jun 2016 - Re: [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@virtuozzo.com>
> ---
>  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.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW