Re: [Qemu-devel] [PATCH for-2.9 1/2] qom: Remove release function from class properties

[Markus Armbruster]

Eduardo Habkost <address@hidden> writes:

> On Thu, Nov 17, 2016 at 01:33:34PM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <address@hidden> writes:
>> 
>> > The release functions are never called for class properties, and
>> > their semantics aren't even defined clearly (should the release
>> > function be called when an instance is destroyed, or when a class
>> > is destroyed?). Remove the unused functionality.
>> >
>> > Signed-off-by: Eduardo Habkost <address@hidden>
> [...]
>> >      g_hash_table_insert(klass->properties, g_strdup(name), prop);
>> > @@ -1808,7 +1806,6 @@ void object_class_property_add_str(ObjectClass 
>> > *klass, const char *name,
>> >      object_class_property_add(klass, name, "string",
>> >                                get ? property_get_str : NULL,
>> >                                set ? property_set_str : NULL,
>> > -                              property_release_str,
>> >                                prop, &local_err);
>> >      if (local_err) {
>> >          error_propagate(errp, local_err);
>> 
>> Here, you drop property_release_str(), which calls g_free().  Assuming
>> you claim that it's never called is correct, this is again no functional
>> change.  But that begs the question why not freeing the stuff
>> property_release_str() frees is correct.  Can you explain?
>
> property_release_str() frees prop. The only right moment to free
> prop is on class destruction or class property removal, but we
> have no mechanisms for class destruction or class property
> removal today.

Would be a nice addition to the commit message.

>> > @@ -1897,7 +1894,6 @@ void object_class_property_add_bool(ObjectClass 
>> > *klass, const char *name,
>> >      object_class_property_add(klass, name, "bool",
>> >                                get ? property_get_bool : NULL,
>> >                                set ? property_set_bool : NULL,
>> > -                              property_release_bool,
>> >                                prop, &local_err);
>> >      if (local_err) {
>> >          error_propagate(errp, local_err);
>> > @@ -1985,7 +1981,6 @@ void object_class_property_add_enum(ObjectClass 
>> > *klass, const char *name,
>> >      object_class_property_add(klass, name, typename,
>> >                                get ? property_get_enum : NULL,
>> >                                set ? property_set_enum : NULL,
>> > -                              property_release_enum,
>> >                                prop, &local_err);
>> >      if (local_err) {
>> >          error_propagate(errp, local_err);
>> > @@ -2082,7 +2077,6 @@ void object_class_property_add_tm(ObjectClass 
>> > *klass, const char *name,
>> >  
>> >      object_class_property_add(klass, name, "struct tm",
>> >                                get ? property_get_tm : NULL, NULL,
>> > -                              property_release_tm,
>> >                                prop, &local_err);
>> >      if (local_err) {
>> >          error_propagate(errp, local_err);
>> 
>> Likewise.
>
> Same as above.
>
>> [...]

Reviewed-by: Markus Armbruster <address@hidden>