Re: [Libguestfs] [v2v PATCH] windows_virtio: favor "fwcfg" over "qemufwcfg"

On 12/15/22 12:15, Laszlo Ersek wrote: > Virtio-win may provide the "qemufwcfg" stub driver and/or the "fwcfg" > actual driver. If both are provided, we must not install both, as they > conflict with each other. Pick "fwcfg" in this case. > > Because the drivers can originate from two sources (libosinfo vs. > virtio-win), *and* because "copy_from_virtio_win" is reused for the QEMU > guest agent (i.e., not just for the drivers), do not sink the above > filtering into "copy_drivers" (or even more deeply). Instead, let copying > complete, and then clean up "driverdir" -- remove "qemufwcfg" if "fwcfg" > is present. (We'd have to consult the full list of drivers anyway, to see > if "fwcfg" indicates we should exclude "qemufwcfg".) > > A note on annotating the "install_drivers" parameter list (OCaml obscurity > level one million): > >> -let rec install_drivers ((g, _) as reg) inspect = >> +let rec install_drivers ((g, _) as reg : Registry.t) inspect = > > Turns out that in this module, OCaml doesn't really have an idea that all > the "g#method" calls are made for an object of type "Guestfs.guestfs". > Instead, the compiler infers an interface from the methods we call, and > then tries to shoehorn that "collected interface" into "Guestfs.guestfs" > when it reaches: > >> reg_import reg (regedits @ common_regedits)

> This used to work fine until now; however, once we call > >> g#glob_expand (driverdir // "qemufwcfg.*") > > in this patch, the "reg_import" call fails like this: > >> File "windows_virtio.ml", line 152, characters 13-16: >> 152 | reg_import reg (regedits @ common_regedits) >> ^^^ >> Error: This expression has type >> (< [snip] >> glob_expand : string -> 'weak1 array; >> [snip] > >> as 'a) * >> 'weak2 >> but an expression was expected of type >> Registry.t = Guestfs.guestfs * Registry.node >> Type >> < [snip] >> glob_expand : string -> 'weak1 array; >> [snip] > >> as 'a >> is not compatible with type >> Guestfs.guestfs = >> < [snip] >> glob_expand : ?directoryslash:bool -> string -> string array; >> [snip] > >> Types for method glob_expand are incompatible > > The problem is that in our "g#glob_expand" call, we silently omit the > optional parameter "directoryslash", so at that point the compiler > "infers" a parameter list > >> glob_expand : string -> 'weak1 array; > > for the "interface" we require. And then that blows up because the actual > object provides only > >> glob_expand : ?directoryslash:bool -> string -> string array; > > The solution is to enlighten the compiler in "install_drivers" about the > actual type of "g", so that it need not infer or "collect" an interface > when we call "g#glob_expand" with "directoryslash" elided. . > Now, "install_drivers" is called (as a callback!) ultimately from > "Registry.with_hive_write", and so its "reg" parameter has type > "Registry.t": > >> type t = Guestfs.guestfs * node > > (This is in fact reported by the compiler too, in the above-quoted error > message. Except said error message is like ten pages long -- see those > [snip] markers? --, so you will only ever find the relevant bit in the > error report if you already know what to look for. Helpful, that!) > > Therefore, annotating the "reg" parameter like this: > >> -let rec install_drivers ((g, _) as reg) inspect = >> +let rec install_drivers ((g, _) as reg : Registry.t) inspect = > > forces "g" to have type "Guestfs.guestfs". > > This is of course super obscure. The hint was that both > "linux_bootloaders.ml" and "convert_linux.ml" already used "g#glob_expand" > without the optional parameter -- and the important difference was that > these files had been type-annotated previously. > > In *retrospect* -- that is, rather uselessly... --, the language > documentation does highlight this > <https://v2.ocaml.org/manual/lablexamples.html#s:label-inference>;: > >> We will not try here to explain in detail how type inference works. One >> must just understand that there is not enough information in the above >> program to deduce the correct type of g or bump. That is, there is no >> way to know whether an argument is optional or not, or which is the >> correct order, by looking only at how a function is applied. The >> strategy used by the compiler is to assume that there are no optional >> arguments, and that applications are done in the right order. >> >> [...] >> >> In practice, such problems appear mostly when using objects whose >> methods have optional arguments, so that writing the type of object >> arguments is often a good idea.

> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2151752 > Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; > --- > convert/windows_virtio.ml | 19 ++++++++++++++++++- > 1 file changed, 18 insertions(+), 1 deletion(-) > > diff --git a/convert/windows_virtio.ml b/convert/windows_virtio.ml > index 3156694d114b..d9fda13f999b 100644 > --- a/convert/windows_virtio.ml > +++ b/convert/windows_virtio.ml > @@ -44,7 +44,7 @@ let viostor_modern_pciid = "VEN_1AF4&DEV_1042&SUBSYS_11001AF4&REV_01" > let vioscsi_legacy_pciid = "VEN_1AF4&DEV_1004&SUBSYS_00081AF4&REV_00" > let vioscsi_modern_pciid = "VEN_1AF4&DEV_1048&SUBSYS_11001AF4&REV_01" > > -let rec install_drivers ((g, _) as reg) inspect = > +let rec install_drivers ((g, _) as reg : Registry.t) inspect = > (* Copy the virtio drivers to the guest. *) > let driverdir = sprintf "%s/Drivers/VirtIO" inspect.i_windows_systemroot in > g#mkdir_p driverdir; > @@ -103,6 +103,23 @@ let rec install_drivers ((g, _) as reg) inspect = > else > Virtio_net in > > + (* The "fwcfg" driver binds the fw_cfg device for real, and provides three > + * files -- ".cat", ".inf", ".sys". (Possibly ".pdb" too.) > + * > + * The "qemufwcfg" driver is only a stub driver; it placates Device Manager > + * (hides the "unknown device" question mark) but does not actually drive > + * the fw_cfg device. It provides two files only -- ".cat", ".inf". > + * > + * These drivers conflict with each other (RHBZ#2151752). If we've copied > + * both (either from libosinfo of virtio-win), let "fwcfg" take priority: > + * remove "qemufwcfg". > + *) > + if g#exists (driverdir // "fwcfg.inf") && > + g#exists (driverdir // "qemufwcfg.inf") then ( > + debug "windows: skipping qemufwcfg stub driver in favor of fwcfg driver"; > + Array.iter g#rm (g#glob_expand (driverdir // "qemufwcfg.*")) > + ); > + > (* Did we install the miscellaneous drivers? *) > let virtio_rng_supported = g#exists (driverdir // "viorng.inf") in > let virtio_ballon_supported = g#exists (driverdir // "balloon.inf") in Commit 0596edf29aa4. (In RHBZ#2151752, QE have tested this and from their logs, I've determined the patch works as intended. For fixing the entire problem, virtio-win changes are needed too (i.e., in the qemufwcfg / fwcfg drivers themselves), and those are tracked in a separate (pre-requisite) RHBZ. That dependency doesn't change the fact that both drivers exclude each other.)