[libvirt] [PATCH] Use camelCase for XML attribute numQueues
In commit d4bf0a9, we used num_queues for an attribute in the XML, but the consensus is that we use camelCase for that. Since there was no release yet (the above commit describes as v1.0.4-65-gd4bf0a9), we still have time to change it. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- This patch applies on top of Laine's RNG tightening patch [1] and it was proposed in that thread as well. [1] http://www.redhat.com/archives/libvir-list/2013-April/msg01320.html --- docs/formatdomain.html.in | 2 +- docs/schemas/domaincommon.rng | 2 +- src/conf/domain_conf.c | 6 +++--- src/qemu/qemu_command.c | 2 +- tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0cc56d9..a5be162 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2135,7 +2135,7 @@ 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>num_queues</code> + "vmpvscsi". The attribute <code>numQueues</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" diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 3976b82..bcb1453 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1444,7 +1444,7 @@ </attribute> </optional> <optional> - <attribute name="num_queues"> + <attribute name="numQueues"> <ref name="unsignedInt"/> </attribute> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1643f30..0487053 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5195,10 +5195,10 @@ virDomainControllerDefParseXML(xmlNodePtr node, def->model = -1; } - if ((num_queues = virXMLPropString(node, "num_queues"))) { + if ((num_queues = virXMLPropString(node, "numQueues"))) { if (virStrToLong_ui(num_queues, NULL, 10, &def->num_queues) < 0) { virReportError(VIR_ERR_XML_ERROR, - _("Malformed 'num_queues' value '%s'"), num_queues); + _("Malformed 'numQueues' value '%s'"), num_queues); goto error; } } @@ -13531,7 +13531,7 @@ virDomainControllerDefFormat(virBufferPtr buf, } if (def->num_queues) - virBufferAsprintf(buf, " num_queues='%u'", def->num_queues); + virBufferAsprintf(buf, " numQueues='%u'", def->num_queues); switch (def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 009d42d..65675b7 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3574,7 +3574,7 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, !(def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI && def->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("'num_queues' is only supported by virtio-scsi controller")); + _("'numQueues' is only supported by virtio-scsi controller")); return NULL; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml index dfa9cf1..063ccf7 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml @@ -20,7 +20,7 @@ <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' num_queues='8'/> + <controller type='scsi' index='0' model='virtio-scsi' numQueues='8'/> <memballoon model='virtio'/> </devices> </domain> -- 1.8.1.5
On 04/18/2013 03:02 AM, Martin Kletzander wrote:
In commit d4bf0a9, we used num_queues for an attribute in the XML, but the consensus is that we use camelCase for that. Since there was no release yet (the above commit describes as v1.0.4-65-gd4bf0a9), we still have time to change it.
You may want to wait for DV's opinion on naming, but I wasn't aware that we had a consensus on camelCase. In fact, '_' is currently winning, although it's hard to say whether that is limited to older interfaces. $ prefix='\(attribute\|element\) name=.[^'\''"]*' $ git grep "${prefix}[A-Z]" docs/schemas/ | wc 14 42 977 $ git grep "${prefix}-" docs/schemas/ | wc 29 87 1925 $ git grep "${prefix}_" docs/schemas/ | wc 45 135 3075 But I personally don't like _, and think camelCase or - is nicer, so I could live with this change if we get consensus.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- This patch applies on top of Laine's RNG tightening patch [1] and it was proposed in that thread as well.
[1] http://www.redhat.com/archives/libvir-list/2013-April/msg01320.html --- docs/formatdomain.html.in | 2 +- docs/schemas/domaincommon.rng | 2 +- src/conf/domain_conf.c | 6 +++--- src/qemu/qemu_command.c | 2 +- tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-)
The patch is accurate, but I don't want to give ACK without more agreement on what naming convention we want for new XML (we should probably patch HACKING to enforce a style if we have consensus). You are correct that once 1.0.5 is released it will be too late to change, so now is the time to decide. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/18/2013 04:27 PM, Eric Blake wrote:
On 04/18/2013 03:02 AM, Martin Kletzander wrote:
In commit d4bf0a9, we used num_queues for an attribute in the XML, but the consensus is that we use camelCase for that. Since there was no release yet (the above commit describes as v1.0.4-65-gd4bf0a9), we still have time to change it.
You may want to wait for DV's opinion on naming, but I wasn't aware that we had a consensus on camelCase. In fact, '_' is currently winning, although it's hard to say whether that is limited to older interfaces.
$ prefix='\(attribute\|element\) name=.[^'\''"]*' $ git grep "${prefix}[A-Z]" docs/schemas/ | wc 14 42 977 $ git grep "${prefix}-" docs/schemas/ | wc 29 87 1925 $ git grep "${prefix}_" docs/schemas/ | wc 45 135 3075
But I personally don't like _, and think camelCase or - is nicer, so I could live with this change if we get consensus.
At first I mistaken '-' and '_' in the other thread, but I explained that later on. However, this commit message didn't reflect that. I also had the feeling that the naming was mainly restricted for attributes, not that much for elements, but of course it makes sense to make it global.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- This patch applies on top of Laine's RNG tightening patch [1] and it was proposed in that thread as well.
[1] http://www.redhat.com/archives/libvir-list/2013-April/msg01320.html --- docs/formatdomain.html.in | 2 +- docs/schemas/domaincommon.rng | 2 +- src/conf/domain_conf.c | 6 +++--- src/qemu/qemu_command.c | 2 +- tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-)
The patch is accurate, but I don't want to give ACK without more agreement on what naming convention we want for new XML (we should probably patch HACKING to enforce a style if we have consensus). You are correct that once 1.0.5 is released it will be too late to change, so now is the time to decide.
I should've made myself clear enough that it is understandable that this patch is more of a proposal, but I couldn't predict that the discussion in the other thread will grow so fast in reactions (I hoped that this will be the place to talk about our decision). Sorry for rushing it so much, we have enough time before 1.0.5 and I'll wait (of course) for some final decision on how the final XML should look like. Thanks, Martin
On 04/18/2013 10:27 AM, Eric Blake wrote:
On 04/18/2013 03:02 AM, Martin Kletzander wrote:
In commit d4bf0a9, we used num_queues for an attribute in the XML, but the consensus is that we use camelCase for that. Since there was no release yet (the above commit describes as v1.0.4-65-gd4bf0a9), we still have time to change it. You may want to wait for DV's opinion on naming, but I wasn't aware that we had a consensus on camelCase. In fact, '_' is currently winning, although it's hard to say whether that is limited to older interfaces.
$ prefix='\(attribute\|element\) name=.[^'\''"]*' $ git grep "${prefix}[A-Z]" docs/schemas/ | wc 14 42 977 $ git grep "${prefix}-" docs/schemas/ | wc 29 87 1925 $ git grep "${prefix}_" docs/schemas/ | wc 45 135 3075
But I personally don't like _, and think camelCase or - is nicer, so I could live with this change if we get consensus.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- This patch applies on top of Laine's RNG tightening patch [1] and it was proposed in that thread as well.
[1] http://www.redhat.com/archives/libvir-list/2013-April/msg01320.html --- docs/formatdomain.html.in | 2 +- docs/schemas/domaincommon.rng | 2 +- src/conf/domain_conf.c | 6 +++--- src/qemu/qemu_command.c | 2 +- tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-num_queues.xml | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-)
The patch is accurate, but I don't want to give ACK without more agreement on what naming convention we want for new XML (we should probably patch HACKING to enforce a style if we have consensus).
I would agree with that - I've brought this up a couple of times, but always am a bit uncomfortable of being possibly perceived as some sort of obsessive-compulsive person, since it really is an aesthetic choice and there are no written rules to back it up. (and actually I personally have no preference).
You are correct that once 1.0.5 is released it will be too late to change, so now is the time to decide.
I'm now also wondering if numQueues should go in a <driver> subelement, since it is a tuning parameter specific to the backend of one particular scsi controller model. Similar attributes for the virtio <interface> and [some disk model I don't feel like looking up right now] <disk> have also been put in <driver>, and as I understand there will be a similar attribute added to <interface> (only for virtio with vhost-net enabled?), and it will most likely fit best in the <driver> subelement.
On Thu, Apr 18, 2013 at 08:27:56AM -0600, Eric Blake wrote:
On 04/18/2013 03:02 AM, Martin Kletzander wrote:
In commit d4bf0a9, we used num_queues for an attribute in the XML, but the consensus is that we use camelCase for that. Since there was no release yet (the above commit describes as v1.0.4-65-gd4bf0a9), we still have time to change it.
You may want to wait for DV's opinion on naming, but I wasn't aware that we had a consensus on camelCase. In fact, '_' is currently winning, although it's hard to say whether that is limited to older interfaces.
Hum, I don't think I ever suggested such rules at the XML level. We have that convention for code, as eric pointed out it is not as clear for XML (and unfortunately we can't hamonize at this point without quite a bit of churn - we would have to support the old syntax forever anyway). I don't have a strong opinion on _ vs. camelcase for markup, I guess the exact same arguments can be raised pros/cons on visibility and existing use :-\ Seems Eric is rather adverse to _ which was looking like the winner, I dislike '-' myself as it is used for comments, and very uncommon in identifiers in XML (well it's barred from being in an identifier in most language). If Eric really can't stand '_' then maybe the simplest is really to start standardizing on camelCase, Daniel -- Daniel Veillard | Open Source and Standards, Red Hat veillard@redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/
On 04/22/2013 06:58 AM, Daniel Veillard wrote:
On Thu, Apr 18, 2013 at 08:27:56AM -0600, Eric Blake wrote:
On 04/18/2013 03:02 AM, Martin Kletzander wrote:
In commit d4bf0a9, we used num_queues for an attribute in the XML, but the consensus is that we use camelCase for that. Since there was no release yet (the above commit describes as v1.0.4-65-gd4bf0a9), we still have time to change it. You may want to wait for DV's opinion on naming, but I wasn't aware that we had a consensus on camelCase. In fact, '_' is currently winning, although it's hard to say whether that is limited to older interfaces. Hum, I don't think I ever suggested such rules at the XML level. We have that convention for code, as eric pointed out it is not as clear for XML (and unfortunately we can't hamonize at this point without quite a bit of churn - we would have to support the old syntax forever anyway).
Yeah, I don't think there's anything to gain (and lots to lose!) from changing attributes that have already been in a release. And I think I first heard about dislike of underscore in XML from danpb (and incorrectly extrapolated that to you: https://www.redhat.com/archives/libvir-list/2013-April/msg01359.html). Of course in this case, we *could* avoid the camelCase vs underscore entirely by just naming the attribute "queues" instead of numQueues or num_queues (the fact that the attribute is a number makes it pretty obvious that it is the "number of queues"). Since I first noticed the name, I'm actually more concerned that the attribute is in the toplevel of <controller> rather than in a <driver> subelement. It's true that this would be the first <driver> item under <controller>, but more backend-specific tuning attributes may follow, and it would be good to have them all collected together in the same place. Since I've already brought this up in another thread (same message as referenced above), I'll drop this if there's no response, but does anyone else have an opinion about that?
I don't have a strong opinion on _ vs. camelcase for markup, I guess the exact same arguments can be raised pros/cons on visibility and existing use :-\
Seems Eric is rather adverse to _ which was looking like the winner, I dislike '-' myself as it is used for comments, and very uncommon in identifiers in XML (well it's barred from being in an identifier in most language).
If Eric really can't stand '_' then maybe the simplest is really to start standardizing on camelCase,
I don't have a preference either, but think that we should (as much as possible) try to stick with one or the other, at least for new attributes. A decision followed by a written rule in HACKING would be a good way to make that happen.
On 04/22/2013 10:13 AM, Laine Stump wrote:
Of course in this case, we *could* avoid the camelCase vs underscore entirely by just naming the attribute "queues" instead of numQueues or num_queues (the fact that the attribute is a number makes it pretty obvious that it is the "number of queues").
And I notice that the patches for supporting multiple queues in interfaces uses <driver queues='n'/>, so consistency would vote in favor of using the same thing for <controller>.
On 22/04/13 22:43, Laine Stump wrote:
On 04/22/2013 10:13 AM, Laine Stump wrote:
Of course in this case, we *could* avoid the camelCase vs underscore entirely by just naming the attribute "queues" instead of numQueues or num_queues (the fact that the attribute is a number makes it pretty obvious that it is the "number of queues"). And I notice that the patches for supporting multiple queues in interfaces uses <driver queues='n'/>, so consistency would vote in favor of using the same thing for <controller>.
I like the 'queues', as it avoids the question whether to use underscore or camelCase.. :-) So I don't mind changing the existing 'num_queues' into 'queues'. Osier
On Mon, Apr 22, 2013 at 10:43:35AM -0400, Laine Stump wrote:
On 04/22/2013 10:13 AM, Laine Stump wrote:
Of course in this case, we *could* avoid the camelCase vs underscore entirely by just naming the attribute "queues" instead of numQueues or num_queues (the fact that the attribute is a number makes it pretty obvious that it is the "number of queues").
And I notice that the patches for supporting multiple queues in interfaces uses <driver queues='n'/>, so consistency would vote in favor of using the same thing for <controller>.
ACK, consistency is important, it could have worked by applying an uniform rule, but we didn't, so let's try to be consistent by naming similary for similar purpose. Daniel -- Daniel Veillard | Open Source and Standards, Red Hat veillard@redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/
On 04/22/2013 04:43 PM, Laine Stump wrote:
On 04/22/2013 10:13 AM, Laine Stump wrote:
Of course in this case, we *could* avoid the camelCase vs underscore entirely by just naming the attribute "queues" instead of numQueues or num_queues (the fact that the attribute is a number makes it pretty obvious that it is the "number of queues").
And I notice that the patches for supporting multiple queues in interfaces uses <driver queues='n'/>, so consistency would vote in favor of using the same thing for <controller>.
I'm not the '+1' kind of guy, but this makes way more sense in the driver element and I think we would definitely make a use of that in the future, even though it increases the complexity of XML. Since there's still time, should I try to change it to <driver queues... or is somebody else against (or already did) that? (Last time I wrongly assumed that sending the patch would speed up solving the question, so I'm rather asking now). Martin
On 04/23/2013 05:32 AM, Martin Kletzander wrote:
On 04/22/2013 04:43 PM, Laine Stump wrote:
On 04/22/2013 10:13 AM, Laine Stump wrote:
Of course in this case, we *could* avoid the camelCase vs underscore entirely by just naming the attribute "queues" instead of numQueues or num_queues (the fact that the attribute is a number makes it pretty obvious that it is the "number of queues").
And I notice that the patches for supporting multiple queues in interfaces uses <driver queues='n'/>, so consistency would vote in favor of using the same thing for <controller>.
I'm not the '+1' kind of guy, but this makes way more sense in the driver element and I think we would definitely make a use of that in the future, even though it increases the complexity of XML. Since there's still time, should I try to change it to <driver queues... or is somebody else against (or already did) that? (Last time I wrongly assumed that sending the patch would speed up solving the question, so I'm rather asking now).
It still hasn't been done. Martin, can you jump in and do it? Laine, speak up now if you were doing it as part of your vfio <driver> work, although it didn't look like that to me. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 04/23/2013 07:44 PM, Eric Blake wrote:
On 04/23/2013 05:32 AM, Martin Kletzander wrote:
On 04/22/2013 04:43 PM, Laine Stump wrote:
On 04/22/2013 10:13 AM, Laine Stump wrote:
Of course in this case, we *could* avoid the camelCase vs underscore entirely by just naming the attribute "queues" instead of numQueues or num_queues (the fact that the attribute is a number makes it pretty obvious that it is the "number of queues"). And I notice that the patches for supporting multiple queues in interfaces uses <driver queues='n'/>, so consistency would vote in favor of using the same thing for <controller>.
I'm not the '+1' kind of guy, but this makes way more sense in the driver element and I think we would definitely make a use of that in the future, even though it increases the complexity of XML. Since there's still time, should I try to change it to <driver queues... or is somebody else against (or already did) that? (Last time I wrongly assumed that sending the patch would speed up solving the question, so I'm rather asking now). It still hasn't been done. Martin, can you jump in and do it? Laine, speak up now if you were doing it as part of your vfio <driver> work, although it didn't look like that to me.
I thought Osier had said that he would make the change... Yep, he says that here: https://www.redhat.com/archives/libvir-list/2013-April/msg01565.html
On 24/04/13 11:25, Laine Stump wrote:
On 04/23/2013 07:44 PM, Eric Blake wrote:
On 04/23/2013 05:32 AM, Martin Kletzander wrote:
On 04/22/2013 04:43 PM, Laine Stump wrote:
On 04/22/2013 10:13 AM, Laine Stump wrote:
Of course in this case, we *could* avoid the camelCase vs underscore entirely by just naming the attribute "queues" instead of numQueues or num_queues (the fact that the attribute is a number makes it pretty obvious that it is the "number of queues"). And I notice that the patches for supporting multiple queues in interfaces uses <driver queues='n'/>, so consistency would vote in favor of using the same thing for <controller>.
I'm not the '+1' kind of guy, but this makes way more sense in the driver element and I think we would definitely make a use of that in the future, even though it increases the complexity of XML. Since there's still time, should I try to change it to <driver queues... or is somebody else against (or already did) that? (Last time I wrongly assumed that sending the patch would speed up solving the question, so I'm rather asking now). It still hasn't been done. Martin, can you jump in and do it? Laine, speak up now if you were doing it as part of your vfio <driver> work, although it didn't look like that to me. I thought Osier had said that he would make the change...
Yep, he says that here:
https://www.redhat.com/archives/libvir-list/2013-April/msg01565.html
Yes, I planned to do it yesterday, but time occupied, I'm doing it. Osier
participants (5)
-
Daniel Veillard -
Eric Blake -
Laine Stump -
Martin Kletzander -
Osier Yang