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

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