Re: [Qemu-devel] [PATCH qom-next 2/7] qom: Add get_id

[Anthony Liguori <anthony@codemonkey.ws>]

On 06/08/2012 03:31 AM, Andreas Färber wrote:
> From: Paolo Bonzini<pbonzini@redhat.com>
>
> 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>
> [AF: Renamed _object_get_id() to object_instance_get_id(), avoid ?:.]
> [AF: Use object_property_is_child().]
> Signed-off-by: Andreas Färber<afaerber@suse.de>

Nack.

This creates confusion IMHO.  There's a big difference between an object 
typename and the path to the object.  I don't think we should confuse the two by 
introducing a third type of name and calling it something generic like id.



> ---
>   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-properties.c b/hw/qdev-properties.c
> index fcc0bed..4dc03f6 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -937,16 +937,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 c12e151..7304e4c 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);
>       }
>   }
> @@ -716,6 +716,13 @@ static void device_finalize(Object *obj)
>       }
>   }
>
> +static const char *qdev_get_id(Object *obj)
> +{
> +    DeviceState *dev = DEVICE(obj);
> +
> +    return dev->id != NULL ? dev->id : object_get_typename(obj);
> +}
> +
>   static void device_class_base_init(ObjectClass *class, void *data)
>   {
>       DeviceClass *klass = DEVICE_CLASS(class);
> @@ -746,6 +753,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,
> @@ -753,6 +765,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 1606777..81e0280 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 *);
>   };
>
>   typedef enum ObjectState {
> @@ -507,6 +510,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 93e0499..02464e1 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -346,6 +346,24 @@ static void object_property_del_child(Object *obj, Object *child, Error **errp)
>       }
>   }
>
> +static const char *object_instance_get_id(Object *obj)
> +{
> +    ObjectProperty *prop;
> +
> +    QTAILQ_FOREACH(prop,&obj->properties, node) {
> +        if (object_property_is_child(prop)&&  prop->opaque == obj) {
> +            return prop->name;
> +        }
> +    }
> +
> +    return "";
> +}
> +
> +const char *object_get_id(Object *obj)
> +{
> +    return obj->class->get_id(obj);
> +}
> +

We should use a canonical path IMHO instead of returning a partial name.

Partial names are ambiguous.

Regards,

Anthony Liguori