Patchwork [v3,2/2] core/qdev: refactor qdev_get_machine() with type assertion

login
register
mail settings
Submitter Like Xu
Date April 15, 2019, 7:59 a.m.
Message ID <1555315185-16414-3-git-send-email-like.xu@linux.intel.com>
Download mbox | patch
Permalink /patch/772879/
State New
Headers show

Comments

Like Xu - April 15, 2019, 7:59 a.m.
To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
mode) and adds type assertion to qdev_get_machine() in system-emulation mode.

Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Like Xu <like.xu@linux.intel.com>
---
 hw/core/qdev.c         | 16 +++++++++++++---
 include/hw/qdev-core.h |  1 +
 qom/cpu.c              |  5 +++--
 3 files changed, 17 insertions(+), 5 deletions(-)
Eduardo Habkost - April 16, 2019, 9:20 p.m.
On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:
> To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
> this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
> mode) and adds type assertion to qdev_get_machine() in system-emulation mode.
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Like Xu <like.xu@linux.intel.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

I'm queueing the series on machine-next, thanks!
Markus Armbruster - April 17, 2019, 5:14 a.m.
Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:
>> To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
>> this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
>> mode) and adds type assertion to qdev_get_machine() in system-emulation mode.
>> 
>> Suggested-by: Igor Mammedov <imammedo@redhat.com>
>> Signed-off-by: Like Xu <like.xu@linux.intel.com>
>
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
>
> I'm queueing the series on machine-next, thanks!

Hold your horses, please.

I dislike the name qdev_get_machine_uncheck().  I could live with
qdev_get_machine_unchecked().

However, I doubt this is the right approach.

The issue at hand is undisciplined creation of QOM object /machine.

This patch adds an asseertion "undisciplined creation of /machine didn't
create crap", but only in some places.

I think we should never create /machine as (surprising!) side effect of
qdev_get_machine().  Create it explicitly instead, and have
qdev_get_machine() use object_resolve_path("/machine", NULL) to get it.
Look ma, no side effects.
Eduardo Habkost - April 17, 2019, 5:10 p.m.
On Wed, Apr 17, 2019 at 07:14:10AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Mon, Apr 15, 2019 at 03:59:45PM +0800, Like Xu wrote:
> >> To avoid the misuse of qdev_get_machine() if machine hasn't been created yet,
> >> this patch uses qdev_get_machine_uncheck() for obj-common (share with user-only
> >> mode) and adds type assertion to qdev_get_machine() in system-emulation mode.
> >> 
> >> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> >> Signed-off-by: Like Xu <like.xu@linux.intel.com>
> >
> > Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
> >
> > I'm queueing the series on machine-next, thanks!
> 
> Hold your horses, please.
> 
> I dislike the name qdev_get_machine_uncheck().  I could live with
> qdev_get_machine_unchecked().
> 
> However, I doubt this is the right approach.
> 
> The issue at hand is undisciplined creation of QOM object /machine.
> 
> This patch adds an asseertion "undisciplined creation of /machine didn't
> create crap", but only in some places.
> 
> I think we should never create /machine as (surprising!) side effect of
> qdev_get_machine().  Create it explicitly instead, and have
> qdev_get_machine() use object_resolve_path("/machine", NULL) to get it.
> Look ma, no side effects.

OK, I'm dropping this one while we discuss it.

I really miss a good explanation why qdev_get_machine_unchecked()
needs to exist.  When exactly do we want /machine to exist but
not be TYPE_MACHINE?  Why?

Once the expectations and use cases are explained, we can choose
a better name for qdev_get_machine_unchecked() and document it
properly.

Patch

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index f9b6efe..8232216 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -223,7 +223,7 @@  HotplugHandler *qdev_get_machine_hotplug_handler(DeviceState *dev)
 {
     MachineState *machine;
     MachineClass *mc;
-    Object *m_obj = qdev_get_machine();
+    Object *m_obj = qdev_get_machine_uncheck();
 
     if (object_dynamic_cast(m_obj, TYPE_MACHINE)) {
         machine = MACHINE(m_obj);
@@ -815,7 +815,7 @@  static void device_set_realized(Object *obj, bool value, Error **errp)
         if (!obj->parent) {
             gchar *name = g_strdup_printf("device[%d]", unattached_count++);
 
-            object_property_add_child(container_get(qdev_get_machine(),
+            object_property_add_child(container_get(qdev_get_machine_uncheck(),
                                                     "/unattached"),
                                       name, obj, &error_abort);
             unattached_parent = true;
@@ -1095,7 +1095,7 @@  void device_reset(DeviceState *dev)
     }
 }
 
-Object *qdev_get_machine(void)
+Object *qdev_get_machine_uncheck(void)
 {
     static Object *dev;
 
@@ -1106,6 +1106,16 @@  Object *qdev_get_machine(void)
     return dev;
 }
 
+Object *qdev_get_machine(void)
+{
+    static Object *dev;
+
+    dev = qdev_get_machine_uncheck();
+    assert(object_dynamic_cast(dev, TYPE_MACHINE) != NULL);
+
+    return dev;
+}
+
 static const TypeInfo device_type_info = {
     .name = TYPE_DEVICE,
     .parent = TYPE_OBJECT,
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index 33ed3b8..e7c6a5a 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -429,6 +429,7 @@  const struct VMStateDescription *qdev_get_vmsd(DeviceState *dev);
 
 const char *qdev_fw_name(DeviceState *dev);
 
+Object *qdev_get_machine_uncheck(void);
 Object *qdev_get_machine(void);
 
 /* FIXME: make this a link<> */
diff --git a/qom/cpu.c b/qom/cpu.c
index a8d2958..bb877d5 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -325,9 +325,10 @@  static void cpu_common_parse_features(const char *typename, char *features,
 static void cpu_common_realizefn(DeviceState *dev, Error **errp)
 {
     CPUState *cpu = CPU(dev);
-    Object *machine = qdev_get_machine();
+    Object *machine = qdev_get_machine_uncheck();
 
-    /* qdev_get_machine() can return something that's not TYPE_MACHINE
+    /*
+     * qdev_get_machine_uncheck() can return something that's not TYPE_MACHINE
      * if this is one of the user-only emulators; in that case there's
      * no need to check the ignore_memory_transaction_failures board flag.
      */