From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37518) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFhUn-0007QY-75 for qemu-devel@nongnu.org; Tue, 18 Feb 2014 05:01:51 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFhUh-0005nq-88 for qemu-devel@nongnu.org; Tue, 18 Feb 2014 05:01:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:17429) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFhUg-0005nk-VK for qemu-devel@nongnu.org; Tue, 18 Feb 2014 05:01:39 -0500 Date: Tue, 18 Feb 2014 12:06:36 +0200 From: "Michael S. Tsirkin" Message-ID: <20140218100636.GA8114@redhat.com> References: <1392604023-20142-1-git-send-email-akong@redhat.com> <53023E43.1000706@redhat.com> <53023F38.5060406@redhat.com> <53023FA3.4030604@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <53023FA3.4030604@redhat.com> Subject: Re: [Qemu-devel] [PATCH] virtio-net: only output the vlan table when VIRTIO_NET_F_CTRL_VLAN is negotiated List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vlad Yasevich Cc: sf@sfritsch.de, Amos Kong , qemu-devel@nongnu.org, lcapitulino@redhat.com 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.