From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34004) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoZy6-0006Ey-I9 for qemu-devel@nongnu.org; Mon, 17 Jun 2013 09:59:41 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UoZy1-0007TB-Bj for qemu-devel@nongnu.org; Mon, 17 Jun 2013 09:59:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:22143) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UoZy1-0007T2-3B for qemu-devel@nongnu.org; Mon, 17 Jun 2013 09:59:33 -0400 Date: Mon, 17 Jun 2013 17:00:18 +0300 From: "Michael S. Tsirkin" Message-ID: <20130617140018.GA10085@redhat.com> References: <1371476111-4449-1-git-send-email-akong@redhat.com> <1371476111-4449-2-git-send-email-akong@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1371476111-4449-2-git-send-email-akong@redhat.com> Subject: Re: [Qemu-devel] [PATCH 1/2] avoid to allcate used macaddr to to new nic List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amos Kong Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org, stefanha@redhat.com 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 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