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

[Laine Stump]

On 08/10/2016 05:56 AM, Ján Tomko wrote:
> 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.

Ah, I missed that.

>
> 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.

No, keep it this way. I had missed the fact that you could add the 
element twice and get both. That is a valid use case - as of very 
recently in upstream qemu, a virtio device that is placed on a 
pcie-*-port slot defaults to 1.0-only (in order to save IO port space), 
and we do need a way to get around that in case someone wants such a 
device on a guest whose OS doesn't support virtio-1.0.

>
>> (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?

I can see a problem with only being able to select specific versions or 
"default". If someone just wants to turn off virtio-0.9 but allow 
anything else above, the only way to do that is to individually list all 
the other versions. Of course right now there's only one other version, 
so we can probably just ignore that until/unless a newer virtio version 
comes around.


>
>>> 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 :(

Oh come on, I'm sure you have other methods of amusing us - what about 
that photoshop project abologna suggested?