[Qemu-devel] [PATCH 0/6] Clean up use of qdev_init()

[Qemu-devel] [PATCH 0/6] Clean up use of qdev_init()

[Markus Armbruster]

qdev_init() can fail since commit 81a322d4.  Most callers don't bother
to check.  This is less serious than it sounds, because they typically
work with devices for which qdev_init() never fails.  It's still
unclean.

The last patch makes the compiler warn when the value of qdev_init()
isn't used.  If that warning triggers after merging this series, more
unchecked calls have crept in since the base of this series (commit
2637c754).  Feel free to ask me for a respin then.

Markus Armbruster (6):
  Move function definitions out of xilinx.h
  Make qdev_init() destroy the device on failure
  Check return value of qdev_init()
  New qdev_init_nofail()
  Make isa_create() terminate program on failure
  Warn if value of qdev_init() isn't checked

 hw/apb_pci.c        |    2 +-
 hw/arm_sysctl.c     |    2 +-
 hw/armv7m.c         |    6 ++--
 hw/axis_dev88.c     |    2 +-
 hw/escc.c           |    4 +-
 hw/esp.c            |    2 +-
 hw/etraxfs.c        |    2 +-
 hw/fdc.c            |    4 +-
 hw/grackle_pci.c    |    2 +-
 hw/i2c.c            |    2 +-
 hw/integratorcp.c   |    2 +-
 hw/isa-bus.c        |   11 +++------
 hw/m48t59.c         |    2 +-
 hw/mc146818rtc.c    |    2 +-
 hw/mips_malta.c     |    2 +-
 hw/musicpal.c       |    4 +-
 hw/ne2000-isa.c     |    2 +-
 hw/pc.c             |    2 +-
 hw/pci-hotplug.c    |    4 +-
 hw/pci.c            |    9 ++++---
 hw/piix_pci.c       |    2 +-
 hw/qdev.c           |   26 +++++++++++++++++++---
 hw/qdev.h           |    3 +-
 hw/scsi-bus.c       |    4 ++-
 hw/smc91c111.c      |    2 +-
 hw/ssi.c            |    2 +-
 hw/stellaris.c      |    2 +-
 hw/sun4m.c          |   28 ++++++++++++------------
 hw/sun4u.c          |    4 +-
 hw/syborg.c         |    4 +-
 hw/sysbus.c         |    2 +-
 hw/unin_pci.c       |    2 +-
 hw/usb-bus.c        |    2 +-
 hw/usb-msd.c        |    3 +-
 hw/vga-pci.c        |    2 +-
 hw/xilinx.h         |   58 ++++++++------------------------------------------
 hw/xilinx_ethlite.c |   19 ++++++++++++++++
 hw/xilinx_intc.c    |   14 ++++++++++++
 hw/xilinx_timer.c   |   15 +++++++++++++
 usb-linux.c         |    4 +-
 40 files changed, 148 insertions(+), 118 deletions(-)

[Qemu-devel] [PATCH 1/6] Move function definitions out of xilinx.h

[Markus Armbruster]

xilinx.h defines a couple of static inline functions for creating
devices.  While that's a fair technique for hot functions, device
initialization is about as cold as it gets.  Define them in the device
source files instead, and keep only declarations in the header.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/xilinx.h         |   58 ++++++++------------------------------------------
 hw/xilinx_ethlite.c |   19 ++++++++++++++++
 hw/xilinx_intc.c    |   14 ++++++++++++
 hw/xilinx_timer.c   |   15 +++++++++++++
 4 files changed, 58 insertions(+), 48 deletions(-)

diff --git a/hw/xilinx.h b/hw/xilinx.h
index 070679c..ea91be1 100644
--- a/hw/xilinx.h
+++ b/hw/xilinx.h
@@ -1,50 +1,12 @@
+#ifndef XILINX_H
+#define XILINX_H
 
-/* OPB Interrupt Controller.  */
 qemu_irq *microblaze_pic_init_cpu(CPUState *env);
-
-static inline DeviceState *
-xilinx_intc_create(target_phys_addr_t base, qemu_irq irq, int kind_of_intr)
-{
-    DeviceState *dev;
-
-    dev = qdev_create(NULL, "xilinx,intc");
-    qdev_prop_set_uint32(dev, "kind-of-intr", kind_of_intr);
-    qdev_init(dev);
-    sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
-    sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq);
-    return dev;
-}
-
-/* OPB Timer/Counter.  */
-static inline DeviceState *
-xilinx_timer_create(target_phys_addr_t base, qemu_irq irq, int nr, int freq)
-{
-    DeviceState *dev;
-
-    dev = qdev_create(NULL, "xilinx,timer");
-    qdev_prop_set_uint32(dev, "nr-timers", nr);
-    qdev_prop_set_uint32(dev, "frequency", freq);
-    qdev_init(dev);
-    sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
-    sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq);
-    return dev;
-}
-
-/* XPS Ethernet Lite MAC.  */
-static inline DeviceState *
-xilinx_ethlite_create(NICInfo *nd, target_phys_addr_t base, qemu_irq irq,
-                      int txpingpong, int rxpingpong)
-{
-    DeviceState *dev;
-
-    qemu_check_nic_model(nd, "xilinx-ethlite");
-
-    dev = qdev_create(NULL, "xilinx,ethlite");
-    dev->nd = nd;
-    qdev_prop_set_uint32(dev, "txpingpong", txpingpong);
-    qdev_prop_set_uint32(dev, "rxpingpong", rxpingpong);
-    qdev_init(dev);
-    sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
-    sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq);
-    return dev;
-}
+DeviceState *xilinx_intc_create(target_phys_addr_t base, qemu_irq irq,
+                                int kind_of_intr);
+DeviceState *xilinx_timer_create(target_phys_addr_t base, qemu_irq irq,
+                                 int nr, int freq);
+DeviceState *xilinx_ethlite_create(NICInfo *nd, target_phys_addr_t base,
+        qemu_irq irq, int txpingpong, int rxpingpong);
+
+#endif
diff --git a/hw/xilinx_ethlite.c b/hw/xilinx_ethlite.c
index 9b0074c..1710149 100644
--- a/hw/xilinx_ethlite.c
+++ b/hw/xilinx_ethlite.c
@@ -25,6 +25,7 @@
 #include "sysbus.h"
 #include "hw.h"
 #include "net.h"
+#include "xilinx.h"
 
 #define D(x)
 #define R_TX_BUF0     0
@@ -201,6 +202,24 @@ static ssize_t eth_rx(VLANClientState *vc, const uint8_t *buf, size_t size)
     return size;
 }
 
+DeviceState *
+xilinx_ethlite_create(NICInfo *nd, target_phys_addr_t base, qemu_irq irq,
+                      int txpingpong, int rxpingpong)
+{
+    DeviceState *dev;
+
+    qemu_check_nic_model(nd, "xilinx-ethlite");
+
+    dev = qdev_create(NULL, "xilinx,ethlite");
+    dev->nd = nd;
+    qdev_prop_set_uint32(dev, "txpingpong", txpingpong);
+    qdev_prop_set_uint32(dev, "rxpingpong", rxpingpong);
+    qdev_init(dev);
+    sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
+    sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq);
+    return dev;
+}
+
 static void eth_cleanup(VLANClientState *vc)
 {
     struct xlx_ethlite *s = vc->opaque;
diff --git a/hw/xilinx_intc.c b/hw/xilinx_intc.c
index 8ef6474..dfb8442 100644
--- a/hw/xilinx_intc.c
+++ b/hw/xilinx_intc.c
@@ -24,6 +24,7 @@
 
 #include "sysbus.h"
 #include "hw.h"
+#include "xilinx.h"
 
 #define D(x)
 
@@ -145,6 +146,19 @@ static void irq_handler(void *opaque, int irq, int level)
     update_irq(p);
 }
 
+DeviceState *
+xilinx_intc_create(target_phys_addr_t base, qemu_irq irq, int kind_of_intr)
+{
+    DeviceState *dev;
+
+    dev = qdev_create(NULL, "xilinx,intc");
+    qdev_prop_set_uint32(dev, "kind-of-intr", kind_of_intr);
+    qdev_init(dev);
+    sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
+    sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq);
+    return dev;
+}
+
 static int xilinx_intc_init(SysBusDevice *dev)
 {
     struct xlx_pic *p = FROM_SYSBUS(typeof (*p), dev);
diff --git a/hw/xilinx_timer.c b/hw/xilinx_timer.c
index e2d9541..26df4b0 100644
--- a/hw/xilinx_timer.c
+++ b/hw/xilinx_timer.c
@@ -25,6 +25,7 @@
 #include "sysbus.h"
 #include "sysemu.h"
 #include "qemu-timer.h"
+#include "xilinx.h"
 
 #define D(x)
 
@@ -189,6 +190,20 @@ static void timer_hit(void *opaque)
     timer_update_irq(t);
 }
 
+DeviceState *
+xilinx_timer_create(target_phys_addr_t base, qemu_irq irq, int nr, int freq)
+{
+    DeviceState *dev;
+
+    dev = qdev_create(NULL, "xilinx,timer");
+    qdev_prop_set_uint32(dev, "nr-timers", nr);
+    qdev_prop_set_uint32(dev, "frequency", freq);
+    qdev_init(dev);
+    sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
+    sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq);
+    return dev;
+}
+
 static int xilinx_timer_init(SysBusDevice *dev)
 {
     struct timerblock *t = FROM_SYSBUS(typeof (*t), dev);
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 1/6] Move function definitions out of xilinx.h

[Blue Swirl]

On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster <armbru@redhat.com> wrote:
> xilinx.h defines a couple of static inline functions for creating
> devices.  While that's a fair technique for hot functions, device
> initialization is about as cold as it gets.  Define them in the device
> source files instead, and keep only declarations in the header.

If I understood the qdev plan correctly, this is going to wrong
direction. These functions should reside near the instantiation, not
in the device code. The current approach looks OK if there are going
to be more users of the devices.

Re: [Qemu-devel] [PATCH 1/6] Move function definitions out of xilinx.h

[Edgar E. Iglesias]

On Sat, Sep 12, 2009 at 09:04:17AM +0300, Blue Swirl wrote:
> On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster <armbru@redhat.com> wrote:
> > xilinx.h defines a couple of static inline functions for creating
> > devices.  While that's a fair technique for hot functions, device
> > initialization is about as cold as it gets.  Define them in the device
> > source files instead, and keep only declarations in the header.
> 
> If I understood the qdev plan correctly, this is going to wrong
> direction. These functions should reside near the instantiation, not
> in the device code. The current approach looks OK if there are going
> to be more users of the devices.

I agree that they shouldn't be in the device source.
The reason they ended up in a header and not with the petalogix board
was that in my tree there are multiple boards using these functions
to easy instantiate devices.

Cheers

Re: [Qemu-devel] [PATCH 1/6] Move function definitions out of xilinx.h

[Gerd Hoffmann]

On 09/12/09 12:10, Edgar E. Iglesias wrote:
> On Sat, Sep 12, 2009 at 09:04:17AM +0300, Blue Swirl wrote:
>> On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster<armbru@redhat.com>  wrote:
>>> xilinx.h defines a couple of static inline functions for creating
>>> devices.  While that's a fair technique for hot functions, device
>>> initialization is about as cold as it gets.  Define them in the device
>>> source files instead, and keep only declarations in the header.
>>
>> If I understood the qdev plan correctly, this is going to wrong
>> direction. These functions should reside near the instantiation, not
>> in the device code. The current approach looks OK if there are going
>> to be more users of the devices.

The functions should go away ;)

Some day the information carried by those code snippeds should come from 
a machine description file, then we'll don't need them any more.

> I agree that they shouldn't be in the device source.
> The reason they ended up in a header and not with the petalogix board
> was that in my tree there are multiple boards using these functions
> to easy instantiate devices.

They have to be somewhere.  Having them in a header file is unclean. 
Having them in the board-specific code isn't practical when multiple 
boards share the code.  I'd stick them to the device source code as 
well.  Also note that this is common practice elsewhere in the tree.

cheers
   Gerd

Re: [Qemu-devel] [PATCH 1/6] Move function definitions out of xilinx.h

[Edgar E. Iglesias]

On Mon, Sep 14, 2009 at 10:57:25AM +0200, Gerd Hoffmann wrote:
> On 09/12/09 12:10, Edgar E. Iglesias wrote:
>> On Sat, Sep 12, 2009 at 09:04:17AM +0300, Blue Swirl wrote:
>>> On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster<armbru@redhat.com>  
>>> wrote:
>>>> xilinx.h defines a couple of static inline functions for creating
>>>> devices.  While that's a fair technique for hot functions, device
>>>> initialization is about as cold as it gets.  Define them in the device
>>>> source files instead, and keep only declarations in the header.
>>>
>>> If I understood the qdev plan correctly, this is going to wrong
>>> direction. These functions should reside near the instantiation, not
>>> in the device code. The current approach looks OK if there are going
>>> to be more users of the devices.
>
> The functions should go away ;)
>
> Some day the information carried by those code snippeds should come from a 
> machine description file, then we'll don't need them any more.

I agree.

>
>> I agree that they shouldn't be in the device source.
>> The reason they ended up in a header and not with the petalogix board
>> was that in my tree there are multiple boards using these functions
>> to easy instantiate devices.
>
> They have to be somewhere.  Having them in a header file is unclean. Having 
> them in the board-specific code isn't practical when multiple boards share 
> the code.  I'd stick them to the device source code as well.  Also note 
> that this is common practice elsewhere in the tree.

I disagree.

But if ppl feel very strongly about this, I can remove them and deal with
the code duplication in my tree. Afterall, it's unlikely that upsteam
qemu gets more xilinx boards before some kind of device tree driven
board support is there.

Cheers

Re: [Qemu-devel] [PATCH 1/6] Move function definitions out of xilinx.h

[Blue Swirl]

On Mon, Sep 14, 2009 at 12:15 PM, Edgar E. Iglesias
<edgar.iglesias@gmail.com> wrote:
> On Mon, Sep 14, 2009 at 10:57:25AM +0200, Gerd Hoffmann wrote:
>> On 09/12/09 12:10, Edgar E. Iglesias wrote:
>>> On Sat, Sep 12, 2009 at 09:04:17AM +0300, Blue Swirl wrote:
>>>> On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster<armbru@redhat.com>
>>>> wrote:
>>>>> xilinx.h defines a couple of static inline functions for creating
>>>>> devices.  While that's a fair technique for hot functions, device
>>>>> initialization is about as cold as it gets.  Define them in the device
>>>>> source files instead, and keep only declarations in the header.
>>>>
>>>> If I understood the qdev plan correctly, this is going to wrong
>>>> direction. These functions should reside near the instantiation, not
>>>> in the device code. The current approach looks OK if there are going
>>>> to be more users of the devices.
>>
>> The functions should go away ;)
>>
>> Some day the information carried by those code snippeds should come from a
>> machine description file, then we'll don't need them any more.
>
> I agree.
>
>>
>>> I agree that they shouldn't be in the device source.
>>> The reason they ended up in a header and not with the petalogix board
>>> was that in my tree there are multiple boards using these functions
>>> to easy instantiate devices.
>>
>> They have to be somewhere.  Having them in a header file is unclean. Having
>> them in the board-specific code isn't practical when multiple boards share
>> the code.  I'd stick them to the device source code as well.  Also note
>> that this is common practice elsewhere in the tree.
>
> I disagree.
>
> But if ppl feel very strongly about this, I can remove them and deal with
> the code duplication in my tree. Afterall, it's unlikely that upsteam
> qemu gets more xilinx boards before some kind of device tree driven
> board support is there.

I also disagree. Why would the functions in a header file be unclean?
The common practice is wrong.

Re: [Qemu-devel] [PATCH 1/6] Move function definitions out of xilinx.h

[Markus Armbruster]

Blue Swirl <blauwirbel@gmail.com> writes:

> On Mon, Sep 14, 2009 at 12:15 PM, Edgar E. Iglesias
> <edgar.iglesias@gmail.com> wrote:
>> On Mon, Sep 14, 2009 at 10:57:25AM +0200, Gerd Hoffmann wrote:
>>> On 09/12/09 12:10, Edgar E. Iglesias wrote:
>>>> On Sat, Sep 12, 2009 at 09:04:17AM +0300, Blue Swirl wrote:
>>>>> On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster<armbru@redhat.com>
>>>>> wrote:
>>>>>> xilinx.h defines a couple of static inline functions for creating
>>>>>> devices.  While that's a fair technique for hot functions, device
>>>>>> initialization is about as cold as it gets.  Define them in the device
>>>>>> source files instead, and keep only declarations in the header.
>>>>>
>>>>> If I understood the qdev plan correctly, this is going to wrong
>>>>> direction. These functions should reside near the instantiation, not
>>>>> in the device code. The current approach looks OK if there are going
>>>>> to be more users of the devices.
>>>
>>> The functions should go away ;)
>>>
>>> Some day the information carried by those code snippeds should come from a
>>> machine description file, then we'll don't need them any more.
>>
>> I agree.
>>
>>>
>>>> I agree that they shouldn't be in the device source.
>>>> The reason they ended up in a header and not with the petalogix board
>>>> was that in my tree there are multiple boards using these functions
>>>> to easy instantiate devices.
>>>
>>> They have to be somewhere.  Having them in a header file is unclean. Having
>>> them in the board-specific code isn't practical when multiple boards share
>>> the code.  I'd stick them to the device source code as well.  Also note
>>> that this is common practice elsewhere in the tree.
>>
>> I disagree.
>>
>> But if ppl feel very strongly about this, I can remove them and deal with
>> the code duplication in my tree. Afterall, it's unlikely that upsteam
>> qemu gets more xilinx boards before some kind of device tree driven
>> board support is there.
>
> I also disagree. Why would the functions in a header file be unclean?
> The common practice is wrong.

The common practice is to put code in .c files, and stick to
declarations needed by several .c files in .h files.  It's okay to
deviate from that common practice if there's a good reason --- we do
that in places.  However, in this case, I couldn't see a reason, so I
moved the code from the header to the place where such code lives for
pretty much every other qdevified device: the device code.

Doing it the same way everywhere means that maintainers have an easier
time to predict where stuff is, how it's named, and so forth.  Don't
disappoint reasonable maintainer expectations without need.

I don't want to make a big deal out of this.  If the code must remain in
the header, expectations be damned, I'll respin the series and move on.

Re: [Qemu-devel] [PATCH 1/6] Move function definitions out of xilinx.h

[Blue Swirl]

On Mon, Sep 14, 2009 at 9:59 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Blue Swirl <blauwirbel@gmail.com> writes:
>
>> On Mon, Sep 14, 2009 at 12:15 PM, Edgar E. Iglesias
>> <edgar.iglesias@gmail.com> wrote:
>>> On Mon, Sep 14, 2009 at 10:57:25AM +0200, Gerd Hoffmann wrote:
>>>> On 09/12/09 12:10, Edgar E. Iglesias wrote:
>>>>> On Sat, Sep 12, 2009 at 09:04:17AM +0300, Blue Swirl wrote:
>>>>>> On Fri, Sep 11, 2009 at 11:19 PM, Markus Armbruster<armbru@redhat.com>
>>>>>> wrote:
>>>>>>> xilinx.h defines a couple of static inline functions for creating
>>>>>>> devices.  While that's a fair technique for hot functions, device
>>>>>>> initialization is about as cold as it gets.  Define them in the device
>>>>>>> source files instead, and keep only declarations in the header.
>>>>>>
>>>>>> If I understood the qdev plan correctly, this is going to wrong
>>>>>> direction. These functions should reside near the instantiation, not
>>>>>> in the device code. The current approach looks OK if there are going
>>>>>> to be more users of the devices.
>>>>
>>>> The functions should go away ;)
>>>>
>>>> Some day the information carried by those code snippeds should come from a
>>>> machine description file, then we'll don't need them any more.
>>>
>>> I agree.
>>>
>>>>
>>>>> I agree that they shouldn't be in the device source.
>>>>> The reason they ended up in a header and not with the petalogix board
>>>>> was that in my tree there are multiple boards using these functions
>>>>> to easy instantiate devices.
>>>>
>>>> They have to be somewhere.  Having them in a header file is unclean. Having
>>>> them in the board-specific code isn't practical when multiple boards share
>>>> the code.  I'd stick them to the device source code as well.  Also note
>>>> that this is common practice elsewhere in the tree.
>>>
>>> I disagree.
>>>
>>> But if ppl feel very strongly about this, I can remove them and deal with
>>> the code duplication in my tree. Afterall, it's unlikely that upsteam
>>> qemu gets more xilinx boards before some kind of device tree driven
>>> board support is there.
>>
>> I also disagree. Why would the functions in a header file be unclean?
>> The common practice is wrong.
>
> The common practice is to put code in .c files, and stick to
> declarations needed by several .c files in .h files.  It's okay to
> deviate from that common practice if there's a good reason --- we do
> that in places.  However, in this case, I couldn't see a reason, so I
> moved the code from the header to the place where such code lives for
> pretty much every other qdevified device: the device code.

Understandable, since we don't see the source code of the other xilinx boards.

> Doing it the same way everywhere means that maintainers have an easier
> time to predict where stuff is, how it's named, and so forth.  Don't
> disappoint reasonable maintainer expectations without need.

It's still wrong, however disappointing that may be. But with enough
cleanups and guidance, the expectations will be directed to right
path.

> I don't want to make a big deal out of this.  If the code must remain in
> the header, expectations be damned, I'll respin the series and move on.

Pushing the device instantiation code higher makes the device code a
bit simpler. Also the internal state can be kept private, provided
that the interface is designed properly. For example, slavio_timer.c
does not export internal state and all functions are static. The
former init function is now in sun4m.c.

Actually, while I'm quite happy with the device level design (and I
think other targets should also be worked to this direction, not the
opposite), the next level design is not so clear since we don't have
the machine config or something.

[Qemu-devel] [PATCH 2/6] Make qdev_init() destroy the device on failure

[Markus Armbruster]

Before, every caller had to do this.  Only two actually did.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/isa-bus.c |    1 -
 hw/qdev.c    |   11 +++++++----
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index 4ecc0f8..fb90be4 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -127,7 +127,6 @@ ISADevice *isa_create_simple(const char *name)
 
     dev = isa_create(name);
     if (qdev_init(&dev->qdev) != 0) {
-        qdev_free(&dev->qdev);
         return NULL;
     }
     return dev;
diff --git a/hw/qdev.c b/hw/qdev.c
index 6451b8a..107acd8 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -202,8 +202,7 @@ DeviceState *qdev_device_add(QemuOpts *opts)
         qdev_free(qdev);
         return NULL;
     }
-    if (qdev_init(qdev) != 0) {
-        qdev_free(qdev);
+    if (qdev_init(qdev) < 0) {
         return NULL;
     }
     return qdev;
@@ -211,14 +210,18 @@ DeviceState *qdev_device_add(QemuOpts *opts)
 
 /* Initialize a device.  Device properties should be set before calling
    this function.  IRQs and MMIO regions should be connected/mapped after
-   calling this function.  */
+   calling this function.
+   On failure, destroy the device and return negative value.
+   Return 0 on success.  */
 int qdev_init(DeviceState *dev)
 {
     int rc;
 
     rc = dev->info->init(dev, dev->info);
-    if (rc < 0)
+    if (rc < 0) {
+        qdev_free(dev);
         return rc;
+    }
     if (dev->info->reset)
         qemu_register_reset(dev->info->reset, dev);
     if (dev->info->vmsd)
-- 
1.6.2.5

[Qemu-devel] [PATCH 3/6] Check return value of qdev_init()

[Markus Armbruster]

But do so only where it may actually fail.  Leave the rest for the
next commit.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/pci-hotplug.c |    4 ++--
 hw/pci.c         |    4 +++-
 hw/scsi-bus.c    |    4 +++-
 hw/usb-msd.c     |    3 ++-
 4 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/pci-hotplug.c b/hw/pci-hotplug.c
index 5348dd1..1f2dff9 100644
--- a/hw/pci-hotplug.c
+++ b/hw/pci-hotplug.c
@@ -149,8 +149,8 @@ static PCIDevice *qemu_pci_hot_add_storage(Monitor *mon,
     default:
         dev = NULL;
     }
-    if (dev)
-        qdev_init(&dev->qdev);
+    if (!dev || qdev_init(&dev->qdev) < 0)
+        return NULL;
     return dev;
 }
 
diff --git a/hw/pci.c b/hw/pci.c
index c12b0be..8dc8864 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -807,6 +807,7 @@ static const char * const pci_nic_names[] = {
 };
 
 /* Initialize a PCI NIC.  */
+/* FIXME callers should check for failure, but don't */
 PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
                         const char *default_devaddr)
 {
@@ -824,7 +825,8 @@ PCIDevice *pci_nic_init(NICInfo *nd, const char *default_model,
             if (nd->id)
                 dev->id = qemu_strdup(nd->id);
             dev->nd = nd;
-            qdev_init(dev);
+            if (qdev_init(dev) < 0)
+                return NULL;
             nd->private = dev;
             return pci_dev;
         }
diff --git a/hw/scsi-bus.c b/hw/scsi-bus.c
index 16afa05..d751f6a 100644
--- a/hw/scsi-bus.c
+++ b/hw/scsi-bus.c
@@ -65,6 +65,7 @@ void scsi_qdev_register(SCSIDeviceInfo *info)
 }
 
 /* handle legacy '-drive if=scsi,...' cmd line args */
+/* FIXME callers should check for failure, but don't */
 SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, DriveInfo *dinfo, int unit)
 {
     const char *driver;
@@ -74,7 +75,8 @@ SCSIDevice *scsi_bus_legacy_add_drive(SCSIBus *bus, DriveInfo *dinfo, int unit)
     dev = qdev_create(&bus->qbus, driver);
     qdev_prop_set_uint32(dev, "scsi-id", unit);
     qdev_prop_set_drive(dev, "drive", dinfo);
-    qdev_init(dev);
+    if (qdev_init(dev) < 0)
+        return NULL;
     return DO_UPCAST(SCSIDevice, qdev, dev);
 }
 
diff --git a/hw/usb-msd.c b/hw/usb-msd.c
index aa0ce6a..0c7ad5c 100644
--- a/hw/usb-msd.c
+++ b/hw/usb-msd.c
@@ -579,7 +579,8 @@ USBDevice *usb_msd_init(const char *filename)
     /* create guest device */
     dev = usb_create(NULL /* FIXME */, "QEMU USB MSD");
     qdev_prop_set_drive(&dev->qdev, "drive", dinfo);
-    qdev_init(&dev->qdev);
+    if (qdev_init(&dev->qdev) < 0)
+        return NULL;
 
     return dev;
 }
-- 
1.6.2.5

[Qemu-devel] [PATCH 4/6] New qdev_init_nofail()

[Markus Armbruster]

Like qdev_init(), but terminate program via hw_error() instead of
returning an error value.

Use it instead of qdev_init() where terminating the program on failure
is okay, either because it's during machine construction, or because
we know that failure can't happen.

Because relying in the latter is somewhat unclean, and the former is
not always obvious, it would be nice to go back to qdev_init() in the
not-so-obvious cases, only with proper error handling.  I'm leaving
that for another day, because it involves making sure that error
values are properly checked by all callers.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/apb_pci.c        |    2 +-
 hw/arm_sysctl.c     |    2 +-
 hw/armv7m.c         |    6 +++---
 hw/axis_dev88.c     |    2 +-
 hw/escc.c           |    4 ++--
 hw/esp.c            |    2 +-
 hw/etraxfs.c        |    2 +-
 hw/fdc.c            |    4 ++--
 hw/grackle_pci.c    |    2 +-
 hw/i2c.c            |    2 +-
 hw/integratorcp.c   |    2 +-
 hw/isa-bus.c        |    2 +-
 hw/m48t59.c         |    2 +-
 hw/mc146818rtc.c    |    2 +-
 hw/mips_malta.c     |    2 +-
 hw/musicpal.c       |    4 ++--
 hw/ne2000-isa.c     |    2 +-
 hw/pc.c             |    2 +-
 hw/pci.c            |    5 ++---
 hw/piix_pci.c       |    2 +-
 hw/qdev.c           |   15 +++++++++++++++
 hw/qdev.h           |    1 +
 hw/smc91c111.c      |    2 +-
 hw/ssi.c            |    2 +-
 hw/stellaris.c      |    2 +-
 hw/sun4m.c          |   28 ++++++++++++++--------------
 hw/sun4u.c          |    4 ++--
 hw/syborg.c         |    4 ++--
 hw/sysbus.c         |    2 +-
 hw/unin_pci.c       |    2 +-
 hw/usb-bus.c        |    2 +-
 hw/vga-pci.c        |    2 +-
 hw/xilinx_ethlite.c |    2 +-
 hw/xilinx_intc.c    |    2 +-
 hw/xilinx_timer.c   |    2 +-
 usb-linux.c         |    4 ++--
 36 files changed, 72 insertions(+), 57 deletions(-)

diff --git a/hw/apb_pci.c b/hw/apb_pci.c
index f1088aa..bd9b571 100644
--- a/hw/apb_pci.c
+++ b/hw/apb_pci.c
@@ -236,7 +236,7 @@ PCIBus *pci_apb_init(target_phys_addr_t special_base,
 
     /* Ultrasparc PBM main bus */
     dev = qdev_create(NULL, "pbm");
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     /* apb_config */
     sysbus_mmio_map(s, 0, special_base + 0x2000ULL);
diff --git a/hw/arm_sysctl.c b/hw/arm_sysctl.c
index 686c1cc..2d96faf 100644
--- a/hw/arm_sysctl.c
+++ b/hw/arm_sysctl.c
@@ -211,7 +211,7 @@ void arm_sysctl_init(uint32_t base, uint32_t sys_id)
 
     dev = qdev_create(NULL, "realview_sysctl");
     qdev_prop_set_uint32(dev, "sys_id", sys_id);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
 }
 
diff --git a/hw/armv7m.c b/hw/armv7m.c
index 059a356..96eacec 100644
--- a/hw/armv7m.c
+++ b/hw/armv7m.c
@@ -139,12 +139,12 @@ static void armv7m_bitband_init(void)
 
     dev = qdev_create(NULL, "ARM,bitband-memory");
     qdev_prop_set_uint32(dev, "base", 0x20000000);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     sysbus_mmio_map(sysbus_from_qdev(dev), 0, 0x22000000);
 
     dev = qdev_create(NULL, "ARM,bitband-memory");
     qdev_prop_set_uint32(dev, "base", 0x40000000);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     sysbus_mmio_map(sysbus_from_qdev(dev), 0, 0x42000000);
 }
 
@@ -199,7 +199,7 @@ qemu_irq *armv7m_init(int flash_size, int sram_size,
 
     nvic = qdev_create(NULL, "armv7m_nvic");
     env->v7m.nvic = nvic;
-    qdev_init(nvic);
+    qdev_init_nofail(nvic);
     cpu_pic = arm_pic_init_cpu(env);
     sysbus_connect_irq(sysbus_from_qdev(nvic), 0, cpu_pic[ARM_PIC_CPU_IRQ]);
     for (i = 0; i < 64; i++) {
diff --git a/hw/axis_dev88.c b/hw/axis_dev88.c
index b5163b6..c549916 100644
--- a/hw/axis_dev88.c
+++ b/hw/axis_dev88.c
@@ -298,7 +298,7 @@ void axisdev88_init (ram_addr_t ram_size,
     dev = qdev_create(NULL, "etraxfs,pic");
     /* FIXME: Is there a proper way to signal vectors to the CPU core?  */
     qdev_prop_set_ptr(dev, "interrupt_vector", &env->interrupt_vector);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     sysbus_mmio_map(s, 0, 0x3001c000);
     sysbus_connect_irq(s, 0, cpu_irq[0]);
diff --git a/hw/escc.c b/hw/escc.c
index 491c4cf..ed7da33 100644
--- a/hw/escc.c
+++ b/hw/escc.c
@@ -741,7 +741,7 @@ int escc_init(target_phys_addr_t base, qemu_irq irqA, qemu_irq irqB,
     qdev_prop_set_chr(dev, "chrA", chrA);
     qdev_prop_set_uint32(dev, "chnBtype", ser);
     qdev_prop_set_uint32(dev, "chnAtype", ser);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     sysbus_connect_irq(s, 0, irqA);
     sysbus_connect_irq(s, 1, irqB);
@@ -904,7 +904,7 @@ void slavio_serial_ms_kbd_init(target_phys_addr_t base, qemu_irq irq,
     qdev_prop_set_chr(dev, "chrA", NULL);
     qdev_prop_set_uint32(dev, "chnBtype", mouse);
     qdev_prop_set_uint32(dev, "chnAtype", kbd);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     sysbus_connect_irq(s, 0, irq);
     sysbus_connect_irq(s, 1, irq);
diff --git a/hw/esp.c b/hw/esp.c
index b5ddfb2..d3175bc 100644
--- a/hw/esp.c
+++ b/hw/esp.c
@@ -667,7 +667,7 @@ void esp_init(target_phys_addr_t espaddr, int it_shift,
     esp->dma_memory_write = dma_memory_write;
     esp->dma_opaque = dma_opaque;
     esp->it_shift = it_shift;
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     sysbus_connect_irq(s, 0, irq);
     sysbus_mmio_map(s, 0, espaddr);
diff --git a/hw/etraxfs.c b/hw/etraxfs.c
index ab6a3a3..3d14264 100644
--- a/hw/etraxfs.c
+++ b/hw/etraxfs.c
@@ -90,7 +90,7 @@ void bareetraxfs_init (ram_addr_t ram_size,
     dev = qdev_create(NULL, "etraxfs,pic");
     /* FIXME: Is there a proper way to signal vectors to the CPU core?  */
     qdev_prop_set_ptr(dev, "interrupt_vector", &env->interrupt_vector);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     sysbus_mmio_map(s, 0, 0x3001c000);
     sysbus_connect_irq(s, 0, cpu_irq[0]);
diff --git a/hw/fdc.c b/hw/fdc.c
index 389d9e6..f0742ce 100644
--- a/hw/fdc.c
+++ b/hw/fdc.c
@@ -1865,7 +1865,7 @@ fdctrl_t *fdctrl_init_sysbus(qemu_irq irq, int dma_chann,
     fdctrl_sysbus_t *sys;
 
     dev = qdev_create(NULL, "sysbus-fdc");
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     sys = DO_UPCAST(fdctrl_sysbus_t, busdev.qdev, dev);
     fdctrl = &sys->state;
     sysbus_connect_irq(&sys->busdev, 0, irq);
@@ -1886,7 +1886,7 @@ fdctrl_t *sun4m_fdctrl_init (qemu_irq irq, target_phys_addr_t io_base,
     fdctrl_t *fdctrl;
 
     dev = qdev_create(NULL, "SUNW,fdtwo");
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     sys = DO_UPCAST(fdctrl_sysbus_t, busdev.qdev, dev);
     fdctrl = &sys->state;
     sysbus_connect_irq(&sys->busdev, 0, irq);
diff --git a/hw/grackle_pci.c b/hw/grackle_pci.c
index 177c888..ce237cf 100644
--- a/hw/grackle_pci.c
+++ b/hw/grackle_pci.c
@@ -138,7 +138,7 @@ PCIBus *pci_grackle_init(uint32_t base, qemu_irq *pic)
     GrackleState *d;
 
     dev = qdev_create(NULL, "grackle");
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     d = FROM_SYSBUS(GrackleState, s);
     d->host_state.bus = pci_register_bus(NULL, "pci",
diff --git a/hw/i2c.c b/hw/i2c.c
index 5473772..a9f2a50 100644
--- a/hw/i2c.c
+++ b/hw/i2c.c
@@ -173,6 +173,6 @@ DeviceState *i2c_create_slave(i2c_bus *bus, const char *name, int addr)
 
     dev = qdev_create(&bus->qbus, name);
     qdev_prop_set_uint32(dev, "address", addr);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     return dev;
 }
diff --git a/hw/integratorcp.c b/hw/integratorcp.c
index 21e7712..bee8298 100644
--- a/hw/integratorcp.c
+++ b/hw/integratorcp.c
@@ -477,7 +477,7 @@ static void integratorcp_init(ram_addr_t ram_size,
 
     dev = qdev_create(NULL, "integrator_core");
     qdev_prop_set_uint32(dev, "memsz", ram_size >> 20);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     sysbus_mmio_map((SysBusDevice *)dev, 0, 0x10000000);
 
     cpu_pic = arm_pic_init_cpu(env);
diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index fb90be4..f7e73d2 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -45,7 +45,7 @@ ISABus *isa_bus_new(DeviceState *dev)
     }
     if (NULL == dev) {
         dev = qdev_create(NULL, "isabus-bridge");
-        qdev_init(dev);
+        qdev_init_nofail(dev);
     }
 
     isabus = FROM_QBUS(ISABus, qbus_create(&isa_bus_info, dev, NULL));
diff --git a/hw/m48t59.c b/hw/m48t59.c
index 0fcf4f8..28dd719 100644
--- a/hw/m48t59.c
+++ b/hw/m48t59.c
@@ -626,7 +626,7 @@ m48t59_t *m48t59_init (qemu_irq IRQ, target_phys_addr_t mem_base,
     qdev_prop_set_uint32(dev, "type", type);
     qdev_prop_set_uint32(dev, "size", size);
     qdev_prop_set_uint32(dev, "io_base", io_base);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     sysbus_connect_irq(s, 0, IRQ);
     if (io_base != 0) {
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 9d6a627..0c86646 100644
--- a/hw/mc146818rtc.c
+++ b/hw/mc146818rtc.c
@@ -637,7 +637,7 @@ RTCState *rtc_init(int base_year)
 
     dev = isa_create("mc146818rtc");
     qdev_prop_set_int32(&dev->qdev, "base_year", base_year);
-    qdev_init(&dev->qdev);
+    qdev_init_nofail(&dev->qdev);
     return DO_UPCAST(RTCState, dev, dev);
 }
 
diff --git a/hw/mips_malta.c b/hw/mips_malta.c
index 25e32bf..838e41f 100644
--- a/hw/mips_malta.c
+++ b/hw/mips_malta.c
@@ -915,7 +915,7 @@ void mips_malta_init (ram_addr_t ram_size,
         eeprom = qdev_create((BusState *)smbus, "smbus-eeprom");
         qdev_prop_set_uint32(eeprom, "address", 0x50 + i);
         qdev_prop_set_ptr(eeprom, "data", eeprom_buf + (i * 256));
-        qdev_init(eeprom);
+        qdev_init_nofail(eeprom);
     }
     pit = pit_init(0x40, isa_reserve_irq(0));
     DMA_init(0);
diff --git a/hw/musicpal.c b/hw/musicpal.c
index 1c4f17c..04570ac 100644
--- a/hw/musicpal.c
+++ b/hw/musicpal.c
@@ -1357,7 +1357,7 @@ static void musicpal_init(ram_addr_t ram_size,
     qemu_check_nic_model(&nd_table[0], "mv88w8618");
     dev = qdev_create(NULL, "mv88w8618_eth");
     dev->nd = &nd_table[0];
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     sysbus_mmio_map(sysbus_from_qdev(dev), 0, MP_ETH_BASE);
     sysbus_connect_irq(sysbus_from_qdev(dev), 0, pic[MP_ETH_IRQ]);
 
@@ -1390,7 +1390,7 @@ static void musicpal_init(ram_addr_t ram_size,
     dev = qdev_create(NULL, "mv88w8618_audio");
     s = sysbus_from_qdev(dev);
     qdev_prop_set_ptr(dev, "wm8750", wm8750_dev);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     sysbus_mmio_map(s, 0, MP_AUDIO_BASE);
     sysbus_connect_irq(s, 0, pic[MP_AUDIO_IRQ]);
 #endif
diff --git a/hw/ne2000-isa.c b/hw/ne2000-isa.c
index 9d8f7aa..e7147ec 100644
--- a/hw/ne2000-isa.c
+++ b/hw/ne2000-isa.c
@@ -89,7 +89,7 @@ void isa_ne2000_init(int base, int irq, NICInfo *nd)
     dev->qdev.nd = nd; /* hack alert */
     qdev_prop_set_uint32(&dev->qdev, "iobase", base);
     qdev_prop_set_uint32(&dev->qdev, "irq",    irq);
-    qdev_init(&dev->qdev);
+    qdev_init_nofail(&dev->qdev);
 }
 
 static ISADeviceInfo ne2000_isa_info = {
diff --git a/hw/pc.c b/hw/pc.c
index d96d756..33cd5e4 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -1403,7 +1403,7 @@ static void pc_init1(ram_addr_t ram_size,
             eeprom = qdev_create((BusState *)smbus, "smbus-eeprom");
             qdev_prop_set_uint32(eeprom, "address", 0x50 + i);
             qdev_prop_set_ptr(eeprom, "data", eeprom_buf + (i * 256));
-            qdev_init(eeprom);
+            qdev_init_nofail(eeprom);
         }
     }
 
diff --git a/hw/pci.c b/hw/pci.c
index 8dc8864..b9f3ff0 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -931,9 +931,8 @@ PCIDevice *pci_create_simple(PCIBus *bus, int devfn, const char *name)
 
     dev = qdev_create(&bus->qbus, name);
     qdev_prop_set_uint32(dev, "addr", devfn);
-    qdev_init(dev);
-
-    return (PCIDevice *)dev;
+    qdev_init_nofail(dev);
+    return DO_UPCAST(PCIDevice, qdev, dev);
 }
 
 static int pci_find_space(PCIDevice *pdev, uint8_t size)
diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index edd6df0..4fbc259 100644
--- a/hw/piix_pci.c
+++ b/hw/piix_pci.c
@@ -240,7 +240,7 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state, int *piix3_devfn, qemu_irq *
     b = pci_register_bus(&s->busdev.qdev, "pci.0",
                          piix3_set_irq, pci_slot_get_pirq, irq_state, 0, 4);
     s->bus = b;
-    qdev_init(dev);
+    qdev_init_nofail(dev);
 
     d = pci_create_simple(b, 0, "i440FX");
     *pi440fx_state = DO_UPCAST(PCII440FXState, dev, d);
diff --git a/hw/qdev.c b/hw/qdev.c
index 107acd8..e17d493 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -229,6 +229,21 @@ int qdev_init(DeviceState *dev)
     return 0;
 }
 
+/* Like qdev_init(), but terminate program via hw_error() instead of
+   returning an error value.  This is okay during machine creation.
+   Don't use for hotplug, because there callers need to recover from
+   failure.  Exception: if you know the device's init() callback can't
+   fail, then qdev_init_nofail() can't fail either, and is therefore
+   usable even then.  But relying on the device implementation that
+   way is somewhat unclean, and best avoided.  */
+void qdev_init_nofail(DeviceState *dev)
+{
+    DeviceInfo *info = dev->info;
+
+    if (qdev_init(dev) < 0)
+        hw_error("Initialization of device %s failed\n", info->name);
+}
+
 /* Unlink device from bus and free the structure.  */
 void qdev_free(DeviceState *dev)
 {
diff --git a/hw/qdev.h b/hw/qdev.h
index c2609b4..e3245e3 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -90,6 +90,7 @@ struct CompatProperty {
 DeviceState *qdev_create(BusState *bus, const char *name);
 DeviceState *qdev_device_add(QemuOpts *opts);
 int qdev_init(DeviceState *dev);
+void qdev_init_nofail(DeviceState *dev);
 void qdev_free(DeviceState *dev);
 
 qemu_irq qdev_get_gpio_in(DeviceState *dev, int n);
diff --git a/hw/smc91c111.c b/hw/smc91c111.c
index a08bdb0..d58821a 100644
--- a/hw/smc91c111.c
+++ b/hw/smc91c111.c
@@ -735,7 +735,7 @@ void smc91c111_init(NICInfo *nd, uint32_t base, qemu_irq irq)
     qemu_check_nic_model(nd, "smc91c111");
     dev = qdev_create(NULL, "smc91c111");
     dev->nd = nd;
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     sysbus_mmio_map(s, 0, base);
     sysbus_connect_irq(s, 0, irq);
diff --git a/hw/ssi.c b/hw/ssi.c
index 54ad1a1..02e09d3 100644
--- a/hw/ssi.c
+++ b/hw/ssi.c
@@ -46,7 +46,7 @@ DeviceState *ssi_create_slave(SSIBus *bus, const char *name)
 {
     DeviceState *dev;
     dev = qdev_create(&bus->qbus, name);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     return dev;
 }
 
diff --git a/hw/stellaris.c b/hw/stellaris.c
index bcde0a2..1628914 100644
--- a/hw/stellaris.c
+++ b/hw/stellaris.c
@@ -1384,7 +1384,7 @@ static void stellaris_init(const char *kernel_filename, const char *cpu_model,
 
         enet = qdev_create(NULL, "stellaris_enet");
         enet->nd = &nd_table[0];
-        qdev_init(enet);
+        qdev_init_nofail(enet);
         sysbus_mmio_map(sysbus_from_qdev(enet), 0, 0x40048000);
         sysbus_connect_irq(sysbus_from_qdev(enet), 0, pic[42]);
     }
diff --git a/hw/sun4m.c b/hw/sun4m.c
index d970723..5bf7ae1 100644
--- a/hw/sun4m.c
+++ b/hw/sun4m.c
@@ -349,7 +349,7 @@ static void *iommu_init(target_phys_addr_t addr, uint32_t version, qemu_irq irq)
 
     dev = qdev_create(NULL, "iommu");
     qdev_prop_set_uint32(dev, "version", version);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     sysbus_connect_irq(s, 0, irq);
     sysbus_mmio_map(s, 0, addr);
@@ -365,7 +365,7 @@ static void *sparc32_dma_init(target_phys_addr_t daddr, qemu_irq parent_irq,
 
     dev = qdev_create(NULL, "sparc32_dma");
     qdev_prop_set_ptr(dev, "iommu_opaque", iommu);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     sysbus_connect_irq(s, 0, parent_irq);
     *dev_irq = qdev_get_gpio_in(dev, 0);
@@ -386,7 +386,7 @@ static void lance_init(NICInfo *nd, target_phys_addr_t leaddr,
     dev = qdev_create(NULL, "lance");
     dev->nd = nd;
     qdev_prop_set_ptr(dev, "dma", dma_opaque);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     sysbus_mmio_map(s, 0, leaddr);
     sysbus_connect_irq(s, 0, irq);
@@ -403,7 +403,7 @@ static DeviceState *slavio_intctl_init(target_phys_addr_t addr,
     unsigned int i, j;
 
     dev = qdev_create(NULL, "slavio_intctl");
-    qdev_init(dev);
+    qdev_init_nofail(dev);
 
     s = sysbus_from_qdev(dev);
 
@@ -432,7 +432,7 @@ static void slavio_timer_init_all(target_phys_addr_t addr, qemu_irq master_irq,
 
     dev = qdev_create(NULL, "slavio_timer");
     qdev_prop_set_uint32(dev, "num_cpus", num_cpus);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     sysbus_connect_irq(s, 0, master_irq);
     sysbus_mmio_map(s, 0, addr + SYS_TIMER_OFFSET);
@@ -458,7 +458,7 @@ static void slavio_misc_init(target_phys_addr_t base,
     SysBusDevice *s;
 
     dev = qdev_create(NULL, "slavio_misc");
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     if (base) {
         /* 8 bit registers */
@@ -495,7 +495,7 @@ static void ecc_init(target_phys_addr_t base, qemu_irq irq, uint32_t version)
 
     dev = qdev_create(NULL, "eccmemctl");
     qdev_prop_set_uint32(dev, "version", version);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     sysbus_connect_irq(s, 0, irq);
     sysbus_mmio_map(s, 0, base);
@@ -510,7 +510,7 @@ static void apc_init(target_phys_addr_t power_base, qemu_irq cpu_halt)
     SysBusDevice *s;
 
     dev = qdev_create(NULL, "apc");
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     /* Power management (APC) XXX: not a Slavio device */
     sysbus_mmio_map(s, 0, power_base);
@@ -529,7 +529,7 @@ static void tcx_init(target_phys_addr_t addr, int vram_size, int width,
     qdev_prop_set_uint16(dev, "width", width);
     qdev_prop_set_uint16(dev, "height", height);
     qdev_prop_set_uint16(dev, "depth", depth);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     /* 8-bit plane */
     sysbus_mmio_map(s, 0, addr + 0x00800000ULL);
@@ -559,7 +559,7 @@ static void idreg_init(target_phys_addr_t addr)
     SysBusDevice *s;
 
     dev = qdev_create(NULL, "macio_idreg");
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
 
     sysbus_mmio_map(s, 0, addr);
@@ -597,7 +597,7 @@ static void prom_init(target_phys_addr_t addr, const char *bios_name)
     int ret;
 
     dev = qdev_create(NULL, "openprom");
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
 
     sysbus_mmio_map(s, 0, addr);
@@ -686,7 +686,7 @@ static void ram_init(target_phys_addr_t addr, ram_addr_t RAM_size,
 
     d = FROM_SYSBUS(RamDevice, s);
     d->size = RAM_size;
-    qdev_init(dev);
+    qdev_init_nofail(dev);
 
     sysbus_mmio_map(s, 0, addr);
 }
@@ -1337,7 +1337,7 @@ static DeviceState *sbi_init(target_phys_addr_t addr, qemu_irq **parent_irq)
     unsigned int i;
 
     dev = qdev_create(NULL, "sbi");
-    qdev_init(dev);
+    qdev_init_nofail(dev);
 
     s = sysbus_from_qdev(dev);
 
@@ -1527,7 +1527,7 @@ static DeviceState *sun4c_intctl_init(target_phys_addr_t addr,
     unsigned int i;
 
     dev = qdev_create(NULL, "sun4c_intctl");
-    qdev_init(dev);
+    qdev_init_nofail(dev);
 
     s = sysbus_from_qdev(dev);
 
diff --git a/hw/sun4u.c b/hw/sun4u.c
index ffda4cd..9657220 100644
--- a/hw/sun4u.c
+++ b/hw/sun4u.c
@@ -407,7 +407,7 @@ static void prom_init(target_phys_addr_t addr, const char *bios_name)
     int ret;
 
     dev = qdev_create(NULL, "openprom");
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
 
     sysbus_mmio_map(s, 0, addr);
@@ -489,7 +489,7 @@ static void ram_init(target_phys_addr_t addr, ram_addr_t RAM_size)
 
     d = FROM_SYSBUS(RamDevice, s);
     d->size = RAM_size;
-    qdev_init(dev);
+    qdev_init_nofail(dev);
 
     sysbus_mmio_map(s, 0, addr);
 }
diff --git a/hw/syborg.c b/hw/syborg.c
index d8d38d4..2aec769 100644
--- a/hw/syborg.c
+++ b/hw/syborg.c
@@ -65,7 +65,7 @@ static void syborg_init(ram_addr_t ram_size,
 
     dev = qdev_create(NULL, "syborg,timer");
     qdev_prop_set_uint32(dev, "frequency", 1000000);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     sysbus_mmio_map(sysbus_from_qdev(dev), 0, 0xC0002000);
     sysbus_connect_irq(sysbus_from_qdev(dev), 0, pic[1]);
 
@@ -84,7 +84,7 @@ static void syborg_init(ram_addr_t ram_size,
         qemu_check_nic_model(&nd_table[0], "virtio");
         dev = qdev_create(NULL, "syborg,virtio-net");
         dev->nd = &nd_table[0];
-        qdev_init(dev);
+        qdev_init_nofail(dev);
         s = sysbus_from_qdev(dev);
         sysbus_mmio_map(s, 0, 0xc000c000);
         sysbus_connect_irq(s, 0, pic[9]);
diff --git a/hw/sysbus.c b/hw/sysbus.c
index f6516fd..1f7f138 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -143,7 +143,7 @@ DeviceState *sysbus_create_varargs(const char *name,
 
     dev = qdev_create(NULL, name);
     s = sysbus_from_qdev(dev);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     if (addr != (target_phys_addr_t)-1) {
         sysbus_mmio_map(s, 0, addr);
     }
diff --git a/hw/unin_pci.c b/hw/unin_pci.c
index e6d9a70..b1ca936 100644
--- a/hw/unin_pci.c
+++ b/hw/unin_pci.c
@@ -253,7 +253,7 @@ PCIBus *pci_pmac_init(qemu_irq *pic)
     /* Use values found on a real PowerMac */
     /* Uninorth main bus */
     dev = qdev_create(NULL, "Uni-north main");
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     s = sysbus_from_qdev(dev);
     d = FROM_SYSBUS(UNINState, s);
     d->host_state.bus = pci_register_bus(NULL, "pci",
diff --git a/hw/usb-bus.c b/hw/usb-bus.c
index 169fb2f..65ab1c9 100644
--- a/hw/usb-bus.c
+++ b/hw/usb-bus.c
@@ -90,7 +90,7 @@ USBDevice *usb_create(USBBus *bus, const char *name)
 USBDevice *usb_create_simple(USBBus *bus, const char *name)
 {
     USBDevice *dev = usb_create(bus, name);
-    qdev_init(&dev->qdev);
+    qdev_init_nofail(&dev->qdev);
     return dev;
 }
 
diff --git a/hw/vga-pci.c b/hw/vga-pci.c
index 6038cec..cbe48fb 100644
--- a/hw/vga-pci.c
+++ b/hw/vga-pci.c
@@ -140,7 +140,7 @@ int pci_vga_init(PCIBus *bus,
     dev = pci_create("VGA", NULL);
     qdev_prop_set_uint32(&dev->qdev, "bios-offset", vga_bios_offset);
     qdev_prop_set_uint32(&dev->qdev, "bios-size", vga_bios_offset);
-    qdev_init(&dev->qdev);
+    qdev_init_nofail(&dev->qdev);
 
     return 0;
 }
diff --git a/hw/xilinx_ethlite.c b/hw/xilinx_ethlite.c
index 1710149..7bfa3f1 100644
--- a/hw/xilinx_ethlite.c
+++ b/hw/xilinx_ethlite.c
@@ -214,7 +214,7 @@ xilinx_ethlite_create(NICInfo *nd, target_phys_addr_t base, qemu_irq irq,
     dev->nd = nd;
     qdev_prop_set_uint32(dev, "txpingpong", txpingpong);
     qdev_prop_set_uint32(dev, "rxpingpong", rxpingpong);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
     sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq);
     return dev;
diff --git a/hw/xilinx_intc.c b/hw/xilinx_intc.c
index dfb8442..f978ffa 100644
--- a/hw/xilinx_intc.c
+++ b/hw/xilinx_intc.c
@@ -153,7 +153,7 @@ xilinx_intc_create(target_phys_addr_t base, qemu_irq irq, int kind_of_intr)
 
     dev = qdev_create(NULL, "xilinx,intc");
     qdev_prop_set_uint32(dev, "kind-of-intr", kind_of_intr);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
     sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq);
     return dev;
diff --git a/hw/xilinx_timer.c b/hw/xilinx_timer.c
index 26df4b0..db8076e 100644
--- a/hw/xilinx_timer.c
+++ b/hw/xilinx_timer.c
@@ -198,7 +198,7 @@ xilinx_timer_create(target_phys_addr_t base, qemu_irq irq, int nr, int freq)
     dev = qdev_create(NULL, "xilinx,timer");
     qdev_prop_set_uint32(dev, "nr-timers", nr);
     qdev_prop_set_uint32(dev, "frequency", freq);
-    qdev_init(dev);
+    qdev_init_nofail(dev);
     sysbus_mmio_map(sysbus_from_qdev(dev), 0, base);
     sysbus_connect_irq(sysbus_from_qdev(dev), 0, irq);
     return dev;
diff --git a/usb-linux.c b/usb-linux.c
index c80499a..8fa35f4 100644
--- a/usb-linux.c
+++ b/usb-linux.c
@@ -1387,7 +1387,7 @@ static int usb_host_auto_scan(void *opaque, int bus_num, int addr,
 
         /* We got a match */
 
-        /* Allredy attached ? */
+        /* Already attached ? */
         if (hostdev_find(bus_num, addr))
             return 0;
 
@@ -1395,7 +1395,7 @@ static int usb_host_auto_scan(void *opaque, int bus_num, int addr,
 
 	dev = usb_host_device_open_addr(bus_num, addr, product_name);
 	if (dev)
-            qdev_init(&dev->qdev);
+            qdev_init_nofail(&dev->qdev);
     }
 
     return 0;
-- 
1.6.2.5

[Qemu-devel] [PATCH 5/6] Make isa_create() terminate program on failure

[Markus Armbruster]

Callers don't check the return value anyway.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/isa-bus.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/hw/isa-bus.c b/hw/isa-bus.c
index f7e73d2..4d489d2 100644
--- a/hw/isa-bus.c
+++ b/hw/isa-bus.c
@@ -114,8 +114,8 @@ ISADevice *isa_create(const char *name)
     DeviceState *dev;
 
     if (!isabus) {
-        fprintf(stderr, "Tried to create isa device %s with no isa bus present.\n", name);
-        return NULL;
+        hw_error("Tried to create isa device %s with no isa bus present.\n",
+                 name);
     }
     dev = qdev_create(&isabus->qbus, name);
     return DO_UPCAST(ISADevice, qdev, dev);
@@ -126,9 +126,7 @@ ISADevice *isa_create_simple(const char *name)
     ISADevice *dev;
 
     dev = isa_create(name);
-    if (qdev_init(&dev->qdev) != 0) {
-        return NULL;
-    }
+    qdev_init_nofail(&dev->qdev);
     return dev;
 }
 
-- 
1.6.2.5

[Qemu-devel] [PATCH 6/6] Warn if value of qdev_init() isn't checked

[Markus Armbruster]

After qdev_init() fails, the device is gone.  Failure to check runs a
high risk of use-after-free.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/qdev.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/hw/qdev.h b/hw/qdev.h
index e3245e3..3e2d271 100644
--- a/hw/qdev.h
+++ b/hw/qdev.h
@@ -89,7 +89,7 @@ struct CompatProperty {
 
 DeviceState *qdev_create(BusState *bus, const char *name);
 DeviceState *qdev_device_add(QemuOpts *opts);
-int qdev_init(DeviceState *dev);
+int qdev_init(DeviceState *dev) __attribute__((warn_unused_result));
 void qdev_init_nofail(DeviceState *dev);
 void qdev_free(DeviceState *dev);
 
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH 0/6] Clean up use of qdev_init()

[Gerd Hoffmann]

On 09/11/09 22:19, Markus Armbruster wrote:
> qdev_init() can fail since commit 81a322d4.  Most callers don't bother
> to check.  This is less serious than it sounds, because they typically
> work with devices for which qdev_init() never fails.  It's still
> unclean.
>
> The last patch makes the compiler warn when the value of qdev_init()
> isn't used.  If that warning triggers after merging this series, more
> unchecked calls have crept in since the base of this series (commit
> 2637c754).  Feel free to ask me for a respin then.
>
> Markus Armbruster (6):
>    Move function definitions out of xilinx.h
>    Make qdev_init() destroy the device on failure
>    Check return value of qdev_init()
>    New qdev_init_nofail()
>    Make isa_create() terminate program on failure
>    Warn if value of qdev_init() isn't checked

Whole series looks good to me, nice cleanup.

Acked-by: Gerd Hoffmann <kraxel@redhat.com>

cheers,
   Gerd