From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:58625) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ScnuG-0001sv-Fe for qemu-devel@nongnu.org; Thu, 07 Jun 2012 21:22:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ScnuE-0001S9-Gp for qemu-devel@nongnu.org; Thu, 07 Jun 2012 21:22:28 -0400 Received: from mail-pz0-f45.google.com ([209.85.210.45]:34780) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ScnuE-0001Rw-7D for qemu-devel@nongnu.org; Thu, 07 Jun 2012 21:22:26 -0400 Received: by dadv2 with SMTP id v2so1813251dad.4 for ; Thu, 07 Jun 2012 18:22:24 -0700 (PDT) Message-ID: <4FD153CC.6040304@codemonkey.ws> Date: Fri, 08 Jun 2012 09:22:20 +0800 From: Anthony Liguori MIME-Version: 1.0 References: <1339097465-22977-1-git-send-email-afaerber@suse.de> <1339097465-22977-3-git-send-email-afaerber@suse.de> In-Reply-To: <1339097465-22977-3-git-send-email-afaerber@suse.de> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH qom-next 2/7] qom: Add get_id List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= Cc: pbonzini@redhat.com, qemu-devel@nongnu.org On 06/08/2012 03:31 AM, Andreas Färber wrote: > From: 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 > [AF: Renamed _object_get_id() to object_instance_get_id(), avoid ?:.] > [AF: Use object_property_is_child().] > Signed-off-by: Andreas Färber 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