Re: [Qemu-devel] [PATCH 03/10] pci: add creation functions that may fail

[Markus Armbruster <armbru@redhat.com>]

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.