Patchwork [09/10] sysbus: add creation function that may fail

login
register
mail settings
Submitter Blue Swirl
Date Feb. 3, 2011, 9:02 p.m.
Message ID <AANLkTinqf7c4x61_=T9yqyMxHQVELpSef-70=tv7Efho@mail.gmail.com>
Download mbox | patch
Permalink /patch/81780/
State New
Headers show

Comments

Blue Swirl - Feb. 3, 2011, 9:02 p.m.
Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 hw/sysbus.c |   31 +++++++++++++++++++++++++++++++
 hw/sysbus.h |    9 +++++++++
 2 files changed, 40 insertions(+), 0 deletions(-)

                                               qemu_irq irq)
@@ -64,4 +66,11 @@ static inline DeviceState
*sysbus_create_simple(const char *name,
     return sysbus_create_varargs(name, addr, irq, NULL);
 }

+static inline DeviceState *sysbus_try_create_simple(const char *name,
+                                                    target_phys_addr_t addr,
+                                                    qemu_irq irq)
+{
+    return sysbus_try_create_varargs(name, addr, irq, NULL);
+}
+
 #endif /* !HW_SYSBUS_H */
Markus Armbruster - Feb. 12, 2011, 5:15 p.m.
Blue Swirl <blauwirbel@gmail.com> writes:

> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> ---
>  hw/sysbus.c |   31 +++++++++++++++++++++++++++++++
>  hw/sysbus.h |    9 +++++++++
>  2 files changed, 40 insertions(+), 0 deletions(-)
>
> diff --git a/hw/sysbus.c b/hw/sysbus.c
> index 1583bd8..8980f34 100644
> --- a/hw/sysbus.c
> +++ b/hw/sysbus.c
> @@ -173,6 +173,37 @@ DeviceState *sysbus_create_varargs(const char *name,
>      return dev;
>  }
>
> +DeviceState *sysbus_try_create_varargs(const char *name,
> +                                       target_phys_addr_t addr, ...)
> +{
> +    DeviceState *dev;
> +    SysBusDevice *s;
> +    va_list va;
> +    qemu_irq irq;
> +    int n;
> +
> +    dev = qdev_try_create(NULL, name);
> +    if (!dev) {
> +        return NULL;
> +    }
> +    s = sysbus_from_qdev(dev);
> +    qdev_init_nofail(dev);
> +    if (addr != (target_phys_addr_t)-1) {
> +        sysbus_mmio_map(s, 0, addr);
> +    }
> +    va_start(va, addr);
> +    n = 0;
> +    while (1) {
> +        irq = va_arg(va, qemu_irq);
> +        if (!irq) {
> +            break;
> +        }
> +        sysbus_connect_irq(s, n, irq);
> +        n++;
> +    }
> +    return dev;
> +}
> +

I really don't like duplicating so much of sysbus_create_varargs()
here.  Could it be implemented on top of your
sysbus_try_create_varargs()?

Aside: I also don't like the use of variadic arguments for passing IRQ
numbers.  It's not type-safe.  Why not pass an array and be done with
it?

>  static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
>  {
>      SysBusDevice *s = sysbus_from_qdev(dev);
> diff --git a/hw/sysbus.h b/hw/sysbus.h
> index e9eb618..4e8cb16 100644
> --- a/hw/sysbus.h
> +++ b/hw/sysbus.h
> @@ -57,6 +57,8 @@ void sysbus_mmio_map(SysBusDevice *dev, int n,
> target_phys_addr_t addr);
>  /* Legacy helper function for creating devices.  */
>  DeviceState *sysbus_create_varargs(const char *name,
>                                   target_phys_addr_t addr, ...);
> +DeviceState *sysbus_try_create_varargs(const char *name,
> +                                       target_phys_addr_t addr, ...);
>  static inline DeviceState *sysbus_create_simple(const char *name,
>                                                target_phys_addr_t addr,
>                                                qemu_irq irq)
> @@ -64,4 +66,11 @@ static inline DeviceState
> *sysbus_create_simple(const char *name,
>      return sysbus_create_varargs(name, addr, irq, NULL);
>  }
>
> +static inline DeviceState *sysbus_try_create_simple(const char *name,
> +                                                    target_phys_addr_t addr,
> +                                                    qemu_irq irq)
> +{
> +    return sysbus_try_create_varargs(name, addr, irq, NULL);
> +}
> +
>  #endif /* !HW_SYSBUS_H */
Blue Swirl - Feb. 12, 2011, 5:38 p.m.
On Sat, Feb 12, 2011 at 7:15 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>> Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
>> ---
>>  hw/sysbus.c |   31 +++++++++++++++++++++++++++++++
>>  hw/sysbus.h |    9 +++++++++
>>  2 files changed, 40 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/sysbus.c b/hw/sysbus.c
>> index 1583bd8..8980f34 100644
>> --- a/hw/sysbus.c
>> +++ b/hw/sysbus.c
>> @@ -173,6 +173,37 @@ DeviceState *sysbus_create_varargs(const char *name,
>>      return dev;
>>  }
>>
>> +DeviceState *sysbus_try_create_varargs(const char *name,
>> +                                       target_phys_addr_t addr, ...)
>> +{
>> +    DeviceState *dev;
>> +    SysBusDevice *s;
>> +    va_list va;
>> +    qemu_irq irq;
>> +    int n;
>> +
>> +    dev = qdev_try_create(NULL, name);
>> +    if (!dev) {
>> +        return NULL;
>> +    }
>> +    s = sysbus_from_qdev(dev);
>> +    qdev_init_nofail(dev);
>> +    if (addr != (target_phys_addr_t)-1) {
>> +        sysbus_mmio_map(s, 0, addr);
>> +    }
>> +    va_start(va, addr);
>> +    n = 0;
>> +    while (1) {
>> +        irq = va_arg(va, qemu_irq);
>> +        if (!irq) {
>> +            break;
>> +        }
>> +        sysbus_connect_irq(s, n, irq);
>> +        n++;
>> +    }
>> +    return dev;
>> +}
>> +
>
> I really don't like duplicating so much of sysbus_create_varargs()
> here.  Could it be implemented on top of your
> sysbus_try_create_varargs()?

I didn't like this much either, but then there would need to be
another function which takes va_list as a parameter and also error
handling is not so nice.

> Aside: I also don't like the use of variadic arguments for passing IRQ
> numbers.  It's not type-safe.  Why not pass an array and be done with
> it?

Or why not just use the separate property handling functions like the rest?

Patch

diff --git a/hw/sysbus.c b/hw/sysbus.c
index 1583bd8..8980f34 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -173,6 +173,37 @@  DeviceState *sysbus_create_varargs(const char *name,
     return dev;
 }

+DeviceState *sysbus_try_create_varargs(const char *name,
+                                       target_phys_addr_t addr, ...)
+{
+    DeviceState *dev;
+    SysBusDevice *s;
+    va_list va;
+    qemu_irq irq;
+    int n;
+
+    dev = qdev_try_create(NULL, name);
+    if (!dev) {
+        return NULL;
+    }
+    s = sysbus_from_qdev(dev);
+    qdev_init_nofail(dev);
+    if (addr != (target_phys_addr_t)-1) {
+        sysbus_mmio_map(s, 0, addr);
+    }
+    va_start(va, addr);
+    n = 0;
+    while (1) {
+        irq = va_arg(va, qemu_irq);
+        if (!irq) {
+            break;
+        }
+        sysbus_connect_irq(s, n, irq);
+        n++;
+    }
+    return dev;
+}
+
 static void sysbus_dev_print(Monitor *mon, DeviceState *dev, int indent)
 {
     SysBusDevice *s = sysbus_from_qdev(dev);
diff --git a/hw/sysbus.h b/hw/sysbus.h
index e9eb618..4e8cb16 100644
--- a/hw/sysbus.h
+++ b/hw/sysbus.h
@@ -57,6 +57,8 @@  void sysbus_mmio_map(SysBusDevice *dev, int n,
target_phys_addr_t addr);
 /* Legacy helper function for creating devices.  */
 DeviceState *sysbus_create_varargs(const char *name,
                                  target_phys_addr_t addr, ...);
+DeviceState *sysbus_try_create_varargs(const char *name,
+                                       target_phys_addr_t addr, ...);
 static inline DeviceState *sysbus_create_simple(const char *name,
                                               target_phys_addr_t addr,