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.drobys...@virtuozzo.com> >> Originally-by: Denis Plotnikov <dplotni...@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 () = > _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
