Re: [libvirt] [PATCHv2 02/11] Add virtio revision attribute to memballoon

On 08/08/2016 12:35 PM, Ján Tomko wrote: > <memballoon model='virtio'> > <virtio revision='0.9'/> > </memballoon> > > https://bugzilla.redhat.com/show_bug.cgi?id=1227354 > --- > docs/formatdomain.html.in | 9 +++ > docs/schemas/domaincommon.rng | 16 ++++ > src/conf/domain_conf.c | 86 ++++++++++++++++++++++ > src/conf/domain_conf.h | 9 +++ > .../qemuxml2argv-virtio-revision.xml | 54 ++++++++++++++ > .../qemuxml2xmlout-virtio-revision.xml | 54 ++++++++++++++ > tests/qemuxml2xmltest.c | 2 + > 7 files changed, 230 insertions(+) > create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-virtio-revision.xml > create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-virtio-revision.xml

> + if (!(revmap = virBitmapNew(VIR_DOMAIN_VIRTIO_REVISION_LAST))) > + goto cleanup; > + > + for (i = 0; i < n; i++) { > + int val; > + > + ctxt->node = nodes[i]; > + > + if (!(str = virXPathString("string(./@revision)", ctxt))) { > + virReportError(VIR_ERR_XML_ERROR, "%s", > + _("Missing 'revision' attribute in <virtio> element")); > + goto cleanup; > + } > + > + if ((val = virDomainVirtioRevisionTypeFromString(str)) <= 0) { > + virReportError(VIR_ERR_XML_ERROR, > + _("Unable to parse virtio revision: '%s'"), > + str); > + goto cleanup; > + } > + > + ignore_value(virBitmapSetBit(revmap, val)); It is just really.... odd that you are storing an attribute that can have one of two possible values as a bitmap. Unless you have some specific future plan for that, it really should just be stored as, well, the value itself.

(Maybe your idea is to maybe someday allow forcing support for both? If so, then how about an enum value called "ALL" that gets turned into "disable-legacy=off,disable-modern=off"?)

> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h > index 8b26724..2b39f41 100644 > --- a/src/conf/domain_conf.h > +++ b/src/conf/domain_conf.h > @@ -154,6 +154,13 @@ typedef virDomainTPMDef *virDomainTPMDefPtr; > typedef struct _virDomainIOMMUDef virDomainIOMMUDef; > typedef virDomainIOMMUDef *virDomainIOMMUDefPtr; > > +typedef enum { > + VIR_DOMAIN_VIRTIO_REVISION_DEFAULT, > + VIR_DOMAIN_VIRTIO_REVISION_HERITAGE, > + VIR_DOMAIN_VIRTIO_REVISION_CONTEMPORARY, > + VIR_DOMAIN_VIRTIO_REVISION_LAST, > +} virDomainVirtioRevision; And beyond storing it in a bitmap, why do you invent *yet another* name for these things.

It was confusing enough that virtio 0.9 is also known as "legacy", and 1.0 as "modern", but now you've come up with a 3rd way to say the same thing. I would suggest giving them the same names as the strings that they represent: VIR_DOMAIN_VIRTIO_REVISION_0_9 VIR_DOMAIN_VIRTIO_REVISION_1_0