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

[Blue Swirl <blauwirbel@gmail.com>]

On Sat, Feb 12, 2011 at 7:10 PM, Markus Armbruster <armbru@redhat.com> wrote:
> 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()?

I considered that, but then the error handling would be duplicated
(from qdev error handling).

>>  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?  :)

No, this is the current way. ;-)

> 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.

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.