<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body> <div class="moz-cite-prefix">On 5/28/21 10:05 PM, Cole Robinson wrote: </div> <blockquote type="cite" cite="mid:b5570fc1-6e6d-5061-9fed-2e2e23934948@redhat.com"> <pre class="moz-quote-pre" wrap="">On 5/28/21 8:06 AM, Shalini Chellathurai Saroja wrote: </pre> <blockquote type="cite" style="color: #007cff;"> <pre class="moz-quote-pre" wrap="">On 5/19/21 12:49 AM, Cole Robinson wrote: </pre> <blockquote type="cite" style="color: #007cff;"> <pre class="moz-quote-pre" wrap="">On 4/14/21 11:18 AM, Shalini Chellathurai Saroja wrote: </pre> <blockquote type="cite" style="color: #007cff;"> <pre class="moz-quote-pre" wrap="">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. </pre> </blockquote> <pre class="moz-quote-pre" wrap="">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. </pre> </blockquote> <pre class="moz-quote-pre" wrap="">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. </pre> </blockquote> <pre class="moz-quote-pre" wrap="">It's kind of complicated. AFFECT_CONFIG for an offline VM (or the offline XML of a running VM) should in theory be identical to the process we currently use: dumpxml, modify XML, redefine. Anything else is a bug IMO. My reasoning is that if AFFECT_CONFIG does anything special besides hitting the XML define code path, then there's logic locked into the AFFECT_CONFIG code path which isn't available to the many other ways of editing the XML. So for example, if there is a validation check on coldplugging mdev devices in libvirt, and it's only accessible through the attachDevice API, that's a libvirt bug IMO. If the check is worth doing at coldplug time, it's also worth doing at XML define time, and should be part of the generic virDomainDefValidate infrastructure. If it's not worth doing at XML validate time for some reasons, there should be no argument for why it makes sense at coldplug time. That's my theory anyways. I think other libvirt devs will agree but I'm not sure. AFFECT_CONFIG in general is not a good fit for virt-install, for the reasons you show below: we can't rely on it being implemented consistently. It's not implemented in the test driver, and it's not implemented for all devices even in the qemu driver. So we would have to either punt the option to the user by explicitly exposing the flag, or try to keep track internally which cases it works for and what cases it doesn't, neither of which ideas I like. So I think if for this case, libvirt already has the validation you want, but it's only in the attachDevice code path, that's an opportunity to improve libvirt. Move that check to domain_validate.c:virDomainHostdevDefValidate, or possibly to virDomainDefValidateInternal , do something like what virDomainDefDuplicateDiskInfoValidate does Hopefully that makes sense Thanks, Cole </pre> </blockquote> Hello Cole, I agree that this validation check should be included in libvirt for defining the XML too, So I am dropping this commit. Thank you for the explanation. <blockquote type="cite" cite="mid:b5570fc1-6e6d-5061-9fed-2e2e23934948@redhat.com"> <pre class="moz-quote-pre" wrap=""> </pre> </blockquote> <pre class="moz-signature" cols="72">-- 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> </body> </html>