Patchwork [04/10] qom: add get_id

login
register
mail settings
Submitter Paolo Bonzini
Date May 23, 2012, 3:44 p.m.
Message ID <1337787881-3579-5-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/160955/
State New
Headers show

Comments

Paolo Bonzini - May 23, 2012, 3:44 p.m.
Some classes may present objects differently in errors, for example if they
are not part of the composition tree or if they are not assigned an id by
the user.  Let them do this with a get_id method on Object, and use the
method consistently where a %(device) appears in the error.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/qdev-properties.c  |    6 +++---
 hw/qdev.c             |   15 ++++++++++++++-
 include/qemu/object.h |   11 +++++++++++
 qom/object.c          |   26 +++++++++++++++++++++++++-
 4 files changed, 53 insertions(+), 5 deletions(-)
Andreas Färber - May 25, 2012, 4:33 p.m.
Am 23.05.2012 17:44, schrieb Paolo Bonzini:
> Some classes may present objects differently in errors, for example if they
> are not part of the composition tree or if they are not assigned an id by
> the user.  Let them do this with a get_id method on Object, and use the
> method consistently where a %(device) appears in the error.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

This patch looks new, so it's not covered by Anthony's carte blanche to
take Paolo's patches into qom-next.

Generally I'm fine with it, some formal issues below.

> ---
>  hw/qdev-properties.c  |    6 +++---
>  hw/qdev.c             |   15 ++++++++++++++-
>  include/qemu/object.h |   11 +++++++++++
>  qom/object.c          |   26 +++++++++++++++++++++++++-
>  4 files changed, 53 insertions(+), 5 deletions(-)

> diff --git a/hw/qdev.c b/hw/qdev.c
> index e909f3b..5d6dc1f 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
[...]
> @@ -714,6 +714,13 @@ static void device_finalize(Object *obj)
>      }
>  }
>  
> +static const char *qdev_get_id(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +
> +    return dev->id?:object_get_typename(obj);

Spaces around ?: please.

> +}
> +
>  static void device_class_base_init(ObjectClass *class, void *data)
>  {
>      DeviceClass *klass = DEVICE_CLASS(class);
[...]
> diff --git a/qom/object.c b/qom/object.c
> index 68a4c57..b19ef94 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -334,6 +334,24 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
>      }
>  }
>  
> +static const char *_object_get_id(Object *obj)

malc's alarm bells will be ringing: Use of an identifier starting with
underscore.

What about object_get_default_id()?

> +{
> +    ObjectProperty *prop;
> +
> +    QTAILQ_FOREACH(prop, &obj->properties, node) {
> +        if (strstart(prop->type, "child<", NULL) && prop->opaque == obj) {

At one point we were discussing an object_property_is_child() inline
helper. I'd really prefer not to spread string comparisons everywhere.
I'll submit a patch based on this one:

http://lists.gnu.org/archive/html/qemu-devel/2012-02/msg02549.html

Andreas

> +            return prop->name;
> +        }
> +    }
> +
> +    return "";
> +}
> +
> +const char *object_get_id(Object *obj)
> +{
> +    return obj->class->get_id(obj);
> +}
> +
>  void object_unparent(Object *obj)
>  {
>      if (obj->parent) {
[...]
> @@ -1236,6 +1254,11 @@ static void object_instance_init(Object *obj)
>      object_property_add_str(obj, "type", qdev_get_type, NULL, NULL);
>  }
>  
> +static void object_class_init(ObjectClass *klass, void *class_data)
> +{
> +    klass->get_id = _object_get_id;
> +}
> +
>  static void register_types(void)
>  {
>      static TypeInfo interface_info = {
[snip]
malc - May 25, 2012, 5:39 p.m.
On Fri, 25 May 2012, Andreas F?rber wrote:

> Am 23.05.2012 17:44, schrieb Paolo Bonzini:
> > Some classes may present objects differently in errors, for example if they
> > are not part of the composition tree or if they are not assigned an id by
> > the user.  Let them do this with a get_id method on Object, and use the
> > method consistently where a %(device) appears in the error.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> This patch looks new, so it's not covered by Anthony's carte blanche to
> take Paolo's patches into qom-next.
> 
> Generally I'm fine with it, some formal issues below.
> 
> > ---
> >  hw/qdev-properties.c  |    6 +++---
> >  hw/qdev.c             |   15 ++++++++++++++-
> >  include/qemu/object.h |   11 +++++++++++
> >  qom/object.c          |   26 +++++++++++++++++++++++++-
> >  4 files changed, 53 insertions(+), 5 deletions(-)
> 
> > diff --git a/hw/qdev.c b/hw/qdev.c
> > index e909f3b..5d6dc1f 100644
> > --- a/hw/qdev.c
> > +++ b/hw/qdev.c
> [...]
> > @@ -714,6 +714,13 @@ static void device_finalize(Object *obj)
> >      }
> >  }
> >  
> > +static const char *qdev_get_id(Object *obj)
> > +{
> > +    DeviceState *dev = DEVICE(obj);
> > +
> > +    return dev->id?:object_get_typename(obj);
> 
> Spaces around ?: please.

Even better is to not use gcc extensions like this one.

> 
> > +}
> > +
> >  static void device_class_base_init(ObjectClass *class, void *data)
> >  {
> >      DeviceClass *klass = DEVICE_CLASS(class);
> [...]
> > diff --git a/qom/object.c b/qom/object.c
> > index 68a4c57..b19ef94 100644
> > --- a/qom/object.c
> > +++ b/qom/object.c
> > @@ -334,6 +334,24 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
> >      }
> >  }
> >  
> > +static const char *_object_get_id(Object *obj)
> 
> malc's alarm bells will be ringing: Use of an identifier starting with
> underscore.

Given that it's forbidden in this context - yeah.

[..snip..]

Patch

diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
index eeb950d..9a6c04a 100644
--- a/hw/qdev-properties.c
+++ b/hw/qdev-properties.c
@@ -931,16 +931,16 @@  void error_set_from_qdev_prop_error(Error **errp, int ret, DeviceState *dev,
     switch (ret) {
     case -EEXIST:
         error_set(errp, QERR_PROPERTY_VALUE_IN_USE,
-                  object_get_typename(OBJECT(dev)), prop->name, value);
+                  object_get_id(OBJECT(dev)), prop->name, value);
         break;
     default:
     case -EINVAL:
         error_set(errp, QERR_PROPERTY_VALUE_BAD,
-                  object_get_typename(OBJECT(dev)), prop->name, value);
+                  object_get_id(OBJECT(dev)), prop->name, value);
         break;
     case -ENOENT:
         error_set(errp, QERR_PROPERTY_VALUE_NOT_FOUND,
-                  object_get_typename(OBJECT(dev)), prop->name, value);
+                  object_get_id(OBJECT(dev)), prop->name, value);
         break;
     case 0:
         break;
diff --git a/hw/qdev.c b/hw/qdev.c
index e909f3b..5d6dc1f 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -259,7 +259,7 @@  void qdev_init_nofail(DeviceState *dev)
 {
     if (qdev_init(dev) < 0) {
         error_report("Initialization of device %s failed",
-                     object_get_typename(OBJECT(dev)));
+                     object_get_id(OBJECT(dev)));
         exit(1);
     }
 }
@@ -714,6 +714,13 @@  static void device_finalize(Object *obj)
     }
 }
 
+static const char *qdev_get_id(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+
+    return dev->id?:object_get_typename(obj);
+}
+
 static void device_class_base_init(ObjectClass *class, void *data)
 {
     DeviceClass *klass = DEVICE_CLASS(class);
@@ -744,6 +751,11 @@  Object *qdev_get_machine(void)
     return dev;
 }
 
+static void device_class_init(ObjectClass *class, void *data)
+{
+    class->get_id = qdev_get_id;
+}
+
 static TypeInfo device_type_info = {
     .name = TYPE_DEVICE,
     .parent = TYPE_OBJECT,
@@ -751,6 +763,7 @@  static TypeInfo device_type_info = {
     .instance_init = device_initfn,
     .instance_finalize = device_finalize,
     .class_base_init = device_class_base_init,
+    .class_init = device_class_init,
     .abstract = true,
     .class_size = sizeof(DeviceClass),
 };
diff --git a/include/qemu/object.h b/include/qemu/object.h
index e714c2c..cb08cfa 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -239,6 +239,9 @@  struct ObjectClass
 {
     /*< private >*/
     Type type;
+
+    /*< public >*/
+    const char *(*get_id)(Object *);
 };
 
 /**
@@ -501,6 +504,14 @@  Object *object_dynamic_cast(Object *obj, const char *typename);
 Object *object_dynamic_cast_assert(Object *obj, const char *typename);
 
 /**
+ * object_get_id:
+ * @obj: A derivative of #Object
+ *
+ * Returns: A string that can be used to refer to @obj.
+ */
+const char *object_get_id(Object *obj);
+
+/**
  * object_get_class:
  * @obj: A derivative of #Object
  *
diff --git a/qom/object.c b/qom/object.c
index 68a4c57..b19ef94 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -334,6 +334,24 @@  static void object_property_del_child(Object *obj, Object *child, Error **errp)
     }
 }
 
+static const char *_object_get_id(Object *obj)
+{
+    ObjectProperty *prop;
+
+    QTAILQ_FOREACH(prop, &obj->properties, node) {
+        if (strstart(prop->type, "child<", NULL) && prop->opaque == obj) {
+            return prop->name;
+        }
+    }
+
+    return "";
+}
+
+const char *object_get_id(Object *obj)
+{
+    return obj->class->get_id(obj);
+}
+
 void object_unparent(Object *obj)
 {
     if (obj->parent) {
@@ -672,7 +690,7 @@  ObjectProperty *object_property_find(Object *obj, const char *name, Error **errp
         }
     }
 
-    error_set(errp, QERR_PROPERTY_NOT_FOUND, "", name);
+    error_set(errp, QERR_PROPERTY_NOT_FOUND, object_get_id(obj), name);
     return NULL;
 }
 
@@ -1236,6 +1254,11 @@  static void object_instance_init(Object *obj)
     object_property_add_str(obj, "type", qdev_get_type, NULL, NULL);
 }
 
+static void object_class_init(ObjectClass *klass, void *class_data)
+{
+    klass->get_id = _object_get_id;
+}
+
 static void register_types(void)
 {
     static TypeInfo interface_info = {
@@ -1248,6 +1271,7 @@  static void register_types(void)
         .name = TYPE_OBJECT,
         .instance_size = sizeof(Object),
         .instance_init = object_instance_init,
+        .class_init = object_class_init,
         .abstract = true,
     };