Re: [Qemu-devel] [PATCH bugfix v1 3/3] qom: object.h: Update object_get_canon_path* doc

[Peter Crosthwaite]

On Wed, Aug 20, 2014 at 7:07 PM, Peter Maydell <address@hidden> wrote:
> On 20 August 2014 06:08, Peter Crosthwaite <address@hidden> wrote:
>> With information about return value ownership.
>>
>> Signed-off-by: Peter Crosthwaite <address@hidden>
>> ---
>>
>>  include/qom/object.h | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/qom/object.h b/include/qom/object.h
>> index 8618e49..87de889 100644
>> --- a/include/qom/object.h
>> +++ b/include/qom/object.h
>> @@ -1029,7 +1029,8 @@ Object *object_get_root(void);
>>   * object_get_canonical_path_component:
>>   *
>>   * Returns: The final component in the object's canonical path.  The 
>> canonical
>> - * path is the path within the composition tree starting from the root.
>> + * path is the path within the composition tree starting from the root. The
>> + * returned value may not be modified.
>>   */
>>  const gchar *object_get_canonical_path_component(Object *obj);
>
> The other thing you need to say is that the returned string is
> only valid for as long as the object remains a child property
> of its parent. (Is that right? I'm not clear. It also sounds like
> a really awkward lifetime management requirement, which
> suggests to me that really the "return-a-copy" semantics are
> nicer.)

It is. But in QOM that is the naming scheme. An object is uniquely
identified by it's full canonical path. If you don't have a parent,
you don't have a name. If you have a dup of the name, but then the
object gets reparented or destroyed your name is stale and I think the
caller should have to deal with that.

>
> Having object_get_canonical_path_component() and
> object_get_canonical_path() having different return value
> ownership semantics is likely to be a bit confusing I think.
>

Alternatives I can think of include,

1: All memory_region_name() callsites have to do a free.
2: Memory API keeps a single copy of the name (lazily inited in
memory_region_name perhaps).
3: Patch struct object with a char instance_name[] field and have QOM
ensure the fixed location is always a valid name ("\0" for an
unparented object).
4: This

Please let me know your preference.

Regards,
Peter

> If we do do this I think I'd put the doc comment change in
> the same patch that changes the semantics.
>
> thanks
> -- PMM
>