[libvirt] [PATCH] Introduce a sub-element <driver> for controller
Like what we did for "disk", "filesystem" and "interface", this introduces sub-element <driver> for "controller", and put the "queues" into it. --- docs/formatdomain.html.in | 26 ++++++++++-------- docs/schemas/domaincommon.rng | 14 ++++++---- src/conf/domain_conf.c | 31 +++++++++++++++------- .../qemuxml2argv-disk-virtio-scsi-num_queues.xml | 4 ++- 4 files changed, 48 insertions(+), 27 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 3dbd58b..9dd283b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2135,17 +2135,14 @@ controller. A "scsi" controller has an optional attribute <code>model</code>, which is one of "auto", "buslogic", "ibmvscsi", "lsilogic", "lsisas1068", "lsisas1078", "virtio-scsi" or - "vmpvscsi". The attribute <code>queues</code> - (<span class="since">1.0.5 (QEMU and KVM only)</span>) specifies - the number of queues for the controller. For best performance, it's - recommended to specify a value matching the number of vCPUs. A "usb" - controller has an optional attribute <code>model</code>, which is one - of "piix3-uhci", "piix4-uhci", "ehci", "ich9-ehci1", "ich9-uhci1", - "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", "pci-ohci" or "nec-xhci". - Additionally, <span class="since">since 0.10.0</span>, if the USB bus - needs to be explicitly disabled for the guest, <code>model='none'</code> - may be used. The PowerPC64 "spapr-vio" addresses do not have an - associated controller. + "vmpvscsi". A "usb" controller has an optional attribute + <code>model</code>, which is one of "piix3-uhci", "piix4-uhci", "ehci", + "ich9-ehci1", "ich9-uhci1", "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", + "pci-ohci" or "nec-xhci". Additionally, + <span class="since">since 0.10.0</span>, if the USB bus needs to be + explicitly disabled for the guest, <code>model='none'</code> may be + used. The PowerPC64 "spapr-vio" addresses do not have an associated + controller. </p> <p> @@ -2156,6 +2153,13 @@ </p> <p> + An optional sub-element <code>driver</code> (<span class="since">1.0.5) + can specify the driver specific options. Currently it only supports + attribute <code>queues</code> (QEMU and KVM only), which specifies the + number of queues for the controller. For best performance, it's recommended + to specify a value matching the number of vCPUs. + </p> + <p> USB companion controllers have an optional sub-element <code><master></code> to specify the exact relationship of the companion to its master controller. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b1c4c2f..d22bb80 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1443,11 +1443,6 @@ </choice> </attribute> </optional> - <optional> - <attribute name="queues"> - <ref name="unsignedInt"/> - </attribute> - </optional> </group> <!-- usb has an optional attribute "model", and optional subelement "master" --> <group> @@ -1493,6 +1488,15 @@ </group> </choice> </interleave> + <optional> + <element name="driver"> + <optional> + <attribute name="queues"> + <ref name="unsignedInt"/> + </attribute> + </optional> + </element> + </optional> </element> </define> <define name="filesystem"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 253c9ef..0b432dd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5156,6 +5156,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, unsigned int flags) { virDomainControllerDefPtr def; + xmlNodePtr cur = NULL; char *type = NULL; char *idx = NULL; char *model = NULL; @@ -5195,12 +5196,19 @@ virDomainControllerDefParseXML(xmlNodePtr node, def->model = -1; } - if ((queues = virXMLPropString(node, "queues"))) { - if (virStrToLong_ui(queues, NULL, 10, &def->queues) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Malformed 'queues' value '%s'"), queues); - goto error; + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(cur->name, BAD_CAST "driver")) + queues = virXMLPropString(cur, "queues"); } + cur = cur->next; + } + + if (queues && virStrToLong_ui(queues, NULL, 10, &def->queues) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Malformed 'queues' value '%s'"), queues); + goto error; } if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) @@ -13524,9 +13532,6 @@ virDomainControllerDefFormat(virBufferPtr buf, virBufferEscapeString(buf, " model='%s'", model); } - if (def->queues) - virBufferAsprintf(buf, " queues='%u'", def->queues); - switch (def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: if (def->opts.vioserial.ports != -1) { @@ -13543,10 +13548,16 @@ virDomainControllerDefFormat(virBufferPtr buf, break; } - if (virDomainDeviceInfoIsSet(&def->info, flags)) { + if (def->queues || virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, ">\n"); - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + + if (def->queues) + virBufferAsprintf(buf, " <driver queues='%u'/>\n", def->queues); + + if (virDomainDeviceInfoIsSet(&def->info, flags) && + virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; + virBufferAddLit(buf, " </controller>\n"); } else { virBufferAddLit(buf, "/>\n"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml index b3b1289..fda976c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml @@ -20,7 +20,9 @@ <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk> <controller type='usb' index='0'/> - <controller type='scsi' index='0' model='virtio-scsi' queues='8'/> + <controller type='scsi' index='0' model='virtio-scsi'> + <driver queues='8'/> + </controller> <memballoon model='virtio'/> </devices> </domain> -- 1.8.1.4
On 04/24/2013 03:24 AM, Osier Yang wrote:
Like what we did for "disk", "filesystem" and "interface", this introduces sub-element <driver> for "controller", and put the "queues" into it. --- docs/formatdomain.html.in | 26 ++++++++++-------- docs/schemas/domaincommon.rng | 14 ++++++---- src/conf/domain_conf.c | 31 +++++++++++++++------- .../qemuxml2argv-disk-virtio-scsi-num_queues.xml | 4 ++- 4 files changed, 48 insertions(+), 27 deletions(-)
Ah, this is what I was looking for.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 3dbd58b..9dd283b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2135,17 +2135,14 @@ controller. A "scsi" controller has an optional attribute <code>model</code>, which is one of "auto", "buslogic", "ibmvscsi", "lsilogic", "lsisas1068", "lsisas1078", "virtio-scsi" or - "vmpvscsi". The attribute <code>queues</code>
and I see that it was on top of your other patch that did s/num_queues/queues/ - maybe you should have sent the two patches as a series.
- (<span class="since">1.0.5 (QEMU and KVM only)</span>) specifies - the number of queues for the controller. For best performance, it's - recommended to specify a value matching the number of vCPUs. A "usb" - controller has an optional attribute <code>model</code>, which is one - of "piix3-uhci", "piix4-uhci", "ehci", "ich9-ehci1", "ich9-uhci1", - "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", "pci-ohci" or "nec-xhci". - Additionally, <span class="since">since 0.10.0</span>, if the USB bus - needs to be explicitly disabled for the guest, <code>model='none'</code> - may be used. The PowerPC64 "spapr-vio" addresses do not have an - associated controller. + "vmpvscsi". A "usb" controller has an optional attribute + <code>model</code>, which is one of "piix3-uhci", "piix4-uhci", "ehci", + "ich9-ehci1", "ich9-uhci1", "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", + "pci-ohci" or "nec-xhci". Additionally, + <span class="since">since 0.10.0</span>, if the USB bus needs to be + explicitly disabled for the guest, <code>model='none'</code> may be + used. The PowerPC64 "spapr-vio" addresses do not have an associated + controller. </p>
<p> @@ -2156,6 +2153,13 @@ </p>
<p> + An optional sub-element <code>driver</code> (<span class="since">1.0.5) + can specify the driver specific options. Currently it only supports + attribute <code>queues</code> (QEMU and KVM only), which specifies the + number of queues for the controller. For best performance, it's recommended + to specify a value matching the number of vCPUs.
Can this sub-element appear for every type of controller, or is it limited to particular types of controllers?
+ </p> + <p> USB companion controllers have an optional sub-element <code><master></code> to specify the exact relationship of the companion to its master controller. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b1c4c2f..d22bb80 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1443,11 +1443,6 @@ </choice> </attribute> </optional> - <optional> - <attribute name="queues"> - <ref name="unsignedInt"/> - </attribute> - </optional>
Spot [1].
</group> <!-- usb has an optional attribute "model", and optional subelement "master" --> <group> @@ -1493,6 +1488,15 @@ </group> </choice> </interleave> + <optional>
You want the sublement to be inside the interleave, so that the user can specify <address> and <driver> in either order. Also, if we only support queues for a particular type of controller (previously, you only had it under the type='scsi' controller), then this block should probably appear as part of the scsi group back at [1].
+ <element name="driver"> + <optional> + <attribute name="queues"> + <ref name="unsignedInt"/> + </attribute> + </optional> + </element> + </optional> </element> </define> <define name="filesystem"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 253c9ef..0b432dd 100644 --- a/src/conf/domain_conf.c
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 24/04/13 23:27, Eric Blake wrote:
On 04/24/2013 03:24 AM, Osier Yang wrote:
Like what we did for "disk", "filesystem" and "interface", this introduces sub-element <driver> for "controller", and put the "queues" into it. --- docs/formatdomain.html.in | 26 ++++++++++-------- docs/schemas/domaincommon.rng | 14 ++++++---- src/conf/domain_conf.c | 31 +++++++++++++++------- .../qemuxml2argv-disk-virtio-scsi-num_queues.xml | 4 ++- 4 files changed, 48 insertions(+), 27 deletions(-) Ah, this is what I was looking for.
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 3dbd58b..9dd283b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2135,17 +2135,14 @@ controller. A "scsi" controller has an optional attribute <code>model</code>, which is one of "auto", "buslogic", "ibmvscsi", "lsilogic", "lsisas1068", "lsisas1078", "virtio-scsi" or - "vmpvscsi". The attribute <code>queues</code> and I see that it was on top of your other patch that did s/num_queues/queues/ - maybe you should have sent the two patches as a series.
Yeah, I should.
- (<span class="since">1.0.5 (QEMU and KVM only)</span>) specifies - the number of queues for the controller. For best performance, it's - recommended to specify a value matching the number of vCPUs. A "usb" - controller has an optional attribute <code>model</code>, which is one - of "piix3-uhci", "piix4-uhci", "ehci", "ich9-ehci1", "ich9-uhci1", - "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", "pci-ohci" or "nec-xhci". - Additionally, <span class="since">since 0.10.0</span>, if the USB bus - needs to be explicitly disabled for the guest, <code>model='none'</code> - may be used. The PowerPC64 "spapr-vio" addresses do not have an - associated controller. + "vmpvscsi". A "usb" controller has an optional attribute + <code>model</code>, which is one of "piix3-uhci", "piix4-uhci", "ehci", + "ich9-ehci1", "ich9-uhci1", "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", + "pci-ohci" or "nec-xhci". Additionally, + <span class="since">since 0.10.0</span>, if the USB bus needs to be + explicitly disabled for the guest, <code>model='none'</code> may be + used. The PowerPC64 "spapr-vio" addresses do not have an associated + controller. </p>
<p> @@ -2156,6 +2153,13 @@ </p>
<p> + An optional sub-element <code>driver</code> (<span class="since">1.0.5) + can specify the driver specific options. Currently it only supports + attribute <code>queues</code> (QEMU and KVM only), which specifies the + number of queues for the controller. For best performance, it's recommended + to specify a value matching the number of vCPUs. Can this sub-element appear for every type of controller, or is it limited to particular types of controllers?
It's only valid for virtio-scsi controller of qemu driver, but what I thought is we can make it generally for all drivers & all controllers. And do the checking inside qemu driver instead.
+ </p> + <p> USB companion controllers have an optional sub-element <code><master></code> to specify the exact relationship of the companion to its master controller. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b1c4c2f..d22bb80 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1443,11 +1443,6 @@ </choice> </attribute> </optional> - <optional> - <attribute name="queues"> - <ref name="unsignedInt"/> - </attribute> - </optional> Spot [1].
</group> <!-- usb has an optional attribute "model", and optional subelement "master" --> <group> @@ -1493,6 +1488,15 @@ </group> </choice> </interleave> + <optional>
You want the sublement to be inside the interleave, so that the user can
Agreed for the interleave.
specify <address> and <driver> in either order. Also, if we only support queues for a particular type of controller (previously, you only had it under the type='scsi' controller), then this block should probably appear as part of the scsi group back at [1].
Previously, since the queues is only valid for virtio-scsi controller, I put in the scsi group, but now we changed to introduce a <driver> sub-element, I think it makes more sense to make the <driver> generic, and doesn't hurt to put the "queues" into it. Osier
On 04/24/2013 05:24 AM, Osier Yang wrote:
Like what we did for "disk", "filesystem" and "interface", this introduces sub-element <driver> for "controller", and put the "queues" into it. --- docs/formatdomain.html.in | 26 ++++++++++-------- docs/schemas/domaincommon.rng | 14 ++++++---- src/conf/domain_conf.c | 31 +++++++++++++++------- .../qemuxml2argv-disk-virtio-scsi-num_queues.xml | 4 ++- 4 files changed, 48 insertions(+), 27 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 3dbd58b..9dd283b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2135,17 +2135,14 @@ controller. A "scsi" controller has an optional attribute <code>model</code>, which is one of "auto", "buslogic", "ibmvscsi", "lsilogic", "lsisas1068", "lsisas1078", "virtio-scsi" or - "vmpvscsi". The attribute <code>queues</code> - (<span class="since">1.0.5 (QEMU and KVM only)</span>) specifies - the number of queues for the controller. For best performance, it's - recommended to specify a value matching the number of vCPUs. A "usb" - controller has an optional attribute <code>model</code>, which is one - of "piix3-uhci", "piix4-uhci", "ehci", "ich9-ehci1", "ich9-uhci1", - "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", "pci-ohci" or "nec-xhci". - Additionally, <span class="since">since 0.10.0</span>, if the USB bus - needs to be explicitly disabled for the guest, <code>model='none'</code> - may be used. The PowerPC64 "spapr-vio" addresses do not have an - associated controller. + "vmpvscsi". A "usb" controller has an optional attribute + <code>model</code>, which is one of "piix3-uhci", "piix4-uhci", "ehci", + "ich9-ehci1", "ich9-uhci1", "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", + "pci-ohci" or "nec-xhci". Additionally, + <span class="since">since 0.10.0</span>, if the USB bus needs to be + explicitly disabled for the guest, <code>model='none'</code> may be + used. The PowerPC64 "spapr-vio" addresses do not have an associated + controller. </p>
It took me a minute to pick out why this hunk was in there (curse this simple-minded line-based diff technology! :-)
<p> @@ -2156,6 +2153,13 @@ </p>
<p> + An optional sub-element <code>driver</code> (<span class="since">1.0.5) + can specify the driver specific options. Currently it only supports + attribute <code>queues</code> (QEMU and KVM only), which specifies the
It may be better to put the <since> down where you say "QEMU and KVM only", so that when other attributes are added in the future, each will have a <since> associated with them (and it's not really required for <driver>, because a <driver> element will just be silently ignored on older versions, and we will have already explicitly noted that any of the attributes within <driver> will be ignored (by giving them a <since>).
+ number of queues for the controller. For best performance, it's recommended + to specify a value matching the number of vCPUs. + </p> + <p> USB companion controllers have an optional sub-element <code><master></code> to specify the exact relationship of the companion to its master controller. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b1c4c2f..d22bb80 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1443,11 +1443,6 @@ </choice> </attribute> </optional> - <optional> - <attribute name="queues"> - <ref name="unsignedInt"/> - </attribute> - </optional> </group> <!-- usb has an optional attribute "model", and optional subelement "master" --> <group> @@ -1493,6 +1488,15 @@ </group> </choice> </interleave> + <optional> + <element name="driver"> + <optional> + <attribute name="queues"> + <ref name="unsignedInt"/> + </attribute> + </optional> + </element> + </optional>
I see an </interleave> just above there. Shouldn't this element also be inside the <interleave> so that all of the elements can be included in different orders?
</element> </define> <define name="filesystem"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 253c9ef..0b432dd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5156,6 +5156,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, unsigned int flags) { virDomainControllerDefPtr def; + xmlNodePtr cur = NULL; char *type = NULL; char *idx = NULL; char *model = NULL; @@ -5195,12 +5196,19 @@ virDomainControllerDefParseXML(xmlNodePtr node, def->model = -1; }
- if ((queues = virXMLPropString(node, "queues"))) { - if (virStrToLong_ui(queues, NULL, 10, &def->queues) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Malformed 'queues' value '%s'"), queues); - goto error; + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(cur->name, BAD_CAST "driver")) + queues = virXMLPropString(cur, "queues"); } + cur = cur->next; + }
We really should standardize on always passing around a ctxt between the parse functions, so that virXPathString() is always usable, rather than needing to resort to all these while loops. (But that's beside the point. What you have follows convention and works :-)
+ + if (queues && virStrToLong_ui(queues, NULL, 10, &def->queues) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Malformed 'queues' value '%s'"), queues); + goto error; }
if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) @@ -13524,9 +13532,6 @@ virDomainControllerDefFormat(virBufferPtr buf, virBufferEscapeString(buf, " model='%s'", model); }
- if (def->queues) - virBufferAsprintf(buf, " queues='%u'", def->queues); - switch (def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: if (def->opts.vioserial.ports != -1) { @@ -13543,10 +13548,16 @@ virDomainControllerDefFormat(virBufferPtr buf, break; }
- if (virDomainDeviceInfoIsSet(&def->info, flags)) { + if (def->queues || virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, ">\n"); - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + + if (def->queues) + virBufferAsprintf(buf, " <driver queues='%u'/>\n", def->queues); + + if (virDomainDeviceInfoIsSet(&def->info, flags) && + virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; + virBufferAddLit(buf, " </controller>\n");
Hmm. This function needs to have its BufferIndents set (again, irrelevant to this patch).
} else { virBufferAddLit(buf, "/>\n"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml index b3b1289..fda976c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml @@ -20,7 +20,9 @@ <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk> <controller type='usb' index='0'/> - <controller type='scsi' index='0' model='virtio-scsi' queues='8'/> + <controller type='scsi' index='0' model='virtio-scsi'> + <driver queues='8'/> + </controller> <memballoon model='virtio'/> </devices> </domain>
ACK, with the doc change and putting the driver element inside the <interleave> in the RNG.
On 24/04/13 23:43, Laine Stump wrote:
On 04/24/2013 05:24 AM, Osier Yang wrote:
Like what we did for "disk", "filesystem" and "interface", this introduces sub-element <driver> for "controller", and put the "queues" into it. --- docs/formatdomain.html.in | 26 ++++++++++-------- docs/schemas/domaincommon.rng | 14 ++++++---- src/conf/domain_conf.c | 31 +++++++++++++++------- .../qemuxml2argv-disk-virtio-scsi-num_queues.xml | 4 ++- 4 files changed, 48 insertions(+), 27 deletions(-)
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 3dbd58b..9dd283b 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2135,17 +2135,14 @@ controller. A "scsi" controller has an optional attribute <code>model</code>, which is one of "auto", "buslogic", "ibmvscsi", "lsilogic", "lsisas1068", "lsisas1078", "virtio-scsi" or - "vmpvscsi". The attribute <code>queues</code> - (<span class="since">1.0.5 (QEMU and KVM only)</span>) specifies - the number of queues for the controller. For best performance, it's - recommended to specify a value matching the number of vCPUs. A "usb" - controller has an optional attribute <code>model</code>, which is one - of "piix3-uhci", "piix4-uhci", "ehci", "ich9-ehci1", "ich9-uhci1", - "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", "pci-ohci" or "nec-xhci". - Additionally, <span class="since">since 0.10.0</span>, if the USB bus - needs to be explicitly disabled for the guest, <code>model='none'</code> - may be used. The PowerPC64 "spapr-vio" addresses do not have an - associated controller. + "vmpvscsi". A "usb" controller has an optional attribute + <code>model</code>, which is one of "piix3-uhci", "piix4-uhci", "ehci", + "ich9-ehci1", "ich9-uhci1", "ich9-uhci2", "ich9-uhci3", "vt82c686b-uhci", + "pci-ohci" or "nec-xhci". Additionally, + <span class="since">since 0.10.0</span>, if the USB bus needs to be + explicitly disabled for the guest, <code>model='none'</code> may be + used. The PowerPC64 "spapr-vio" addresses do not have an associated + controller. </p> It took me a minute to pick out why this hunk was in there (curse this simple-minded line-based diff technology! :-)
<p> @@ -2156,6 +2153,13 @@ </p>
<p> + An optional sub-element <code>driver</code> (<span class="since">1.0.5) + can specify the driver specific options. Currently it only supports + attribute <code>queues</code> (QEMU and KVM only), which specifies the
It may be better to put the <since> down where you say "QEMU and KVM only", so that when other attributes are added in the future, each will have a <since> associated with them (and it's not really required for <driver>, because a <driver> element will just be silently ignored on older versions, and we will have already explicitly noted that any of the attributes within <driver> will be ignored (by giving them a <since>).
Yeah, this sounds better.
+ number of queues for the controller. For best performance, it's recommended + to specify a value matching the number of vCPUs. + </p> + <p> USB companion controllers have an optional sub-element <code><master></code> to specify the exact relationship of the companion to its master controller. diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b1c4c2f..d22bb80 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1443,11 +1443,6 @@ </choice> </attribute> </optional> - <optional> - <attribute name="queues"> - <ref name="unsignedInt"/> - </attribute> - </optional> </group> <!-- usb has an optional attribute "model", and optional subelement "master" --> <group> @@ -1493,6 +1488,15 @@ </group> </choice> </interleave> + <optional> + <element name="driver"> + <optional> + <attribute name="queues"> + <ref name="unsignedInt"/> + </attribute> + </optional> + </element> + </optional>
I see an </interleave> just above there. Shouldn't this element also be inside the <interleave> so that all of the elements can be included in different orders?
Agreed.
</element> </define> <define name="filesystem"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 253c9ef..0b432dd 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5156,6 +5156,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, unsigned int flags) { virDomainControllerDefPtr def; + xmlNodePtr cur = NULL; char *type = NULL; char *idx = NULL; char *model = NULL; @@ -5195,12 +5196,19 @@ virDomainControllerDefParseXML(xmlNodePtr node, def->model = -1; }
- if ((queues = virXMLPropString(node, "queues"))) { - if (virStrToLong_ui(queues, NULL, 10, &def->queues) < 0) { - virReportError(VIR_ERR_XML_ERROR, - _("Malformed 'queues' value '%s'"), queues); - goto error; + cur = node->children; + while (cur != NULL) { + if (cur->type == XML_ELEMENT_NODE) { + if (xmlStrEqual(cur->name, BAD_CAST "driver")) + queues = virXMLPropString(cur, "queues"); } + cur = cur->next; + }
We really should standardize on always passing around a ctxt between the parse functions, so that virXPathString() is always usable, rather than needing to resort to all these while loops. (But that's beside the point. What you have follows convention and works :-)
Agreed, I don't like the while loop either, especially even for parsing a single options.
+ + if (queues && virStrToLong_ui(queues, NULL, 10, &def->queues) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Malformed 'queues' value '%s'"), queues); + goto error; }
if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) @@ -13524,9 +13532,6 @@ virDomainControllerDefFormat(virBufferPtr buf, virBufferEscapeString(buf, " model='%s'", model); }
- if (def->queues) - virBufferAsprintf(buf, " queues='%u'", def->queues); - switch (def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: if (def->opts.vioserial.ports != -1) { @@ -13543,10 +13548,16 @@ virDomainControllerDefFormat(virBufferPtr buf, break; }
- if (virDomainDeviceInfoIsSet(&def->info, flags)) { + if (def->queues || virDomainDeviceInfoIsSet(&def->info, flags)) { virBufferAddLit(buf, ">\n"); - if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + + if (def->queues) + virBufferAsprintf(buf, " <driver queues='%u'/>\n", def->queues); + + if (virDomainDeviceInfoIsSet(&def->info, flags) && + virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; + virBufferAddLit(buf, " </controller>\n");
Hmm. This function needs to have its BufferIndents set (again, irrelevant to this patch).
Yeah, it's another thing better to clean up in domain_conf.c. Except these, I believe domain_conf.c has much things to clean up. Let's do it later.
} else { virBufferAddLit(buf, "/>\n"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml index b3b1289..fda976c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml @@ -20,7 +20,9 @@ <address type='drive' controller='0' bus='0' target='0' unit='0'/> </disk> <controller type='usb' index='0'/> - <controller type='scsi' index='0' model='virtio-scsi' queues='8'/> + <controller type='scsi' index='0' model='virtio-scsi'> + <driver queues='8'/> + </controller> <memballoon model='virtio'/> </devices> </domain>
ACK, with the doc change and putting the driver element inside the <interleave> in the RNG.
Thanks, I pushed it with the docs changed and putting the driver element inside <interleave>. Osier
participants (3)
-
Eric Blake -
Laine Stump -
Osier Yang