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

[Andreas Färber]

Am 08.06.2012 03:22, schrieb Anthony Liguori:
> On 06/08/2012 03:31 AM, Andreas Färber wrote:
>> From: Paolo Bonzini<address@hidden>
>>
>> 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<address@hidden>
>> [AF: Renamed _object_get_id() to object_instance_get_id(), avoid ?:.]
>> [AF: Use object_property_is_child().]
>> Signed-off-by: Andreas Färber<address@hidden>
> 
> 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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg