From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1LN2mX-0007l5-BO
	for qemu-devel@nongnu.org; Wed, 14 Jan 2009 05:15:29 -0500
Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43)
	id 1LN2mW-0007jb-Hy
	for qemu-devel@nongnu.org; Wed, 14 Jan 2009 05:15:28 -0500
Received: from [199.232.76.173] (port=48421 helo=monty-python.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1LN2mW-0007jT-4P
	for qemu-devel@nongnu.org; Wed, 14 Jan 2009 05:15:28 -0500
Received: from mx2.redhat.com ([66.187.237.31]:52521)
	by monty-python.gnu.org with esmtp (Exim 4.60)
	(envelope-from <markmc@redhat.com>) id 1LN2mU-0006qq-9k
	for qemu-devel@nongnu.org; Wed, 14 Jan 2009 05:15:27 -0500
From: Mark McLoughlin <markmc@redhat.com>
In-Reply-To: <1231881842.9095.196.camel@bling>
References: <1231881842.9095.196.camel@bling>
Content-Type: text/plain
Date: Wed, 14 Jan 2009 10:15:17 +0000
Message-Id: <1231928117.4944.294.camel@localhost.localdomain>
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Subject: [Qemu-devel] Re: [PATCH 5/5] virtio-net: Add additional MACs via a
	filter table
Reply-To: Mark McLoughlin <markmc@redhat.com>, qemu-devel@nongnu.org
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Alex Williamson <alex.williamson@hp.com>
Cc: qemu-devel <qemu-devel@nongnu.org>, kvm <kvm@vger.kernel.org>

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.