Libguestfs - Dec 2022 - [V2V PATCH v3] parse_libvirt_xml: look for manual firmware in "/domain/os/loader/@type"

According to [1], there're different ways to specify which firmware is
to be used by a libvirt-driven VM.  Namely, there's an automatic
firmware selection, e.g.:

    ...
    <os firmware='(bios|efi)'>
    ...

and a manual one, e.g.:

    ...
    <os>
        <loader readonly='yes'
type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader>
        ...
    </os>
    ...

with the latter being a way to specify UEFI firmware.  So let's add this
search path as well when parsing source VM's libvirt xml.

[1] https://libvirt.org/formatdomain.html#bios-bootloader

Signed-off-by: Andrey Drobyshev <andrey.drobyshev at virtuozzo.com>
Originally-by: Denis Plotnikov <dplotnikov at virtuozzo.com>
---
 input/parse_libvirt_xml.ml | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/input/parse_libvirt_xml.ml b/input/parse_libvirt_xml.ml
index 1e98ce1a..449e3466 100644
--- a/input/parse_libvirt_xml.ml
+++ b/input/parse_libvirt_xml.ml
@@ -446,12 +446,28 @@ let parse_libvirt_xml ?conn xml      done;
     List.rev !nics in
 
-  (* Firmware. *)
+  (* Firmware.
+   * If "/domain/os" node doesn't contain "firmware"
attribute (automatic
+   * firmware), we look for the presence of "pflash" in
+   * "/domain/os/loader/@type" attribute (manual firmware), with the
latter
+   * indicating the UEFI firmware.
+   * See https://libvirt.org/formatdomain.html#bios-bootloader
+   *)
   let firmware      match xpath_string "/domain/os/@firmware" with
     | Some "bios" -> BIOS
     | Some "efi" -> UEFI
-    | None | Some _ -> UnknownFirmware in
+    | None | Some _ -> (
+      let loader = xpath_string "/domain/os/loader/@type" in
+        match loader with
+        | None -> UnknownFirmware
+        | _ -> (
+          let loader = Option.default "" loader in
+          match loader with
+          | "pflash" -> UEFI
+          | _ -> UnknownFirmware
+        )
+    ) in
 
   (* Check for hostdev devices. (RHBZ#1472719) *)
   let () -- 
2.31.1

On 12/16/22 20:01, Andrey Drobyshev wrote:
> According to [1], there're different ways to specify which firmware is
> to be used by a libvirt-driven VM.  Namely, there's an automatic
> firmware selection, e.g.:
>
>     ...
>     <os firmware='(bios|efi)'>
>     ...
>
> and a manual one, e.g.:
>
>     ...
>     <os>
>         <loader readonly='yes'
type='pflash'>/usr/share/OVMF/OVMF_CODE.fd</loader>
>         ...
>     </os>
>     ...
>
> with the latter being a way to specify UEFI firmware.  So let's add
this
> search path as well when parsing source VM's libvirt xml.
>
> [1] https://libvirt.org/formatdomain.html#bios-bootloader
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev at virtuozzo.com>
> Originally-by: Denis Plotnikov <dplotnikov at virtuozzo.com>
> ---
>  input/parse_libvirt_xml.ml | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/input/parse_libvirt_xml.ml b/input/parse_libvirt_xml.ml
> index 1e98ce1a..449e3466 100644
> --- a/input/parse_libvirt_xml.ml
> +++ b/input/parse_libvirt_xml.ml
> @@ -446,12 +446,28 @@ let parse_libvirt_xml ?conn xml >      done;
>      List.rev !nics in
>
> -  (* Firmware. *)
> +  (* Firmware.
> +   * If "/domain/os" node doesn't contain
"firmware" attribute (automatic
> +   * firmware), we look for the presence of "pflash" in
> +   * "/domain/os/loader/@type" attribute (manual firmware), with
the latter
> +   * indicating the UEFI firmware.
> +   * See https://libvirt.org/formatdomain.html#bios-bootloader
> +   *)
>    let firmware >      match xpath_string
"/domain/os/@firmware" with
>      | Some "bios" -> BIOS
>      | Some "efi" -> UEFI
> -    | None | Some _ -> UnknownFirmware in
> +    | None | Some _ -> (
I'd prefer if we kept assigning UnknownFirmware to the "Some _"
pattern
here. That would indicate <os firmware='something_unknown'>, so I
think
we should give up further domain XML-based checks in that case.
> +      let loader = xpath_string "/domain/os/loader/@type" in
> +        match loader with
> +        | None -> UnknownFirmware
> +        | _ -> (
> +          let loader = Option.default "" loader in
> +          match loader with
> +          | "pflash" -> UEFI
> +          | _ -> UnknownFirmware
> +        )
> +    ) in
This looks needlessly complicated. "Option.default" is just a
shorthand
for pattern matching [common/mlstdutils/std_utils.ml]:
> module Option = struct
>     [...]
>     let default def = function
>       | None -> def
>       | Some x -> x
> end
(where "function" is again syntactic sugar; it means:
> module Option = struct
>     [...]
>     let default def opt = match opt with
>       | None -> def
>       | Some x -> x
> end
)

so once you're matching patterns already, "Option.default" is
superfluous. How about this:

diff --git a/input/parse_libvirt_xml.ml b/input/parse_libvirt_xml.ml
index 1e98ce1a8694..a98cdfb76109 100644
--- a/input/parse_libvirt_xml.ml
+++ b/input/parse_libvirt_xml.ml
@@ -451,7 +451,12 @@ let parse_libvirt_xml ?conn xml      match xpath_string
"/domain/os/@firmware" with
     | Some "bios" -> BIOS
     | Some "efi" -> UEFI
-    | None | Some _ -> UnknownFirmware in
+    | Some _ -> UnknownFirmware
+    | None -> (
+        match xpath_string "/domain/os/loader/@type" with
+        | Some "pflash" -> UEFI
+        | _ -> UnknownFirmware
+      ) in

   (* Check for hostdev devices. (RHBZ#1472719) *)
   let () 
Laszlo
>
>    (* Check for hostdev devices. (RHBZ#1472719) *)
>    let () =