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@xxxxxxxxxxxxx>
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
