Re: [libvirt] [PATCH] Introduce a sub-element <driver> for controller

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>&lt;master&gt;</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.