From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1NFu05-0002eW-68
	for qemu-devel@nongnu.org; Wed, 02 Dec 2009 13:32:29 -0500
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1NFu00-0002YX-RZ
	for qemu-devel@nongnu.org; Wed, 02 Dec 2009 13:32:28 -0500
Received: from [199.232.76.173] (port=60777 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1NFu00-0002YF-EB
	for qemu-devel@nongnu.org; Wed, 02 Dec 2009 13:32:24 -0500
Received: from mx1.redhat.com ([209.132.183.28]:51774)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <mst@redhat.com>) id 1NFtzx-00065a-SP
	for qemu-devel@nongnu.org; Wed, 02 Dec 2009 13:32:24 -0500
Received: from int-mx08.intmail.prod.int.phx2.redhat.com
	(int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21])
	by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id nB2IWLDH006717
	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK)
	for <qemu-devel@nongnu.org>; Wed, 2 Dec 2009 13:32:21 -0500
Date: Wed, 2 Dec 2009 20:29:39 +0200
From: "Michael S. Tsirkin" <mst@redhat.com>
Message-ID: <20091202182939.GD3956@redhat.com>
References: <cover.1259754426.git.quintela@redhat.com>
	<0212bb69ec7e94e572f4f0a7d8718a6131e9ceaa.1259754427.git.quintela@redhat.com>
	<20091202144915.GD18519@redhat.com> <m3y6llcjvt.fsf@neno.neno>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <m3y6llcjvt.fsf@neno.neno>
Subject: [Qemu-devel] Re: [PATCH 27/41] virtio-net: abstract vlans operations
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Juan Quintela <quintela@redhat.com>
Cc: qemu-devel@nongnu.org

On Wed, Dec 02, 2009 at 07:26:30PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Dec 02, 2009 at 01:04:25PM +0100, Juan Quintela wrote:
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  hw/virtio-net.c |   21 ++++++++++++++++++---
> >>  1 files changed, 18 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> >> index 97db0d0..cf13e94 100644
> >> --- a/hw/virtio-net.c
> >> +++ b/hw/virtio-net.c
> >> @@ -63,6 +63,21 @@ typedef struct VirtIONet
> >>   * - we could suppress RX interrupt if we were so inclined.
> >>   */
> >> 
> >> +static void vlan_add(VirtIONet *n, int vid)
> >> +{
> >> +    n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
> >> +}
> >> +
> >> +static void vlan_del(VirtIONet *n, int vid)
> >> +{
> >> +    n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
> >> +}
> >> +
> >> +static bool vlan_is_set(VirtIONet *n, int vid)
> >> +{
> >> +    return n->vlans[vid >> 5] & ~(1U << (vid & 0x1f));
> >
> > This one looks wrong. Did you check this does not break vlans?
> 
> I don't know how to use vlans (more than one).
> 
> Current code is wrong (it is not big<->little endian safe.
> And it is not trivial to make it correct and work from big/little endian
> old states.

So migration is broken and you want to fix it, fine,
but please do not break the feature itself.
You can't check whether bit "vid"
is set by doing  & ~(1 << vid), can you?

> I commented in the cover patch that this needed testing/investigation.
> How is that supposde to be tested?
> 
> Later, Juan.

Pass in a packet, see if it makes it in?

-- 
MST