[libvirt] [PATCH] Introduce a sub-element <driver> for controller
Osier Yang
jyang at redhat.com
Thu Apr 25 04:03:33 UTC 2013
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
More information about the libvir-list
mailing list