On 5/19/21 12:46 AM, Cole Robinson wrote:
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
Hello Cole,
Thank you. ok, sure.
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.
yes, it is necessary for my tests. I will move this and the tests to a
separate patch.
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
Sure, I will do it.
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
ok.
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.
Yes, it is not. I will drop this code snippet.
Thank you for the review. I will incorporate these changes and send a v2.
Thanks,
Cole
--
Kind regards
Shalini Chellathurai Saroja
Linux on Z and Virtualization Development
Vorsitzende des Aufsichtsrats: Gregor Pillen
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
