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

[Vlad Yasevich <vyasevic@redhat.com>]

On 02/17/2014 11:56 AM, Eric Blake wrote:
> 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?
> 

I think a completely empty array should be sufficient.

-vlad