Libguestfs - Dec 2022 - [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 at redhat.com>
---
 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

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 at redhat.com>
> ---
>  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.)

Laszlo