From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33387) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFlHc-000887-4s for qemu-devel@nongnu.org; Tue, 18 Feb 2014 09:04:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WFlHW-0006Cp-5c for qemu-devel@nongnu.org; Tue, 18 Feb 2014 09:04:24 -0500 Received: from mx1.redhat.com ([209.132.183.28]:16868) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WFlHV-0006Cg-UK for qemu-devel@nongnu.org; Tue, 18 Feb 2014 09:04:18 -0500 Message-ID: <5303685D.2040607@redhat.com> Date: Tue, 18 Feb 2014 09:04:13 -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> <53023FA3.4030604@redhat.com> <20140218100636.GA8114@redhat.com> In-Reply-To: <20140218100636.GA8114@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 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: "Michael S. Tsirkin" Cc: sf@sfritsch.de, Amos Kong , qemu-devel@nongnu.org, lcapitulino@redhat.com 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