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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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





[Index of Archives]     [Linux Virtualization]     [KVM Development]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]     [Video 4 Linux]

  Powered by Linux