From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=36100 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1PoIzE-0001Y6-JT
	for qemu-devel@nongnu.org; Sat, 12 Feb 2011 12:10:21 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1PoIzD-0000cm-0Z
	for qemu-devel@nongnu.org; Sat, 12 Feb 2011 12:10:20 -0500
Received: from mx1.redhat.com ([209.132.183.28]:56156)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <armbru@redhat.com>) id 1PoIzC-0000ch-MJ
	for qemu-devel@nongnu.org; Sat, 12 Feb 2011 12:10:18 -0500
From: Markus Armbruster <armbru@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 03/10] pci: add creation functions that may
	fail
References: <AANLkTim7TDM0aw37apBELtfKNPxvxxYBJ_KBtedh412_@mail.gmail.com>
Date: Sat, 12 Feb 2011 18:10:05 +0100
In-Reply-To: <AANLkTim7TDM0aw37apBELtfKNPxvxxYBJ_KBtedh412_@mail.gmail.com>
	(Blue Swirl's message of "Thu, 3 Feb 2011 20:59:33 +0000")
Message-ID: <m339ntnpqq.fsf@blackfin.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Blue Swirl <blauwirbel@gmail.com>
Cc: qemu-devel <qemu-devel@nongnu.org>

Blue Swirl <blauwirbel@gmail.com> writes:

> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  hw/pci.c |   20 ++++++++++++++++++++
>  hw/pci.h |    4 ++++
>  2 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,
>      return DO_UPCAST(PCIDevice, qdev, dev);
>  }
>
> +PCIDevice *pci_try_create_multifunction(PCIBus *bus, int devfn,
> +                                        bool multifunction,
> +                                        const char *name)
> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_try_create(&bus->qbus, name);
> +    if (!dev) {
> +        return NULL;
> +    }
> +    qdev_prop_set_uint32(dev, "addr", devfn);
> +    qdev_prop_set_bit(dev, "multifunction", multifunction);
> +    return DO_UPCAST(PCIDevice, qdev, dev);
> +}
> +

Near-duplicate of pci_create_multifunction().  What about implementing
pci_create_multifunction() on top of pci_try_create_multifunction()?

>  PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn,
>                                             bool multifunction,
>                                             const char *name)
> @@ -1727,6 +1742,11 @@ PCIDevice *pci_create_simple(PCIBus *bus, int
> devfn, const char *name)
>      return pci_create_simple_multifunction(bus, devfn, false, name);
>  }
>
> +PCIDevice *pci_try_create(PCIBus *bus, int devfn, const char *name)
> +{
> +    return pci_try_create_multifunction(bus, devfn, false, name);
> +}
> +
>  static int pci_find_space(PCIDevice *pdev, uint8_t size)
>  {
>      int config_size = 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,
>  PCIDevice *pci_create_simple_multifunction(PCIBus *bus, int devfn,
>                                             bool multifunction,
>                                             const char *name);
> +PCIDevice *pci_try_create_multifunction(PCIBus *bus, int devfn,
> +                                        bool multifunction,
> +                                        const char *name);
>  PCIDevice *pci_create(PCIBus *bus, int devfn, const char *name);
>  PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name);
> +PCIDevice *pci_try_create(PCIBus *bus, int devfn, const char *name);
>
>  static inline int pci_is_express(const PCIDevice *d)
>  {

Six functions to create PCI devices the "old way", i.e. from board setup
code.  Isn't that baroque?  :)

Existing code varies them along two dimensions:

* Automatically "init" the device ("simple") or not.  The "simple"
  functions save their callers wrapping qdev_init_nofail() around the
  non-simple ones.  Marginally useful.

  Aside: calling something that combinines create with init
  "create_simple" isn't exactly the acme of good taste, in my opinion.

* Multifunction parameter or not.  The non-"multifunction" functions
  save their callers passing a false argument.  They also make the more
  general function have a much longer name.  Bah.

You add a third:

* May return failure or not.  You don't implement it for "simple", thus
  we 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.  With names of
more sensible length.