[Qemu-devel] [PATCH] qdev: Deprecated qdev_init() is finally unused, drop

[Qemu-devel] [PATCH] qdev: Deprecated qdev_init() is finally unused, drop

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
Depends on
[PATCH 3/3] s390x: Replace unchecked qdev_init() by qdev_init_nofail()
[PATCH] macio: Convert to realize()

 hw/core/qdev.c         | 44 +++++++++++++-------------------------------
 include/hw/qdev-core.h |  3 +--
 2 files changed, 14 insertions(+), 33 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 6be5866..dcb344c 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -126,9 +126,9 @@ void qbus_set_bus_hotplug_handler(BusState *bus, Error **errp)
     qbus_set_hotplug_handler_internal(bus, OBJECT(bus), errp);
 }
 
-/* Create a new device.  This only initializes the device state structure
-   and allows properties to be set.  qdev_init should be called to
-   initialize the actual device emulation.  */
+/* Create a new device.  This only initializes the device state
+   structure and allows properties to be set.  The device still needs
+   to be realized.  See qdev-core.h.  */
 DeviceState *qdev_create(BusState *bus, const char *name)
 {
     DeviceState *dev;
@@ -168,27 +168,6 @@ DeviceState *qdev_try_create(BusState *bus, const char *type)
     return dev;
 }
 
-/* Initialize a device.  Device properties should be set before calling
-   this function.  IRQs and MMIO regions should be connected/mapped after
-   calling this function.
-   On failure, destroy the device and return negative value.
-   Return 0 on success.  */
-int qdev_init(DeviceState *dev)
-{
-    Error *local_err = NULL;
-
-    assert(!dev->realized);
-
-    object_property_set_bool(OBJECT(dev), true, "realized", &local_err);
-    if (local_err != NULL) {
-        qerror_report_err(local_err);
-        error_free(local_err);
-        object_unparent(OBJECT(dev));
-        return -1;
-    }
-    return 0;
-}
-
 static QTAILQ_HEAD(device_listeners, DeviceListener) device_listeners
     = QTAILQ_HEAD_INITIALIZER(device_listeners);
 
@@ -364,13 +343,16 @@ void qdev_simple_device_unplug_cb(HotplugHandler *hotplug_dev,
     object_unparent(OBJECT(dev));
 }
 
-/* Like qdev_init(), but terminate program via error_report() 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.  */
+/* Initialize a device.  Device properties should be set before calling
+   this function.  IRQs and MMIO regions should be connected/mapped after
+   calling this function.
+   On failure, report an error with error_report() and terminate the
+   program.  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)
 {
     Error *err = NULL;
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 4e673f9..8f7e1c9 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -66,7 +66,7 @@ struct VMStateDescription;
  * After successful realization, setting static properties will fail.
  *
  * As an interim step, the #DeviceState:realized property is set by deprecated
- * functions qdev_init() and qdev_init_nofail().
+ * function qdev_init_nofail().
  * In the future, devices will propagate this state change to their children
  * and along busses they expose.
  * The point in time will be deferred to machine creation, so that values
@@ -262,7 +262,6 @@ typedef struct GlobalProperty {
 
 DeviceState *qdev_create(BusState *bus, const char *name);
 DeviceState *qdev_try_create(BusState *bus, const char *name);
-int qdev_init(DeviceState *dev) QEMU_WARN_UNUSED_RESULT;
 void qdev_init_nofail(DeviceState *dev);
 void qdev_set_legacy_instance_id(DeviceState *dev, int alias_id,
                                  int required_for_version);
-- 
1.9.3

Re: [Qemu-devel] [PATCH] qdev: Deprecated qdev_init() is finally unused, drop

[Peter Maydell]

On 12 March 2015 at 14:54, Markus Armbruster <armbru@redhat.com> wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> Depends on
> [PATCH 3/3] s390x: Replace unchecked qdev_init() by qdev_init_nofail()
> [PATCH] macio: Convert to realize()

Always nice to remove a deprecated API...

> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -66,7 +66,7 @@ struct VMStateDescription;
>   * After successful realization, setting static properties will fail.
>   *
>   * As an interim step, the #DeviceState:realized property is set by deprecated
> - * functions qdev_init() and qdev_init_nofail().
> + * function qdev_init_nofail().
>   * In the future, devices will propagate this state change to their children
>   * and along busses they expose.
>   * The point in time will be deferred to machine creation, so that values


So is the non-deprecated approach to have the board call
    object_property_set_bool(OBJECT(dev), true, "realized", &err);
directly itself (handling errors however it feels like) rather
than calling qdev_init_nofail() ?

-- PMM

Re: [Qemu-devel] [PATCH] qdev: Deprecated qdev_init() is finally unused, drop

[Paolo Bonzini]

On 12/03/2015 16:10, Peter Maydell wrote:
> So is the non-deprecated approach to have the board call
>     object_property_set_bool(OBJECT(dev), true, "realized", &err);
> directly itself (handling errors however it feels like) rather
> than calling qdev_init_nofail() ?

qdev_init_nofail seems a good enough name for a shortcut...  The problem
with qdev_init was that it was a symptom of bad errors, but I would
happily un-deprecate qdev_init_nofail, especially until we figure out
whether we really want recursive realization.

Paolo

Re: [Qemu-devel] [PATCH] qdev: Deprecated qdev_init() is finally unused, drop

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 12/03/2015 16:10, Peter Maydell wrote:
>> So is the non-deprecated approach to have the board call
>>     object_property_set_bool(OBJECT(dev), true, "realized", &err);
>> directly itself (handling errors however it feels like) rather
>> than calling qdev_init_nofail() ?
>
> qdev_init_nofail seems a good enough name for a shortcut...  The problem
> with qdev_init was that it was a symptom of bad errors, but I would
> happily un-deprecate qdev_init_nofail, especially until we figure out
> whether we really want recursive realization.

I agree.

I'm getting rid of qdev_init() because it's fundamentally tied to
qerror_report().  And I'm getting rid of qerror_report() because it
doesn't play nicely with error_set().

I can't see anything wrong with qdev_init_nofail().  Except perhaps the
name, but improving it doesn't seem to be worth the churn.