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

[Shalini Chellathurai Saroja <shalini@xxxxxxxxxxxxx>]

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 = ""
+        if options.update:
+            action = ""
+        else:
+            action = ""

     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