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

[Alex Williamson <alex.williamson@hp.com>]

On Wed, 2009-01-14 at 10:15 +0000, Mark McLoughlin wrote:  
> > -#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 :-)

Yep, that'd be easier to search for too.  I'll at least make a local
define for that.

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

It's setup to do that, but each case is still pretty short and simple,
so I hadn't made the cut yet.

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

Agree, I put one on the guest driver and thought I had one here, but
must have missed it.  Need to protect from malicious guests.

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

Good idea, I'll play with this and see what comes out.  Thanks for the
comments.

Alex

> 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;
> > +                }
> > +            }
> ...


-- 
Alex Williamson                             HP Open Source & Linux Org.