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

[Juan Quintela]

"Michael S. Tsirkin" <address@hidden> wrote:
> 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?

/me stares at it.  stares something more.
I am enlighted now!!!

/me put brown paper bag on head.

Fixing in next version, thanks.
>
>> 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?

I use default networking, and networking works.  but I don't have more
than one vlan.

Later, Juan.