Laszlo Ersek
2021-Nov-01 17:06 UTC
[Libguestfs] [virt-v2v RFC wave 2 03/10] convert/windows_virtio: restrict the warning with virtio-win.iso absent
Consider the following two situations:
(a) For the Windows version being converted, the virtio-win.iso image does
not offer any guest drivers (IOW, the Windows version as a whole is
unsupported by virtio-win.iso).
(b) For the Windows version being converted, the virtio-win.iso image
offers *some* drivers (but not for all possible paravirtualized
devices).
There are three relevant device categories: block, network, display.
If the conversion specifically requests paravirt for a given device
category, and that particular paravirt driver is unavailable -- be that
due to reason (a) or (b) --, the conversion fails, with an error message.
This is expected.
If the conversion does not express any preference for paravirt in a given
device category, and that particular paravirt driver is unavailable -- be
that due to reason (a) or (b) --, a warning is emitted, and the conversion
continues, using an emulated device in that category. This is expected as
well.
However, if the conversion explicitly requests an emulated device model
for a given device category -- that is, if the conversion *forbids*
paravirtualization for a given device category --, and that particular
paravirt driver is unavailable anyway, then the behavior between (a) and
(b) differs. Under (a), a warning is emitted (incorrectly!), while under
(b), no warning is emitted.
Under condition (a), only issue the warning if the conversion permits
paravirtualization in at least one device category.
Fixes: 9ed9d048f2b296cd0ebbd638ac3a2c5c8b47fded
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1961107
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
convert/windows_virtio.ml | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/convert/windows_virtio.ml b/convert/windows_virtio.ml
index 1851fe2f3292..11b34a03f876 100644
--- a/convert/windows_virtio.ml
+++ b/convert/windows_virtio.ml
@@ -59,12 +59,14 @@ let rec install_drivers ((g, _) as reg) inspect rcaps
inspect.i_major_version inspect.i_minor_version inspect.i_arch
inspect.i_product_variant virtio_win
- | { rcaps_block_bus = (Some IDE | None);
+ | { rcaps_block_bus = ((Some IDE | None) as block_type);
rcaps_net_bus = ((Some E1000 | Some RTL8139 | None) as net_type);
- rcaps_video = (Some Cirrus | None) } ->
- warning (f_"there are no virtio drivers available for this version
of Windows (%d.%d %s %s). virt-v2v looks for drivers in %s\n\nThe guest will be
configured to use slower emulated devices.")
- inspect.i_major_version inspect.i_minor_version inspect.i_arch
- inspect.i_product_variant virtio_win;
+ rcaps_video = ((Some Cirrus | None) as video_type) } ->
+ if block_type = None || net_type = None || video_type = None then
+ warning (f_"there are no virtio drivers available for this version
of Windows (%d.%d %s %s). virt-v2v looks for drivers in %s\n\nThe guest will be
configured to use slower emulated devices.")
+ inspect.i_major_version inspect.i_minor_version inspect.i_arch
+ inspect.i_product_variant virtio_win
+ else ();
let net_type match net_type with
| Some model -> model
--
2.19.1.3.g30247aa5d201
Richard W.M. Jones
2021-Nov-02 08:52 UTC
[Libguestfs] [virt-v2v RFC wave 2 03/10] convert/windows_virtio: restrict the warning with virtio-win.iso absent
On Mon, Nov 01, 2021 at 06:06:11PM +0100, Laszlo Ersek wrote:> Consider the following two situations: > > (a) For the Windows version being converted, the virtio-win.iso image does > not offer any guest drivers (IOW, the Windows version as a whole is > unsupported by virtio-win.iso). > > (b) For the Windows version being converted, the virtio-win.iso image > offers *some* drivers (but not for all possible paravirtualized > devices). > > There are three relevant device categories: block, network, display. > > If the conversion specifically requests paravirt for a given device > category, and that particular paravirt driver is unavailable -- be that > due to reason (a) or (b) --, the conversion fails, with an error message. > This is expected. > > If the conversion does not express any preference for paravirt in a given > device category, and that particular paravirt driver is unavailable -- be > that due to reason (a) or (b) --, a warning is emitted, and the conversion > continues, using an emulated device in that category. This is expected as > well. > > However, if the conversion explicitly requests an emulated device model > for a given device category -- that is, if the conversion *forbids* > paravirtualization for a given device category --, and that particular > paravirt driver is unavailable anyway, then the behavior between (a) and > (b) differs. Under (a), a warning is emitted (incorrectly!), while under > (b), no warning is emitted. > > Under condition (a), only issue the warning if the conversion permits > paravirtualization in at least one device category.In the current code (since removing --in-place) rcaps is always { None; None; None }, which means select the best possible driver for each of block, bus and video. Since I think we should completely remove rcaps, it only makes sense to think about this case, and not worry about what would happen if rcaps contained anything else.> Fixes: 9ed9d048f2b296cd0ebbd638ac3a2c5c8b47fded > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1961107 > Signed-off-by: Laszlo Ersek <lersek at redhat.com> > --- > convert/windows_virtio.ml | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/convert/windows_virtio.ml b/convert/windows_virtio.ml > index 1851fe2f3292..11b34a03f876 100644 > --- a/convert/windows_virtio.ml > +++ b/convert/windows_virtio.ml > @@ -59,12 +59,14 @@ let rec install_drivers ((g, _) as reg) inspect rcaps > inspect.i_major_version inspect.i_minor_version inspect.i_arch > inspect.i_product_variant virtio_win > > - | { rcaps_block_bus = (Some IDE | None); > + | { rcaps_block_bus = ((Some IDE | None) as block_type); > rcaps_net_bus = ((Some E1000 | Some RTL8139 | None) as net_type); > - rcaps_video = (Some Cirrus | None) } -> > - warning (f_"there are no virtio drivers available for this version of Windows (%d.%d %s %s). virt-v2v looks for drivers in %s\n\nThe guest will be configured to use slower emulated devices.") > - inspect.i_major_version inspect.i_minor_version inspect.i_arch > - inspect.i_product_variant virtio_win; > + rcaps_video = ((Some Cirrus | None) as video_type) } -> > + if block_type = None || net_type = None || video_type = None then > + warning (f_"there are no virtio drivers available for this version of Windows (%d.%d %s %s). virt-v2v looks for drivers in %s\n\nThe guest will be configured to use slower emulated devices.") > + inspect.i_major_version inspect.i_minor_version inspect.i_arch > + inspect.i_product_variant virtio_win > + else ();You don't need the else () clause here, OCaml will assume that. The deeper problem with this patch is that it bypasses the matching which the match statement is already able to do, ie: | { rcaps_block_bus = None; rcaps_net_bus = None; rcaps_video = None } -> warning (f_"...") | { rcaps_block_bus = Some IDE; etc } -> () (* ie. the "else ()" case *) There's a bit more code that follows which might have to be factored out (but read on).> let net_type > match net_type with > | Some model -> modelThe fundamental problem is that we should first remove rcaps, which would simplify the whole thing. I hesitate to submit patches for parts of the code where someone else is working since it creates a mess of conflicts, but would you like me to have a go at a patch for removing rcaps? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/