[libvirt] [PATCH] Introduce a sub-element <driver> for controller
Laine Stump
laine at laine.org
Wed Apr 24 15:43:05 UTC 2013
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.
More information about the libvir-list
mailing list