| Message ID | 1339097465-22977-3-git-send-email-afaerber@suse.de |
|---|---|
| State | New |
| Headers | show |
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
Am 08.06.2012 03:22, schrieb Anthony Liguori: > 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. Unfortunately that comment comes a bit late (original submission May 23rd, me specifically cc'ing you in my reply that it's new and not covered by your carte blanche). It renders all your later Reviewed-bys on this series obsolete since they introduce uses of this function. The general idea as I understand it is to have a mechanism for having devices fill in their ID into %(device) in the error messages once the code emitting those errors is at Object level. Peter's improved error message practically becomes "Property '.propertyname' ..." because without it we'll need to fill in "" in lack of an actual value. > 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. [...] >> 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. Possibly, but a partial name still is an improvement over the current situation with no name at all. Also my previous request to not use %(device) for Object-level API was rejected with reference to existing QMP users, so by the same argument we cannot stuff a QOM path into %(device) either and would need a new QMP field %(path) or so. Cc'ing Luiz. There is no guarantee that the object actually has a canonical path at that point, and object_get_canonical_path() would g_assert() in such a case. Please advise on how to proceed with this series. Andreas
On 06/08/2012 03:11 PM, Andreas Färber wrote: > Am 08.06.2012 03:22, schrieb Anthony Liguori: >> 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. > > Unfortunately that comment comes a bit late (original submission May > 23rd, me specifically cc'ing you in my reply that it's new and not > covered by your carte blanche). Uh, that was a week before the release. Don't send significant things during the final part of a release and expect to get significant review. > The general idea as I understand it is to have a mechanism for having > devices fill in their ID into %(device) in the error messages once the > code emitting those errors is at Object level. Peter's improved error > message practically becomes "Property '.propertyname' ..." because > without it we'll need to fill in "" in lack of an actual value. Ambiguity is fundamentally bad. If you want to return the path, return the path. If you want to return the type, return the type. Returning the type because we're too lazy to implement the path properly is not acceptable and makes the error messages useless (because they're ambiguous). Have a separate 'path' and 'typename' attribute in the errors. With some cleverness, you can probably use '%p' and interpret the pointer an as Object * and automagically generate an embedded 'device': { 'path': '/path/to/device', 'typename': 'FancyType' }. >> >> We should use a canonical path IMHO instead of returning a partial name. >> >> Partial names are ambiguous. > > Possibly, but a partial name still is an improvement over the current > situation with no name at all. Also my previous request to not use > %(device) for Object-level API was rejected with reference to existing > QMP users, so by the same argument we cannot stuff a QOM path into > %(device) either and would need a new QMP field %(path) or so. Cc'ing Luiz. Since qdev->id is NULL 90% of the time, I don't think a user can realistically rely on it. I don't think changing the type of the data in the error is going to be a problem. Doesn't libvirt ignore the contents of an error object? Regards, Anthony Liguori > > There is no guarantee that the object actually has a canonical path at > that point, and object_get_canonical_path() would g_assert() in such a case. > > Please advise on how to proceed with this series. > > Andreas >
Am 08.06.2012 09:44, schrieb Anthony Liguori: > On 06/08/2012 03:11 PM, Andreas Färber wrote: >> Am 08.06.2012 03:22, schrieb Anthony Liguori: >>> 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. >> >> Unfortunately that comment comes a bit late (original submission May >> 23rd, me specifically cc'ing you in my reply that it's new and not >> covered by your carte blanche). > > Uh, that was a week before the release. Don't send significant things > during the final part of a release and expect to get significant review. Well, obviously we were hoping to get this series committed immediately after the release, so we needed to get review before that. :) >> The general idea as I understand it is to have a mechanism for having >> devices fill in their ID into %(device) in the error messages once the >> code emitting those errors is at Object level. Peter's improved error >> message practically becomes "Property '.propertyname' ..." because >> without it we'll need to fill in "" in lack of an actual value. > > Ambiguity is fundamentally bad. If you want to return the path, return > the path. If you want to return the type, return the type. Returning > the type because we're too lazy to implement the path properly is not > acceptable and makes the error messages useless (because they're > ambiguous). > > Have a separate 'path' and 'typename' attribute in the errors. With > some cleverness, you can probably use '%p' and interpret the pointer an > as Object * and automagically generate an embedded 'device': { 'path': > '/path/to/device', 'typename': 'FancyType' }. > >>> >>> We should use a canonical path IMHO instead of returning a partial name. >>> >>> Partial names are ambiguous. >> >> Possibly, but a partial name still is an improvement over the current >> situation with no name at all. Also my previous request to not use >> %(device) for Object-level API was rejected with reference to existing >> QMP users, so by the same argument we cannot stuff a QOM path into >> %(device) either and would need a new QMP field %(path) or so. Cc'ing >> Luiz. > > Since qdev->id is NULL 90% of the time, I don't think a user can > realistically rely on it. I don't think changing the type of the data > in the error is going to be a problem. > > Doesn't libvirt ignore the contents of an error object? I'm out of my field there, those questions are for Luiz and the libvirt guys to answer. (Context is ongoing DeviceState -> Object transition on qom-next branch, properties being moved to Object and what info to include in Error objects then) If you reach consensus how to handle this, I can refactor accordingly, or Paolo could pick up my tweaked series again and refactor himself. Regards, Andreas >> There is no guarantee that the object actually has a canonical path at >> that point, and object_get_canonical_path() would g_assert() in such a >> case.
On Fri, Jun 08, 2012 at 10:17:05AM +0200, Andreas Färber wrote: > Am 08.06.2012 09:44, schrieb Anthony Liguori: > > On 06/08/2012 03:11 PM, Andreas Färber wrote: > > Since qdev->id is NULL 90% of the time, I don't think a user can > > realistically rely on it. I don't think changing the type of the data > > in the error is going to be a problem. > > > > Doesn't libvirt ignore the contents of an error object? > > I'm out of my field there, those questions are for Luiz and the libvirt > guys to answer. (Context is ongoing DeviceState -> Object transition on > qom-next branch, properties being moved to Object and what info to > include in Error objects then) Libvirt will look at two fields in the a JSON 'error' reply. We will pass 'desc' field through to the libvirt code - we treat it as an opaque value. We will look at the string value in the 'class' field to check for certain types of error - eg we strcmp against things like MigrationExpected, DeviceNotActive, CommandNotFound, KVMMissingCap, DeviceInUse, etc. All other fields are ignored. Regards, Daniel
On 06/08/2012 01:44 AM, Anthony Liguori wrote: > Since qdev->id is NULL 90% of the time, I don't think a user can > realistically rely on it. I don't think changing the type of the data > in the error is going to be a problem. > > Doesn't libvirt ignore the contents of an error object? Libvirt tries to parse the contents of an error object in order to reconstruct the most user-friendly message possible for libvirt's error message. There's already some special-casing in libvirt's code, and we can probably add more if you change the layout of particular errors.
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); +} + void object_unparent(Object *obj) { if (obj->parent) { @@ -685,7 +703,7 @@ ObjectProperty *object_property_find(Object *obj, const char *name, } } - error_set(errp, QERR_PROPERTY_NOT_FOUND, "", name); + error_set(errp, QERR_PROPERTY_NOT_FOUND, object_get_id(obj), name); return NULL; } @@ -1249,6 +1267,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_instance_get_id; +} + static void register_types(void) { static TypeInfo interface_info = { @@ -1261,6 +1284,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, };