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.

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