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
