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

[Michael S. Tsirkin]

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.