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

[Andrey Drobyshev]

On 12/19/22 12:02, Laszlo Ersek wrote:
> 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.

Alright, makes sense.

> 
>> +      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:

Apparently, my OCaml-fu is amateur (:
Thanks for the clarification.

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

Your version is definitely simpler and more readable, I'll include it in
v4 with you as a co-author.

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