[Qemu-devel] Re: [PATCH 27/41] virtio-net: abstract vlans operations

[Michael S. Tsirkin]

On Wed, Dec 02, 2009 at 07:26:30PM +0100, Juan Quintela wrote:
> "Michael S. Tsirkin" <address@hidden> wrote:
> > On Wed, Dec 02, 2009 at 01:04:25PM +0100, Juan Quintela wrote:
> >> 
> >> Signed-off-by: Juan Quintela <address@hidden>
> >> ---
> >>  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