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

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(a)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(a)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(a)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?

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