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

Ján Tomko jtomko at redhat.com
Wed Aug 10 09:56:25 UTC 2016


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160810/2c10c8d4/attachment-0001.sig>


More information about the libvir-list mailing list