[27/41] virtio-net: abstract vlans operations

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hw/virtio-net.c |   21 ++++++++++++++++++---
 1 files changed, 18 insertions(+), 3 deletions(-)

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?

> +}
> +
>  static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>  {
>      VirtIONet *n = DO_UPCAST(VirtIONet, vdev, vdev);
> @@ -306,9 +321,9 @@ static int virtio_net_handle_vlan_table(VirtIONet *n, uint8_t cmd,
>          return VIRTIO_NET_ERR;
> 
>      if (cmd == VIRTIO_NET_CTRL_VLAN_ADD)
> -        n->vlans[vid >> 5] |= (1U << (vid & 0x1f));
> +        vlan_add(n, vid);
>      else if (cmd == VIRTIO_NET_CTRL_VLAN_DEL)
> -        n->vlans[vid >> 5] &= ~(1U << (vid & 0x1f));
> +        vlan_del(n, vid);
>      else
>          return VIRTIO_NET_ERR;
> 
> @@ -471,7 +486,7 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
> 
>      if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
>          int vid = be16_to_cpup((uint16_t *)(ptr + 14)) & 0xfff;
> -        if (!(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
> +        if (!vlan_is_set(n, vid))
>              return 0;
>      }
> 
> -- 
> 1.6.5.2

"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.

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

Later, Juan.

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?

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> 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?

/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.