Re: [Qemu-devel] [PATCH] virtio-net: only output the vlan table when VIRTIO_NET_F_CTRL_VLAN is negotiated

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2306 bytes --]

On 02/17/2014 09:52 AM, Eric Blake wrote:
> On 02/16/2014 07:27 PM, Amos Kong wrote:
>> Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't
>> filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't negotiated.
>>
>> We should also not send the vlan table to management, this patch makes
>> the vlan-talbe optional.
> 
> s/talbe/table/
> 

>> @@ -4053,7 +4053,7 @@
>>      'multicast-overflow': 'bool',
>>      'unicast-overflow':   'bool',
>>      'main-mac':           'str',
>> -    'vlan-table':         ['int'],
>> +    '*vlan-table':         ['int'],
> 
> Indentation is now off.
> 
>>  - "main-mac": main macaddr string (json-string)
>> -- "vlan-table": a json-array of active vlan id
>> +- "vlan-table": a json-array of active vlan id (optoinal)
> 
> s/optoinal/optional/
> 
> Those fixes are trivial enough, so I'm okay if you correct them then add:

Scratch that.  I revoke my R-b, on the following grounds:

On 02/17/2014 08:27 AM, Vlad Yasevich wrote:
> On 02/16/2014 09:27 PM, Amos Kong wrote:
>> Stefan Fritsch just fixed a virtio-net driver bug [1], virtio-net won't
>> filter out VLAN-tagged packets if VIRTIO_NET_F_CTRL_VLAN isn't
negotiated.
>>
>> We should also not send the vlan table to management, this patch makes
>> the vlan-talbe optional.
>       ^^^^^^^^^^
>        table.
>
> One question I have from the API perspective is can we suddenly change
> something to be optional?  If there are any users of this ,
> wouldn't they have to change now to check the existence of this
> list?

You are correct.  Since the parameter is an output field, older clients
may be depending on it existing.  It is okay to generate an empty array,
but you must not entirely omit the array unless you add further
justification in your commit message that you are 100% positive that
there are no clients of 1.6 that will be broken when the array no longer
appears in the output.

Can you rework the patch to just leave the array empty in the case where
the bit does not indicate it is used?  Or do we need to add a new bool
field to the output for new enough management to know whether to use the
array?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]