Libguestfs - Nov 2021 - [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 -> model
The 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/

On 11/02/21 09:52, Richard W.M. Jones wrote:
> 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.
OK.
> 
>> 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.
OK, thanks -- indeed, now that I double-checked, the book I use for
learning does explain this, I just didn't recall it (and when I looked
quickly while writing the patch, I missed that small paragraph).
> 
> 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 *)
I disagree here; the code I wrote was very intentional.

The condition (aka "pattern") we need to handle, in general, is:
"no
paravirt device requested in rcaps at all" [1].

That is, we have an original condition (a set of patterns) that says [0]:
- block is virtio-blk (+ maybe virtio-scsi), OR
- net is virtio-net, OR
- video is QXL.

Then, for [1], we need to handle the negation of [0], which yields (per
de Morgan):
- block hint is missing, or is IDE; AND
- net hint is missing, or is E1000 / RTL8139; AND
- video hint is missing, or is Cirrus.

The *primary* handling for this condition is that we output some basic
(non-paravirt, emulated) device configuration. The *secondary* handling
is that we emit a warning.

The pre-patch code does this alright, except the secondary handling --
the warning. Namely, in case *all* device preferences are explicitly
requested in rcaps, there is nothing to warn the user about. I.e., in
that case, the secondary handling (the warning) should be omitted. If
the user wants emulated devices in all categories, there is no
"degradation" to speak of. The primary handling (outputting the
particular set of guest caps) should stay the same.

This is what the patch fixes. It does not change the primary handling;
it only restricts the warning (the secondary handling) to the case when
at least one device category hint is omitted, and so the "degradation"
occurs, and deserves a mention to the user.

This cannot be implemented at the same level where the patterns are
currently listed, because once a pattern matches, other matches are not
attempted (to my knowledge). (In general, the pattern set should cover
every possibility, but no two patterns should overlap.)

The code you propose has two issues, in my opinion:

- It sets the "at least one device type hint missing in rcaps" case
*entirely apart*, and therefore that case would be handled *only* with a
warning. That's wrong: in that case, we *still* need the primary
handling, namely determining (and returning to the caller) a guest caps
tuple.

- The other issue is that the subcondition we want to associate with the
warning is "all device hints present" (alternatively, the negated
form,
"at least one device hint missing). But the condition (pattern) you
wrote is something else: it says "all device hints missing".
That's not
a condition we care about.


Put more simply, the "if / then / else" expression *subdivides* -- for
the sake of the secondary handling, i.e., for the warning -- the
existent condition [1]. Then, regardless of the branch taken in the "if
/ then / else", we still need to contine to the preexistent return
expression.
> 
> There's a bit more code that follows which might have to be factored
out
Well yes -- if we factor out that that "tail code", then breaking the
logic into entirely separate patterns could be possible. But I still
think it would require a one-level-deeper nesting somewhere, and then
the "if / then / else" subdivision is not much different.
> (but read on).
> 
>>        let net_type >>          match net_type with
>>          | Some model -> model
> 
> The 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?
Yes, absolutely; please go ahead and do it. I'm happy to rebuild my
patches on top of that -- I won't really approach the new situation as a
rebase, with conflicts to resolve, but as a series of potential
individual cherry picks, and reimplementations from zero.

When removing code, my main question is always "granularity". I prefer
"finest granularity" in general, but that may not be right in this
case
(or in the v2v projects in general). It depends quite a bit on reviewer
preference.

Thank you!
Laszlo