Re: [Libguestfs] [PATCH v2] parse_libvirt_xml: look for manual firmware in "/domain/os/loader"

On 12/15/22 21:25, Richard W.M. Jones wrote: > On Thu, Dec 15, 2022 at 08:11:15PM +0200, 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(a)virtuozzo.com&gt; >> Originally-by: Denis Plotnikov <dplotnikov(a)virtuozzo.com&gt; >> --- >> input/parse_libvirt_xml.ml | 21 +++++++++++++++++++-- >> 1 file changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/input/parse_libvirt_xml.ml b/input/parse_libvirt_xml.ml >> index 1e98ce1a..0fecde33 100644 >> --- a/input/parse_libvirt_xml.ml >> +++ b/input/parse_libvirt_xml.ml >> @@ -27,6 +27,9 @@ open Xpath_helpers >> open Types >> open Utils >> >> +(* Detect that "/domain/os/loader" node contains path to UEFI firmware. *) >> +let loader_contains_ovmf_re = PCRE.compile "/OVMF_CODE" >> + >> type disk = { >> d_format : string option; (* Disk format from XML if known. *) >> d_type : disk_type; (* Disk type and extra information. *) >> @@ -446,12 +449,26 @@ 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 "OVMF_CODE" in "/domain/os/loader" >> + * node (manual 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" in >> + match loader with >> + | None -> UnknownFirmware >> + | _ -> ( >> + let loader = Option.default "" loader in >> + if PCRE.matches loader_contains_ovmf_re loader then UEFI >> + else UnknownFirmware >> + ) >> + ) in >> >> (* Check for hostdev devices. (RHBZ#1472719) *) >> let () = > > It seems fine to me, just want to see what Laszlo thinks first. > > Rich. > BTW speaking of the regexps, I think we might as well not use them at all (either PCRE or any other kind). Libvirt wouldn't allow any modifications (letter case, spaces etc.) for the attribute value, meaning that if the attribute is present, it MUST be exactly "pflash". So in this case I'd go with straighforward string pattern matching.