[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] virtio-net: only output the vlan table when VIR
From: |
Vlad Yasevich |
Subject: |
Re: [Qemu-devel] [PATCH] virtio-net: only output the vlan table when VIRTIO_NET_F_CTRL_VLAN is negotiated |
Date: |
Tue, 18 Feb 2014 09:04:13 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 |
On 02/18/2014 05:06 AM, Michael S. Tsirkin wrote:
> On Mon, Feb 17, 2014 at 11:58:11AM -0500, Vlad Yasevich wrote:
>> 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
>
> An empty array could mean either no vlans allowed or
> all vlans allowed. Also it's nice if users can detect an old
> buggy qemu.
>
> Let's just add an rx state.
>
Fine with me.
-vlad