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

[Andreas Färber]

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<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).
> 
> 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.

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