Fwd: Re: [PATCH virt-install v1 3/3] virtxml: prevent defining same hostdev again

[Shalini Chellathurai Saroja]

On 5/19/21 12:49 AM, Cole Robinson wrote:
> On 4/14/21 11:18 AM, Shalini Chellathurai Saroja wrote:
>> Currently, it is possible to add same device multiple times, when the
>> guest domain is in shut-off state. This patch prevents this unexpected
>> behavior for all hostdev devices, including mdev devices.
>>
> If this is an invalid config, these types of validation checks should
> live in libvirt so all clients benefit from them, and libvirt is the one
> source of truth for what's supported or not.

Hello Cole,

Thank you for the review.

This validation check is available in libvirt, for this to work, the 
flag "VIR_DOMAIN_AFFECT_CONFIG" has to be set, when the virtual server 
is not in running state.

With the below changes, libvirt restricts addition of same device 
multiple times.

diff --git a/virtinst/virtxml.py b/virtinst/virtxml.py
index 0c8da37e..6de66488 100644
--- a/virtinst/virtxml.py
+++ b/virtinst/virtxml.py
@@ -222,12 +222,24 @@ def setup_device(dev):
      dev.build_storage(cli.get_meter())


-def define_changes(conn, inactive_xmlobj, devs, action, confirm):
+def define_changes(domain, conn, inactive_xmlobj, devs, action, confirm):
      if confirm:
          if not prompt_yes_or_no(
                  _("Define '%s' with the changed XML?") % 
inactive_xmlobj.name):
              return False

+    if action == "coldplug":
+        msg_fail = _("Error attempting device coldplug: %(error)s")
+
+        for dev in devs:
+            xml = dev.get_xml()
+            setup_device(dev)
+
+            try:
+                domain.attachDeviceFlags(xml, 
libvirt.VIR_DOMAIN_AFFECT_CONFIG)
+            except libvirt.libvirtError as e:
+                fail(msg_fail % {"error": e})
+
      if action == "hotplug":
          for dev in devs:
              setup_device(dev)
@@ -324,7 +336,10 @@ def prepare_changes(xmlobj, options, parserclass):

      elif options.add_device:
          devs = action_add_device(xmlobj, options, parserclass)
-        action = "hotplug"
+        if options.update:
+            action = "hotplug"
+        else:
+            action = "coldplug"

      elif options.remove_device:
          devs = action_remove_device(xmlobj, options, parserclass)
@@ -524,7 +539,7 @@ def main(conn=None):
                                     action, options.confirm)
          return 0

-    dom = define_changes(conn, inactive_xmlobj,
+    dom = define_changes(domain, conn, inactive_xmlobj,
                           devs, action, options.confirm)
      if not dom:
          # --confirm user said 'no'

However, some tests fail as shown below because the flag is not 
supported by the test driver.

ERROR    Error attempting device coldplug: this function is not 
supported by the connection driver: virDomainAttachDeviceFlags

TESTSUITE: Expected command to pass, but it didn't.


============================= short test summary info 
=============================
FAILED tests/test_cli.py::testCLI0455virt_xml - AssertionError: Command 
was: ./v...
FAILED tests/test_cli.py::testCLI0458virt_xml_add_host_device - 
AssertionError: ...
FAILED tests/test_cli.py::testCLI0459virt_xml_add_sound - 
AssertionError: Comman...
FAILED tests/test_cli.py::testCLI0460virt_xml_add_disk_basic - 
AssertionError: C...
FAILED tests/test_cli.py::testCLI0461virt_xml_add_disk_notarget - 
AssertionError...
FAILED tests/test_cli.py::testCLI0462virt_xml_add_disk_create_storage - 
Assertio...
FAILED tests/test_cli.py::testCLI0463virt_xml_add_disk_default_storage - 
Asserti...
FAILED tests/test_cli.py::testCLI0470virt_xml_add_hostdev_mdev - 
AssertionError:...
FAILED tests/test_cli.py::testCLI0474virt_xml_add_host_device_start - 
AssertionE...
FAILED tests/test_cli.py::testCLI0479virt_xml_kvm_add_disk_os_from_xml - 
Asserti...
FAILED 
tests/test_cli.py::testCLI0480virt_xml_kvm_add_disk_os_from_cmdline - Ass...
FAILED 
tests/test_cli.py::testCLI0481virt_xml_kvm_add_network_os_from_xml - Asse...
FAILED 
tests/test_cli.py::testCLI0482virt_xml_kvm_add_network_os_from_cmdline - ...
=================== 13 failed, 582 passed, 2 skipped in 14.01s 
====================

Is it ok to add these tests as invalid like the below example?

c.add_invalid("test-for-virtxml --add-device --host-device 0x04b3:0x4485 
--update --confirm", input_text="yes")  # test driver doesn't support 
attachdevice...

Please let me know your comments.

>
> We make some exceptions in virt-install/virt-xml to validate common
> configuration errors but I don't think this is one of them.
>
> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20210528/7f9c0e71/attachment.htm>