On Tue, Aug 09, 2016 at 10:37:30PM -0400, Laine Stump wrote:
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.
Both can be set at the same time. I was thinking it would be useful for libvirt to understand both if someone wants to migrate from a newer libvirt version. But to be useful, they would need to have a QEMU that supports a machine type that does not have 0.9+1.0 as the default and they are using it with an old libvirt, so it probably does not make sense. I still have the single-attribute version on a branch, I could revert back to that.
(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"?)
ALL meaning all the revisions supported by current libvirt does not seem like a good idea to me - if a new version comes along, do we include it in there or not?
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.
I was hoping to amuse the reader.
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
Okay :( Jan
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list