Laszlo Ersek
2022-Dec-22 07:20 UTC
[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 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") inCommit 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
Richard W.M. Jones
2023-Jan-05 10:31 UTC
[Libguestfs] [v2v PATCH] windows_virtio: favor "fwcfg" over "qemufwcfg"
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 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.)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