From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NkcZZ-0005NM-LA for qemu-devel@nongnu.org; Thu, 25 Feb 2010 07:12:05 -0500 Received: from [199.232.76.173] (port=53805 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NkcZZ-0005N8-6Q for qemu-devel@nongnu.org; Thu, 25 Feb 2010 07:12:05 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1NkcZX-0001ru-HD for qemu-devel@nongnu.org; Thu, 25 Feb 2010 07:12:05 -0500 Received: from mx1.redhat.com ([209.132.183.28]:61615) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NkcZX-0001rk-1o for qemu-devel@nongnu.org; Thu, 25 Feb 2010 07:12:03 -0500 Received: from int-mx04.intmail.prod.int.phx2.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.17]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o1PCC1lN027924 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 25 Feb 2010 07:12:01 -0500 Date: Thu, 25 Feb 2010 14:08:48 +0200 From: "Michael S. Tsirkin" Message-ID: <20100225120848.GA9223@redhat.com> References: <20100225114256.GC9116@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: [Qemu-devel] Re: [PATCH] qdev: Catch attempt to attach more than one device to a netdev List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: Mark McLoughlin , qemu-devel@nongnu.org, Gerd Hoffmann On Thu, Feb 25, 2010 at 01:07:54PM +0100, Markus Armbruster wrote: > "Michael S. Tsirkin" writes: > > > On Thu, Feb 25, 2010 at 11:10:15AM +0100, Markus Armbruster wrote: > >> Guest device and host netdev are peers, i.e. it's a 1:1 relation. > >> However, we fail to enforce that: > >> > >> $ qemu -nodefaults --nographic -netdev user,id=net0 -device e1000,netdev=net0 -device virtio-net-pci,netdev=net0 -monitor stdio > >> QEMU 0.12.50 monitor - type 'help' for more information > >> (qemu) info network > >> Devices not on any VLAN: > >> net0: net=10.0.2.0, restricted=n peer=virtio-net-pci.0 > >> e1000.0: model=e1000,macaddr=52:54:00:12:34:56 peer=net0 > >> virtio-net-pci.0: model=virtio-net-pci,macaddr=52:54:00:12:34:57 peer=net0 > >> > >> It's all downhill from there. > >> > >> Signed-off-by: Markus Armbruster > >> --- > >> hw/qdev-properties.c | 3 +++ > >> net.c | 1 + > >> 2 files changed, 4 insertions(+), 0 deletions(-) > >> > >> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c > >> index 277ff9e..89efd91 100644 > >> --- a/hw/qdev-properties.c > >> +++ b/hw/qdev-properties.c > >> @@ -341,6 +341,9 @@ static int parse_netdev(DeviceState *dev, Property *prop, const char *str) > >> *ptr = qemu_find_netdev(str); > >> if (*ptr == NULL) > >> return -1; > >> + if ((*ptr)->peer) { > >> + return -1; > >> + } > > > > Not even -EBUSY? > > Can this produce a helpful diagnostic message? > > Callers take care of that. It's how property parse methods work. > In this case: > > property "virtio-net-pci.netdev": failed to parse "net0" > can't set property "netdev" to "net0" for "virtio-net-pci" Yes, but does not tell you why. If parse_netdev even just returned a meaningful error code, we could see: can't set property "netdev" to "net0" for "virtio-net-pci": device or resource busy. which is at least a hint that something else uses it. As it is, it looks like qemu could not parse "net0" which is not really right. > >> return 0; > >> } > >> > >> diff --git a/net.c b/net.c > >> index a1bf49f..e6c96d3 100644 > >> --- a/net.c > >> +++ b/net.c > >> @@ -245,6 +245,7 @@ VLANClientState *qemu_new_net_client(NetClientInfo *info, > >> QTAILQ_INSERT_TAIL(&vc->vlan->clients, vc, next); > >> } else { > >> if (peer) { > >> + assert(!peer->peer); > > > > Do we ever get herE? > > If "get here" means "conditional entered": yes. For instance, with the > command line from the commit message, we get here for each of the > -device, like this: > > #0 qemu_new_net_client (info=0x8b2ce0, vlan=0x0, peer=0xcd83f0, > model=0x5ea826 "e1000", name=0x0) at /work/armbru/qemu/net.c:227 > #1 0x000000000047682f in qemu_new_nic (info=0x8b2ce0, conf=0x7fffece4a270, > model=0x5ea826 "e1000", name=0x0, opaque=0x7fffece4a010) > at /work/armbru/qemu/net.c:273 > #2 0x0000000000447c74 in pci_e1000_init (pci_dev=0x7fffece4a010) > at /work/armbru/qemu/hw/e1000.c:1125 > #3 0x0000000000422335 in pci_qdev_init (qdev=0x7fffece4a010, base=0x8b2dc0) > at /work/armbru/qemu/hw/pci.c:1647 > #4 0x00000000004bbb29 in qdev_init (dev=0x7fffece4a010) > at /work/armbru/qemu/hw/qdev.c:265 > > If "get here" means "assertion fails": only if somebody breaks something > elsewhere. And then assertion will slap his wrist. I see. > >> vc->peer = peer; > >> peer->peer = vc; > >> } > >> -- > >> 1.6.6 > >> > >>