Re: [Qemu-devel] [PATCH 1/2] avoid to allcate used macaddr to to new nic

[Michael S. Tsirkin]

On Mon, Jun 17, 2013 at 09:35:10PM +0800, Amos Kong wrote:
> QEMU allocates macaddr to nic if user doesn't assigne macaddr.
> But we didn't check if the allocated macaddr is used, it might
> cause macaddr repeated.
> 
>  # qemu -device e1000,netdev=h1,mac=52:54:00:12:34:56
>   (qemu) device_add e1000
>   (qemu) info network
>   e1000.0: index=0,type=nic,model=e1000,macaddr=52:54:00:12:34:56
>    \ h1: index=0,type=user,net=10.0.2.0,restrict=off
>   e1000.1: index=0,type=nic,model=e1000,macaddr=52:54:00:12:34:56
> 
> This patch adds a check in allocating macaddr, reallocate macaddr
> if it's used by other nic.
> 
> Signed-off-by: Amos Kong <address@hidden>

I'm not sure this is not exactly what was intended in this case.
Also this ptotects against an unlikely case of mixing
implicit and explicit addresses, but not against
a likely case of multiple qemu on same LAN using same MAC.

In short, implicit mac is a bad idea, don't use it.
I'd ack a patch that marks mac non-optional in
help text and documentation.

> ---
>  net/net.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/net/net.c b/net/net.c
> index 43a74e4..f019da4 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -143,15 +143,43 @@ void qemu_macaddr_default_if_unset(MACAddr *macaddr)
>  {
>      static int index = 0;
>      static const MACAddr zero = { .a = { 0,0,0,0,0,0 } };
> +    char info_str[256];
> +    NetClientState *nc, *peer;
> +    NetClientOptionsKind type;
>  
>      if (memcmp(macaddr, &zero, sizeof(zero)) != 0)
>          return;
> +
> +realloc_mac:
>      macaddr->a[0] = 0x52;
>      macaddr->a[1] = 0x54;
>      macaddr->a[2] = 0x00;
>      macaddr->a[3] = 0x12;
>      macaddr->a[4] = 0x34;
>      macaddr->a[5] = 0x56 + index++;
> +
> +    QTAILQ_FOREACH(nc, &net_clients, next) {
> +        peer = nc->peer;
> +        type = nc->info->type;
> +
> +        if (net_hub_id_for_client(nc, NULL) == 0) {
> +            continue;
> +        }
> +
> +        if (!peer || type == NET_CLIENT_OPTIONS_KIND_NIC) {
> +            snprintf(info_str, sizeof(info_str),
> +                     "model=%s,macaddr=%02x:%02x:%02x:%02x:%02x:%02x",
> +                     nc->model,
> +                     macaddr->a[0], macaddr->a[1], macaddr->a[2],
> +                     macaddr->a[3], macaddr->a[4], macaddr->a[5]);
> +
> +            /* reallocate macaddr if it's used by other nic */
> +            if (!strcmp(nc->info_str, info_str)) {
> +                goto realloc_mac;

Please don't code loops like that.

> +            }
> +        }
> +    }
> +
>  }
>  
>  /**
> -- 
> 1.8.1.4