[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@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@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
On Thu, Dec 22, 2022 at 08:20:56AM +0100, Laszlo Ersek wrote:
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 is a feature ("duck typing" https://en.wikipedia.org/wiki/Duck_typing). It allows you to have two classes with the same subset of methods, and work out what class you mean later. eg you have two unrelated (not inherited) classes which both have a "print" method: class file = object method print msg = ... print msg to a file ... end class memory = object method print msg = ... print msg to a memory buffer ... end Then you can define a function that works on either type: let debug st msg = st#print ("debug: " ^ msg) (Compare Java/Smalltalk where you'd have to do this using inheritance.)
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.
... however indeed it doesn't work well with complicated type inference involving labels as you mention (especially if you don't care about / use duck typing), so guiding inference with a well-placed type annotation helps.
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2151752 Signed-off-by: Laszlo Ersek <lersek@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.)
It looks good. Thanks for fixing this while I was away. Since the patch is upstream, shouldn't the bug (2151752) be moved to POST? Or is there more to do? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
On 1/5/23 11:31, Richard W.M. Jones wrote:
This is a feature ("duck typing" https://en.wikipedia.org/wiki/Duck_typing).
Ah, good point. I guess I've heard of it before, it's just always seemed so heretical to me (coming from an "inheritance" background rather than a "pattern matching" background) that I've never internalized it.
Since the patch is upstream, shouldn't the bug (2151752) be moved to POST? Or is there more to do?
Haha, no, you are right; that's a timing mistake on my part. I merged the patch on 22 Dec 2022 (<https://bugzilla.redhat.com/show_bug.cgi?id=2151752#c21>), but back then I didn't know about a rebase BZ (or even a *plan* for rebasing I think?), so I couldn't set up a dependency, and therefore I refrained from moving the BZ to post. Later (on 31 Dec 2022), when I found out about the rebase BZ, I set up the dependencies from the rebase BZ's perspective <https://bugzilla.redhat.com/show_bug.cgi?id=2135762#c2>, but then I missed moving the original BZ to POST. :) Sorry about that, doing it now. Laszlo
participants (2)
-
Laszlo Ersek -
Richard W.M. Jones