[libvirt] [RFC] conf: Order of AddImplicitControllers and DomainDefPostParse
Implicit controllers may be dependent on device definitions altered in a post-parse callback. E.g., if a console device is defined without the target type, the type will be set in QEMU's callback. In the case of s390, this is virtio, which requires an implicit virtio-serial controller. By moving the implicit controller definition after the post-parse procssing this can be fixed. OTOH the implicit controllers might need post-parse actions as well, although I am currentyl not aware of one. What would speak against swapping the order as suggested in the patch below? Feedback welcome. Signed-off-by: Viktor Mihajlovski <mihajlov@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 8 ++++---- .../qemuxml2xmlout-balloon-device-auto.xml | 2 +- .../qemuxml2xmlout-channel-virtio-auto.xml | 2 +- .../qemuxml2xmlout-console-virtio.xml | 2 +- .../qemuxml2xmlout-disk-scsi-device-auto.xml | 2 +- 5 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 29d6edc..ef1f876 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -12062,14 +12062,14 @@ virDomainDefParseXML(xmlDocPtr xml, (def->ns.parse)(xml, root, ctxt, &def->namespaceData) < 0) goto error; - /* Auto-add any implied controllers which aren't present */ - if (virDomainDefAddImplicitControllers(def) < 0) - goto error; - /* callback to fill driver specific domain aspects */ if (virDomainDefPostParse(def, caps, xmlopt) < 0) goto error; + /* Auto-add any implied controllers which aren't present */ + if (virDomainDefAddImplicitControllers(def) < 0) + goto error; + virHashFree(bootHash); return def; diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-auto.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-auto.xml index 6aed326..380b70f 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-auto.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-balloon-device-auto.xml @@ -20,8 +20,8 @@ <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk> <controller type='usb' index='0'/> - <controller type='ide' index='0'/> <controller type='pci' index='0' model='pci-root'/> + <controller type='ide' index='0'/> <memballoon model='virtio'/> </devices> </domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml index 0175272..fd6b852 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-channel-virtio-auto.xml @@ -25,8 +25,8 @@ <controller type='virtio-serial' index='1'> <address type='pci' domain='0x0000' bus='0x00' slot='0x0a' function='0x0'/> </controller> - <controller type='virtio-serial' index='2'/> <controller type='pci' index='0' model='pci-root'/> + <controller type='virtio-serial' index='2'/> <channel type='pty'> <target type='virtio' name='org.linux-kvm.port.0'/> <address type='virtio-serial' controller='0' bus='0' port='1'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-console-virtio.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-console-virtio.xml index 3c865c3..340430e 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-console-virtio.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-console-virtio.xml @@ -21,8 +21,8 @@ </disk> <controller type='usb' index='0'/> <controller type='ide' index='0'/> - <controller type='virtio-serial' index='0'/> <controller type='pci' index='0' model='pci-root'/> + <controller type='virtio-serial' index='0'/> <console type='pty'> <target type='virtio' port='0'/> </console> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-device-auto.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-device-auto.xml index 7d152bc..5ec1e94 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-device-auto.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-scsi-device-auto.xml @@ -26,8 +26,8 @@ </disk> <controller type='usb' index='0'/> <controller type='ide' index='0'/> - <controller type='scsi' index='0'/> <controller type='pci' index='0' model='pci-root'/> + <controller type='scsi' index='0'/> <memballoon model='virtio'/> </devices> </domain> -- 1.7.9.5
On 06/14/2013 02:34 PM, Viktor Mihajlovski wrote:
Implicit controllers may be dependent on device definitions altered in a post-parse callback. E.g., if a console device is defined without the target type, the type will be set in QEMU's callback. In the case of s390, this is virtio, which requires an implicit virtio-serial controller.
By moving the implicit controller definition after the post-parse procssing this can be fixed. OTOH the implicit controllers might need post-parse actions as well, although I am currentyl not aware of one.
What would speak against swapping the order as suggested in the patch below? Feedback welcome.
I can't say I'd be against that. I hope there's no implicit controller that needs post-parse function, because when there is, all the actions can be taken care of in the function. And if same things need to be checked for, deduplication of such code takes care of it. In case such code movements will show that these two functions would be better off merged together, than it can be acted upon (I'm going to extremes with this unnecessary explanation). However, it would be nice if you could also add a test case to show and check for the behavior you're trying to achieve. Other than that I'd be ok with such movement, but also happy if someone else provided his/her opinion as well. Have a nice day, Martin
participants (2)
-
Martin Kletzander -
Viktor Mihajlovski