[PATCH virt-install v1 2/3] virt-xml: add support for mediated devices

[Cole Robinson]

On 4/14/21 11:18 AM, Shalini Chellathurai Saroja wrote:
> Provide support to add/remove mdev in a guest domain, which is in
> shut-off or running state (hotplug/unplug). Also support update of
> already existing mdev device, when the guest domain is in shut-off
> state. Please note that libvirt does not support update of mdev
> device, when the guest domain is in running state.
> 
> Signed-off-by: Shalini Chellathurai Saroja <shalini at linux.ibm.com>

Patch looks pretty good, but I have some comments that I think will
require a v2. See below

> diff --git a/tests/utils.py b/tests/utils.py
> index 802d6590..8a8a243a 100644
> --- a/tests/utils.py
> +++ b/tests/utils.py
> @@ -232,7 +232,7 @@ def diff_compare(actual_out, filename=None, expect_out=None):
>              open(filename, "w").write(actual_out)
>          expect_out = open(filename).read()
>  
> -    diff = xmlutil.diff(expect_out, actual_out,
> +    diff = xmlutil.diff(expect_out.rstrip(), actual_out.rstrip(),
>              filename or '', "Generated output")

Is this necessary? I can't tell from test output what the point is. If
so I'd rather have it done as a separate patch so it's easier to see how
it affects the test suite output in isolation.

>      if diff:
>          raise AssertionError("Conversion outputs did not match.\n%s" % diff)
> diff --git a/virtinst/nodedev.py b/virtinst/nodedev.py
> index 97841794..873002ec 100644
> --- a/virtinst/nodedev.py
> +++ b/virtinst/nodedev.py
> @@ -5,6 +5,7 @@
>  # See the COPYING file in the top-level directory.
>  
>  import os
> +import uuid
>  
>  from .logger import log
>  from .xmlbuilder import XMLBuilder, XMLProperty, XMLChildProperty
> @@ -101,6 +102,12 @@ class NodeDevice(XMLBuilder):
>                  _compare_int(self.bus, hostdev.bus) and
>                  _compare_int(self.device, hostdev.device))
>  
> +        if self.device_type == "mdev":
> +            if hostdev.type != "mdev":
> +                return False
> +
> +            return uuid.UUID(self.name[5:].replace('_', '-')) == uuid.UUID(hostdev.uuid)
> +

These UUID conversions should be wrapped in try except. See how
_compare_int handles int conversions as an example. We need to be very
conservative here because of the way this lookup path is used in places
like virt-manager IIRC

Also the self.name[5:]... bit is repeated in several places. It would
help to add a get_mdev_uuid() function to NodeDevice IMO that
centralizes this logic

>          return False
>  
>  
> @@ -207,6 +214,10 @@ def _AddressStringToHostdev(conn, addrstr):
>              hostdev.type = "usb"
>              hostdev.bus = bus
>              hostdev.device = device
> +
> +        elif "mdev_" in addrstr:
> +            hostdev.type = "mdev"
> +            hostdev.uuid = addrstr[5:].replace('_', '-')
>          else:
>              raise RuntimeError("Unknown address type")
>      except Exception:
> 

This special syntactic sugar is intended only for --hostdev $PCI and
--hostdev $USB addresses of the well known address forms. I don't think
it's necessary for this case, so please drop it or explain what I missed.

Thanks,
Cole