[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Qemu-devel] Re: [PATCH 5/5] virtio-net: Add additional MACs via a filte
From: |
Mark McLoughlin |
Subject: |
[Qemu-devel] Re: [PATCH 5/5] virtio-net: Add additional MACs via a filter table |
Date: |
Wed, 14 Jan 2009 10:15:17 +0000 |
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 <address@hidden>
> ---
>
> 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.