From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55275) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFRWQ-0006El-M3 for qemu-devel@nongnu.org; Mon, 17 Feb 2014 11:58:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFRWK-00019j-Mh for qemu-devel@nongnu.org; Mon, 17 Feb 2014 11:58:22 -0500 Received: from mx1.redhat.com ([209.132.183.28]:55229) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFRWK-00019b-D1 for qemu-devel@nongnu.org; Mon, 17 Feb 2014 11:58:16 -0500 Message-ID: <53023FA3.4030604@redhat.com> Date: Mon, 17 Feb 2014 11:58:11 -0500 From: Vlad Yasevich MIME-Version: 1.0 References: <1392604023-20142-1-git-send-email-akong@redhat.com> <53023E43.1000706@redhat.com> <53023F38.5060406@redhat.com> In-Reply-To: <53023F38.5060406@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] virtio-net: only output the vlan table when VIRTIO_NET_F_CTRL_VLAN is negotiated Reply-To: vyasevic@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , Amos Kong , qemu-devel@nongnu.org Cc: lcapitulino@redhat.com, sf@sfritsch.de, mst@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