[Qemu-devel] Re: [PATCH 5/5] virtio-net: Add additional MACs via a filter table

[Mark McLoughlin <markmc@redhat.com>]

Hi Alex,

Whole series looks good to me, my suggestions are mostly trivial.

On Tue, 2009-01-13 at 14:24 -0700, Alex Williamson wrote:
> Signed-off-by: Alex Williamson <alex.williamson@hp.com>
> ---
> 
>  qemu/hw/virtio-net.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  qemu/hw/virtio-net.h |    4 +++
>  2 files changed, 77 insertions(+), 1 deletions(-)
> 
> diff --git a/qemu/hw/virtio-net.c b/qemu/hw/virtio-net.c
> index 99e582f..69b7511 100644
> --- a/qemu/hw/virtio-net.c
> +++ b/qemu/hw/virtio-net.c
> @@ -21,7 +21,7 @@
>  
>  #define TAP_VNET_HDR
>  
> -#define VIRTIO_VM_VERSION       3
> +#define VIRTIO_VM_VERSION       4

We could probably do with an ETH_ALEN macro at this stage. There's a lot
of '6's around the place :-)

...
> @@ -157,10 +173,46 @@ static void virtio_net_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
>                  n->allmulti = *on;
>              else
>                  *status = VIRTIO_NET_ERR;
> +
> +        } else if (ctrl->class == VIRTIO_NET_CTRL_MAC_TABLE) {

Hmm, it'd be nice to factor each of the commands out in their own
function.

> +            if (ctrl->cmd == VIRTIO_NET_CTRL_MAC_TABLE_ALLOC) {
> +                uint32_t *entries;
> +
> +                if (n->mac_table.entries || elem.out_num != 2 ||
> +                    elem.out_sg[1].iov_len != sizeof(*entries)) {
> +                    *status = VIRTIO_NET_ERR;
> +                    goto reply;
> +                }
> +
> +                entries = (void *)elem.out_sg[1].iov_base;

No need for the cast.

> +                n->mac_table.macs = qemu_mallocz(*entries * 6);

We should put a limit on the size of the table.

> +                if (!n->mac_table.macs) {
> +                    *status = VIRTIO_NET_ERR;
> +                    goto reply;
> +                }
> +
> +                n->mac_table.entries = *entries;
> +                *status = VIRTIO_NET_OK;
> +            } else if (ctrl->cmd == VIRTIO_NET_CTRL_MAC_TABLE_SET) {
> +                if (!n->mac_table.entries || (elem.out_num == 2 &&

Think I'd just check that out_num is 1 or 2 here ...

then e.g.

   n_entries = 0;
   if (elem.out_num == 2)
       n_entries = elem.out_sg[1].iov_len / 6;

   if (n_entries > n->mac_table.entries) {
       *status = VIRTIO_NET_HDR
   ...

   n->mac_table.in_use = 0;
   if (n_entries) {
   ...

> +                    (elem.out_sg[1].iov_len / 6) > n->mac_table.entries)) {
> +                    *status = VIRTIO_NET_ERR;
> +                    goto reply;
> +                }
> +
> +                if (elem.out_num == 2) {
> +                    memcpy(n->mac_table.macs, elem.out_sg[1].iov_base,
> +                           elem.out_sg[1].iov_len);
> +                    n->mac_table.in_use = elem.out_sg[1].iov_len / 6;
> +                } else {
> +                    n->mac_table.in_use = 0;
> +                }
> +            }
...

Cheers,
Mark.