Hi Andrey,

On 12/19/22 19:59, 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
> 
> Co-authored-by: Laszlo Ersek <ler...@redhat.com>
> Signed-off-by: Andrey Drobyshev <andrey.drobys...@virtuozzo.com>
> Originally-by: Denis Plotnikov <dplotni...@virtuozzo.com>
> ---
>  input/parse_libvirt_xml.ml | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/input/parse_libvirt_xml.ml b/input/parse_libvirt_xml.ml
> index 56ce1c22..ab72c0ce 100644
> --- a/input/parse_libvirt_xml.ml
> +++ b/input/parse_libvirt_xml.ml
> @@ -446,12 +446,23 @@ 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
> +    | Some _ -> UnknownFirmware
> +    | None -> (
> +        match xpath_string "/domain/os/loader/@type" with
> +        | Some "pflash" -> UEFI
> +        | _ -> UnknownFirmware
> +      ) in
>  
>    (* Fallback to BIOS if we haven't found explicitly specified firmware.
>     * This is VZ-specific since we're either using "/domain/os/loader" node

I'm OK with this patch.

The only reason I can't give R-b for it is that you noted me as
co-author on the patch, and I can't review my own (or co-authored)
patches. But that's not a problem; I actually tried to apply (and then
push) this patch, without any R-b's (with Rich being on PTO).

However, the patch does not apply to master @ 1c8ff404582f. The conflict
is in the trailing context of the patch: the trailing comment in v4
introduces a VZ-specific code section, whereas on the master branch, we
have:

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

Can you please rebase to the master branch and repost?

(Quickly checking versions 1 through 3 of the patch, those were all
based on the master branch; I think it is only in v4 where you have
based the patch on a downstream-only branch. That's totally fine of
course, but please send the upstream version to the list.)

Thanks!
Laszlo
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs

Reply via email to