From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=33594 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PoJEU-0001rx-DH for qemu-devel@nongnu.org; Sat, 12 Feb 2011 12:26:07 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PoJET-0004Nf-3g for qemu-devel@nongnu.org; Sat, 12 Feb 2011 12:26:06 -0500 Received: from mail-vw0-f45.google.com ([209.85.212.45]:51630) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PoJES-0004NQ-SV for qemu-devel@nongnu.org; Sat, 12 Feb 2011 12:26:04 -0500 Received: by vws12 with SMTP id 12so2294178vws.4 for ; Sat, 12 Feb 2011 09:26:04 -0800 (PST) MIME-Version: 1.0 In-Reply-To: References: From: Blue Swirl Date: Sat, 12 Feb 2011 19:25:44 +0200 Message-ID: Subject: Re: [Qemu-devel] [PATCH 03/10] pci: add creation functions that may fail Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: qemu-devel On Sat, Feb 12, 2011 at 7:10 PM, Markus Armbruster wrot= e: > Blue Swirl writes: > >> Signed-off-by: Blue Swirl >> --- >> =C2=A0hw/pci.c | =C2=A0 20 ++++++++++++++++++++ >> =C2=A0hw/pci.h | =C2=A0 =C2=A04 ++++ >> =C2=A02 files changed, 24 insertions(+), 0 deletions(-) >> >> diff --git a/hw/pci.c b/hw/pci.c >> index d5bbba9..5e6e216 100644 >> --- a/hw/pci.c >> +++ b/hw/pci.c >> @@ -1708,6 +1708,21 @@ PCIDevice *pci_create_multifunction(PCIBus >> *bus, int devfn, bool multifunction, >> =C2=A0 =C2=A0 =C2=A0return DO_UPCAST(PCIDevice, qdev, dev); >> =C2=A0} >> >> +PCIDevice *pci_try_create_multifunction(PCIBus *bus, int devfn, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0bool m= ultifunction, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0const = char *name) >> +{ >> + =C2=A0 =C2=A0DeviceState *dev; >> + >> + =C2=A0 =C2=A0dev =3D qdev_try_create(&bus->qbus, name); >> + =C2=A0 =C2=A0if (!dev) { >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0return NULL; >> + =C2=A0 =C2=A0} >> + =C2=A0 =C2=A0qdev_prop_set_uint32(dev, "addr", devfn); >> + =C2=A0 =C2=A0qdev_prop_set_bit(dev, "multifunction", multifunction); >> + =C2=A0 =C2=A0return DO_UPCAST(PCIDevice, qdev, dev); >> +} >> + > > Near-duplicate of pci_create_multifunction(). =C2=A0What about implementi= ng > pci_create_multifunction() on top of pci_try_create_multifunction()? I considered that, but then the error handling would be duplicated (from qdev error handling). >> =C2=A0PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn, >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 bool multifunction, >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 const char *name) >> @@ -1727,6 +1742,11 @@ PCIDevice *pci_create_simple(PCIBus *bus, int >> devfn, const char *name) >> =C2=A0 =C2=A0 =C2=A0return pci_create_simple_multifunction(bus, devfn, f= alse, name); >> =C2=A0} >> >> +PCIDevice *pci_try_create(PCIBus *bus, int devfn, const char *name) >> +{ >> + =C2=A0 =C2=A0return pci_try_create_multifunction(bus, devfn, false, na= me); >> +} >> + >> =C2=A0static int pci_find_space(PCIDevice *pdev, uint8_t size) >> =C2=A0{ >> =C2=A0 =C2=A0 =C2=A0int config_size =3D pci_config_size(pdev); >> diff --git a/hw/pci.h b/hw/pci.h >> index 0d2753f..113e556 100644 >> --- a/hw/pci.h >> +++ b/hw/pci.h >> @@ -453,8 +453,12 @@ PCIDevice *pci_create_multifunction(PCIBus *bus, >> int devfn, bool multifunction, >> =C2=A0PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn, >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 bool multifunction, >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 const char *name); >> +PCIDevice *pci_try_create_multifunction(PCIBus *bus, int devfn, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0bool m= ultifunction, >> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0const = char *name); >> =C2=A0PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name); >> =C2=A0PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *n= ame); >> +PCIDevice *pci_try_create(PCIBus *bus, int devfn, const char *name); >> >> =C2=A0static inline int pci_is_express(const PCIDevice *d) >> =C2=A0{ > > Six functions to create PCI devices the "old way", i.e. from board setup > code. =C2=A0Isn't that baroque? =C2=A0:) No, this is the current way. ;-) > Existing code varies them along two dimensions: > > * Automatically "init" the device ("simple") or not. =C2=A0The "simple" > =C2=A0functions save their callers wrapping qdev_init_nofail() around the > =C2=A0non-simple ones. =C2=A0Marginally useful. > > =C2=A0Aside: calling something that combinines create with init > =C2=A0"create_simple" isn't exactly the acme of good taste, in my opinion= . > > * Multifunction parameter or not. =C2=A0The non-"multifunction" functions > =C2=A0save their callers passing a false argument. =C2=A0They also make t= he more > =C2=A0general function have a much longer name. =C2=A0Bah. > > You add a third: > > * May return failure or not. =C2=A0You don't implement it for "simple", t= hus > =C2=A0we end up with "only" six rather than eight functions. > > I figure just your pci_try_create_multifunction() and > pci_create_simple_multifunction() would do just fine. =C2=A0With names of > more sensible length. Alternatively there could be one or two combined generic functions with parameters to specify whether to return on failure or not etc., others could be implemented on top of these.