<html>
<head>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
</head>
<body>
<br>
<div class="moz-forward-container">On 5/19/21 12:49 AM, Cole
Robinson wrote:<br>
<blockquote type="cite">On 4/14/21 11:18 AM, Shalini Chellathurai
Saroja wrote:<br>
<blockquote type="cite">Currently, it is possible to add same
device multiple times, when the<br>
guest domain is in shut-off state. This patch prevents this
unexpected<br>
behavior for all hostdev devices, including mdev devices.<br>
<br>
</blockquote>
If this is an invalid config, these types of validation checks
should<br>
live in libvirt so all clients benefit from them, and libvirt is
the one<br>
source of truth for what's supported or not.<br>
</blockquote>
</div>
<div class="moz-forward-container"><br>
</div>
<div class="moz-forward-container">Hello Cole,<br>
</div>
<div class="moz-forward-container"><br>
</div>
<div class="moz-forward-container">Thank you for the review.<br>
<br>
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.<br>
<br>
With the below changes, libvirt restricts addition of same device
multiple times.<br>
<br>
diff --git a/virtinst/virtxml.py b/virtinst/virtxml.py<br>
index 0c8da37e..6de66488 100644<br>
--- a/virtinst/virtxml.py<br>
+++ b/virtinst/virtxml.py<br>
@@ -222,12 +222,24 @@ def setup_device(dev):<br>
dev.build_storage(cli.get_meter())<br>
<br>
<br>
-def define_changes(conn, inactive_xmlobj, devs, action, confirm):<br>
+def define_changes(domain, conn, inactive_xmlobj, devs, action,
confirm):<br>
if confirm:<br>
if not prompt_yes_or_no(<br>
_("Define '%s' with the changed XML?") %
inactive_xmlobj.name):<br>
return False<br>
<br>
+ if action == "coldplug":<br>
+ msg_fail = _("Error attempting device coldplug:
%(error)s")<br>
+<br>
+ for dev in devs:<br>
+ xml = dev.get_xml()<br>
+ setup_device(dev)<br>
+<br>
+ try:<br>
+ domain.attachDeviceFlags(xml,
libvirt.VIR_DOMAIN_AFFECT_CONFIG)<br>
+ except libvirt.libvirtError as e:<br>
+ fail(msg_fail % {"error": e})<br>
+<br>
if action == "hotplug":<br>
for dev in devs:<br>
setup_device(dev)<br>
@@ -324,7 +336,10 @@ def prepare_changes(xmlobj, options,
parserclass):<br>
<br>
elif options.add_device:<br>
devs = action_add_device(xmlobj, options, parserclass)<br>
- action = "hotplug"<br>
+ if options.update:<br>
+ action = "hotplug"<br>
+ else:<br>
+ action = "coldplug"<br>
<br>
elif options.remove_device:<br>
devs = action_remove_device(xmlobj, options, parserclass)<br>
@@ -524,7 +539,7 @@ def main(conn=None):<br>
action, options.confirm)<br>
return 0<br>
<br>
- dom = define_changes(conn, inactive_xmlobj,<br>
+ dom = define_changes(domain, conn, inactive_xmlobj,<br>
devs, action, options.confirm)<br>
if not dom:<br>
# --confirm user said 'no'<br>
<br>
However, some tests fail as shown below
<style type="text/css">pre { border: 1px solid #000000; padding: 0.02in; direction: inherit }pre.cjk { font-family: "WenQuanYi Zen Hei Sharp", monospace }p { margin-bottom: 0.1in; line-height: 120% }a:link { so-language: zxx }</style>
because the flag is not supported by the test driver.<br>
<br>
ERROR Error attempting device coldplug: this function is not
supported by the connection driver: virDomainAttachDeviceFlags<br>
<br>
TESTSUITE: Expected command to pass, but it didn't.<br>
<br>
<br>
============================= short test summary info
=============================<br>
FAILED tests/test_cli.py::testCLI0455virt_xml - AssertionError:
Command was: ./v...<br>
FAILED tests/test_cli.py::testCLI0458virt_xml_add_host_device -
AssertionError: ...<br>
FAILED tests/test_cli.py::testCLI0459virt_xml_add_sound -
AssertionError: Comman...<br>
FAILED tests/test_cli.py::testCLI0460virt_xml_add_disk_basic -
AssertionError: C...<br>
FAILED tests/test_cli.py::testCLI0461virt_xml_add_disk_notarget -
AssertionError...<br>
FAILED
tests/test_cli.py::testCLI0462virt_xml_add_disk_create_storage -
Assertio...<br>
FAILED
tests/test_cli.py::testCLI0463virt_xml_add_disk_default_storage -
Asserti...<br>
FAILED tests/test_cli.py::testCLI0470virt_xml_add_hostdev_mdev -
AssertionError:...<br>
FAILED
tests/test_cli.py::testCLI0474virt_xml_add_host_device_start -
AssertionE...<br>
FAILED
tests/test_cli.py::testCLI0479virt_xml_kvm_add_disk_os_from_xml -
Asserti...<br>
FAILED
tests/test_cli.py::testCLI0480virt_xml_kvm_add_disk_os_from_cmdline
- Ass...<br>
FAILED
tests/test_cli.py::testCLI0481virt_xml_kvm_add_network_os_from_xml
- Asse...<br>
FAILED
tests/test_cli.py::testCLI0482virt_xml_kvm_add_network_os_from_cmdline
- ...<br>
=================== 13 failed, 582 passed, 2 skipped in 14.01s
====================<br>
<br>
Is it ok to add these tests as invalid like the below example?<br>
</div>
<div class="moz-forward-container"><br>
</div>
<div class="moz-forward-container">c.add_invalid("test-for-virtxml
--add-device --host-device 0x04b3:0x4485 --update --confirm",
input_text="yes") # test driver doesn't support attachdevice...</div>
<div class="moz-forward-container"><br>
</div>
<div class="moz-forward-container">Please let me know your comments.</div>
<div class="moz-forward-container"><br>
</div>
<div class="moz-forward-container">
<style type="text/css">pre { border: 1px solid #000000; padding: 0.02in; direction: inherit }pre.cjk { font-family: "WenQuanYi Zen Hei Sharp", monospace }p { margin-bottom: 0.1in; line-height: 120% }a:link { so-language: zxx }</style>
<blockquote type="cite"><br>
We make some exceptions in virt-install/virt-xml to validate
common<br>
configuration errors but I don't think this is one of them.<br>
<br>
Thanks,<br>
Cole<br>
<br>
</blockquote>
<pre class="moz-signature">--
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
</pre>
</div>
</body>
</html>