Andrey Drobyshev
2022-Dec-16 19:01 UTC
[Libguestfs] [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
Laszlo Ersek
2022-Dec-19 10:02 UTC
[Libguestfs] [V2V PATCH v3] parse_libvirt_xml: look for manual firmware in "/domain/os/loader/@type"
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 > + ) > + ) inThis 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 () =