* [Qemu-devel] [PATCH for-2.9 0/2] qom, qdev: Cleanup release functions @ 2016-11-16 18:57 Eduardo Habkost 2016-11-16 18:57 ` [Qemu-devel] [PATCH for-2.9 1/2] qom: Remove release function from class properties Eduardo Habkost 2016-11-16 18:57 ` [Qemu-devel] [PATCH for-2.9 2/2] qdev: Change signature of PropertyInfo::release Eduardo Habkost 0 siblings, 2 replies; 8+ messages in thread From: Eduardo Habkost @ 2016-11-16 18:57 UTC (permalink / raw) To: qemu-devel, Markus Armbruster, Andreas Färber While working on the qdev class properteis series, I've noticed that the release function for class properties is never called, and have unclear semantics (should it be called when the object is destroyed, or when the class is destroyed?). Patch 1/1 removes the unused feature. Patch 2/2 changes the function signature of qdev property release functions to make their implementations simpler and safer, and make them not depend on the way property release functions are implemented (so the functions don't need to be rewritten if we change qdev to use class properties). Eduardo Habkost (2): qom: Remove release function from class properties qdev: Change signature of PropertyInfo::release backends/hostmem.c | 4 ++-- hw/core/machine.c | 6 +++--- hw/core/qdev-properties-system.c | 8 ++------ hw/core/qdev-properties.c | 10 +++++----- hw/core/qdev.c | 10 +++++++++- hw/i386/pc.c | 8 ++++---- hw/ppc/pnv.c | 2 +- include/hw/qdev-core.h | 2 +- include/qom/object.h | 1 - qom/object.c | 14 ++++---------- 10 files changed, 31 insertions(+), 34 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH for-2.9 1/2] qom: Remove release function from class properties 2016-11-16 18:57 [Qemu-devel] [PATCH for-2.9 0/2] qom, qdev: Cleanup release functions Eduardo Habkost @ 2016-11-16 18:57 ` Eduardo Habkost 2016-11-17 12:33 ` Markus Armbruster 2016-11-16 18:57 ` [Qemu-devel] [PATCH for-2.9 2/2] qdev: Change signature of PropertyInfo::release Eduardo Habkost 1 sibling, 1 reply; 8+ messages in thread From: Eduardo Habkost @ 2016-11-16 18:57 UTC (permalink / raw) To: qemu-devel, Markus Armbruster, Andreas Färber 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 <ehabkost@redhat.com> --- backends/hostmem.c | 4 ++-- hw/core/machine.c | 6 +++--- hw/i386/pc.c | 8 ++++---- hw/ppc/pnv.c | 2 +- include/qom/object.h | 1 - qom/object.c | 14 ++++---------- 6 files changed, 14 insertions(+), 21 deletions(-) diff --git a/backends/hostmem.c b/backends/hostmem.c index 4256d24..856e96e 100644 --- a/backends/hostmem.c +++ b/backends/hostmem.c @@ -368,11 +368,11 @@ host_memory_backend_class_init(ObjectClass *oc, void *data) object_class_property_add(oc, "size", "int", host_memory_backend_get_size, host_memory_backend_set_size, - NULL, NULL, &error_abort); + NULL, &error_abort); object_class_property_add(oc, "host-nodes", "int", host_memory_backend_get_host_nodes, host_memory_backend_set_host_nodes, - NULL, NULL, &error_abort); + NULL, &error_abort); object_class_property_add_enum(oc, "policy", "HostMemPolicy", HostMemPolicy_lookup, host_memory_backend_get_policy, diff --git a/hw/core/machine.c b/hw/core/machine.c index b0fd91f..c64e5f1 100644 --- a/hw/core/machine.c +++ b/hw/core/machine.c @@ -372,13 +372,13 @@ static void machine_class_init(ObjectClass *oc, void *data) object_class_property_add(oc, "kernel-irqchip", "OnOffSplit", NULL, machine_set_kernel_irqchip, - NULL, NULL, &error_abort); + NULL, &error_abort); object_class_property_set_description(oc, "kernel-irqchip", "Configure KVM in-kernel irqchip", &error_abort); object_class_property_add(oc, "kvm-shadow-mem", "int", machine_get_kvm_shadow_mem, machine_set_kvm_shadow_mem, - NULL, NULL, &error_abort); + NULL, &error_abort); object_class_property_set_description(oc, "kvm-shadow-mem", "KVM shadow MMU size", &error_abort); @@ -409,7 +409,7 @@ static void machine_class_init(ObjectClass *oc, void *data) object_class_property_add(oc, "phandle-start", "int", machine_get_phandle_start, machine_set_phandle_start, - NULL, NULL, &error_abort); + NULL, &error_abort); object_class_property_set_description(oc, "phandle-start", "The first phandle ID we may generate dynamically", &error_abort); diff --git a/hw/i386/pc.c b/hw/i386/pc.c index a9b1950..46f95bf 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -2308,24 +2308,24 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) object_class_property_add(oc, PC_MACHINE_MEMHP_REGION_SIZE, "int", pc_machine_get_hotplug_memory_region_size, NULL, - NULL, NULL, &error_abort); + NULL, &error_abort); object_class_property_add(oc, PC_MACHINE_MAX_RAM_BELOW_4G, "size", pc_machine_get_max_ram_below_4g, pc_machine_set_max_ram_below_4g, - NULL, NULL, &error_abort); + NULL, &error_abort); object_class_property_set_description(oc, PC_MACHINE_MAX_RAM_BELOW_4G, "Maximum ram below the 4G boundary (32bit boundary)", &error_abort); object_class_property_add(oc, PC_MACHINE_SMM, "OnOffAuto", pc_machine_get_smm, pc_machine_set_smm, - NULL, NULL, &error_abort); + NULL, &error_abort); object_class_property_set_description(oc, PC_MACHINE_SMM, "Enable SMM (pc & q35)", &error_abort); object_class_property_add(oc, PC_MACHINE_VMPORT, "OnOffAuto", pc_machine_get_vmport, pc_machine_set_vmport, - NULL, NULL, &error_abort); + NULL, &error_abort); object_class_property_set_description(oc, PC_MACHINE_VMPORT, "Enable vmport (pc & q35)", &error_abort); diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c index 9df7b25..3fb68c3 100644 --- a/hw/ppc/pnv.c +++ b/hw/ppc/pnv.c @@ -777,7 +777,7 @@ static void powernv_machine_class_props_init(ObjectClass *oc) { object_class_property_add(oc, "num-chips", "uint32_t", pnv_get_num_chips, pnv_set_num_chips, - NULL, NULL, NULL); + NULL, NULL); object_class_property_set_description(oc, "num-chips", "Specifies the number of processor chips", NULL); diff --git a/include/qom/object.h b/include/qom/object.h index 5ecc2d1..fbf9df2 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -945,7 +945,6 @@ ObjectProperty *object_class_property_add(ObjectClass *klass, const char *name, const char *type, ObjectPropertyAccessor *get, ObjectPropertyAccessor *set, - ObjectPropertyRelease *release, void *opaque, Error **errp); /** diff --git a/qom/object.c b/qom/object.c index 7a05e35..c6c0255 100644 --- a/qom/object.c +++ b/qom/object.c @@ -955,7 +955,6 @@ object_class_property_add(ObjectClass *klass, const char *type, ObjectPropertyAccessor *get, ObjectPropertyAccessor *set, - ObjectPropertyRelease *release, void *opaque, Error **errp) { @@ -975,7 +974,6 @@ object_class_property_add(ObjectClass *klass, prop->get = get; prop->set = set; - prop->release = release; prop->opaque = opaque; 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); @@ -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); @@ -2134,7 +2128,7 @@ void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name, const uint8_t *v, Error **errp) { object_class_property_add(klass, name, "uint8", property_get_uint8_ptr, - NULL, NULL, (void *)v, errp); + NULL, (void *)v, errp); } void object_property_add_uint16_ptr(Object *obj, const char *name, @@ -2148,7 +2142,7 @@ void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name, const uint16_t *v, Error **errp) { object_class_property_add(klass, name, "uint16", property_get_uint16_ptr, - NULL, NULL, (void *)v, errp); + NULL, (void *)v, errp); } void object_property_add_uint32_ptr(Object *obj, const char *name, @@ -2162,7 +2156,7 @@ void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name, const uint32_t *v, Error **errp) { object_class_property_add(klass, name, "uint32", property_get_uint32_ptr, - NULL, NULL, (void *)v, errp); + NULL, (void *)v, errp); } void object_property_add_uint64_ptr(Object *obj, const char *name, @@ -2176,7 +2170,7 @@ void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name, const uint64_t *v, Error **errp) { object_class_property_add(klass, name, "uint64", property_get_uint64_ptr, - NULL, NULL, (void *)v, errp); + NULL, (void *)v, errp); } typedef struct { -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 1/2] qom: Remove release function from class properties 2016-11-16 18:57 ` [Qemu-devel] [PATCH for-2.9 1/2] qom: Remove release function from class properties Eduardo Habkost @ 2016-11-17 12:33 ` Markus Armbruster 2016-11-17 13:36 ` Eduardo Habkost 0 siblings, 1 reply; 8+ messages in thread From: Markus Armbruster @ 2016-11-17 12:33 UTC (permalink / raw) To: Eduardo Habkost; +Cc: qemu-devel, Andreas Färber Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com> > --- > backends/hostmem.c | 4 ++-- > hw/core/machine.c | 6 +++--- > hw/i386/pc.c | 8 ++++---- > hw/ppc/pnv.c | 2 +- > include/qom/object.h | 1 - > qom/object.c | 14 ++++---------- > 6 files changed, 14 insertions(+), 21 deletions(-) > > diff --git a/backends/hostmem.c b/backends/hostmem.c > index 4256d24..856e96e 100644 > --- a/backends/hostmem.c > +++ b/backends/hostmem.c > @@ -368,11 +368,11 @@ host_memory_backend_class_init(ObjectClass *oc, void *data) > object_class_property_add(oc, "size", "int", > host_memory_backend_get_size, > host_memory_backend_set_size, > - NULL, NULL, &error_abort); > + NULL, &error_abort); > object_class_property_add(oc, "host-nodes", "int", > host_memory_backend_get_host_nodes, > host_memory_backend_set_host_nodes, > - NULL, NULL, &error_abort); > + NULL, &error_abort); > object_class_property_add_enum(oc, "policy", "HostMemPolicy", > HostMemPolicy_lookup, > host_memory_backend_get_policy, > diff --git a/hw/core/machine.c b/hw/core/machine.c > index b0fd91f..c64e5f1 100644 > --- a/hw/core/machine.c > +++ b/hw/core/machine.c > @@ -372,13 +372,13 @@ static void machine_class_init(ObjectClass *oc, void *data) > > object_class_property_add(oc, "kernel-irqchip", "OnOffSplit", > NULL, machine_set_kernel_irqchip, > - NULL, NULL, &error_abort); > + NULL, &error_abort); > object_class_property_set_description(oc, "kernel-irqchip", > "Configure KVM in-kernel irqchip", &error_abort); > > object_class_property_add(oc, "kvm-shadow-mem", "int", > machine_get_kvm_shadow_mem, machine_set_kvm_shadow_mem, > - NULL, NULL, &error_abort); > + NULL, &error_abort); > object_class_property_set_description(oc, "kvm-shadow-mem", > "KVM shadow MMU size", &error_abort); > > @@ -409,7 +409,7 @@ static void machine_class_init(ObjectClass *oc, void *data) > > object_class_property_add(oc, "phandle-start", "int", > machine_get_phandle_start, machine_set_phandle_start, > - NULL, NULL, &error_abort); > + NULL, &error_abort); > object_class_property_set_description(oc, "phandle-start", > "The first phandle ID we may generate dynamically", &error_abort); > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index a9b1950..46f95bf 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -2308,24 +2308,24 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) > > object_class_property_add(oc, PC_MACHINE_MEMHP_REGION_SIZE, "int", > pc_machine_get_hotplug_memory_region_size, NULL, > - NULL, NULL, &error_abort); > + NULL, &error_abort); > > object_class_property_add(oc, PC_MACHINE_MAX_RAM_BELOW_4G, "size", > pc_machine_get_max_ram_below_4g, pc_machine_set_max_ram_below_4g, > - NULL, NULL, &error_abort); > + NULL, &error_abort); > > object_class_property_set_description(oc, PC_MACHINE_MAX_RAM_BELOW_4G, > "Maximum ram below the 4G boundary (32bit boundary)", &error_abort); > > object_class_property_add(oc, PC_MACHINE_SMM, "OnOffAuto", > pc_machine_get_smm, pc_machine_set_smm, > - NULL, NULL, &error_abort); > + NULL, &error_abort); > object_class_property_set_description(oc, PC_MACHINE_SMM, > "Enable SMM (pc & q35)", &error_abort); > > object_class_property_add(oc, PC_MACHINE_VMPORT, "OnOffAuto", > pc_machine_get_vmport, pc_machine_set_vmport, > - NULL, NULL, &error_abort); > + NULL, &error_abort); > object_class_property_set_description(oc, PC_MACHINE_VMPORT, > "Enable vmport (pc & q35)", &error_abort); > > diff --git a/hw/ppc/pnv.c b/hw/ppc/pnv.c > index 9df7b25..3fb68c3 100644 > --- a/hw/ppc/pnv.c > +++ b/hw/ppc/pnv.c > @@ -777,7 +777,7 @@ static void powernv_machine_class_props_init(ObjectClass *oc) > { > object_class_property_add(oc, "num-chips", "uint32_t", > pnv_get_num_chips, pnv_set_num_chips, > - NULL, NULL, NULL); > + NULL, NULL); > object_class_property_set_description(oc, "num-chips", > "Specifies the number of processor chips", > NULL); The above all drop null releases. No functional change. > diff --git a/include/qom/object.h b/include/qom/object.h > index 5ecc2d1..fbf9df2 100644 > --- a/include/qom/object.h > +++ b/include/qom/object.h > @@ -945,7 +945,6 @@ ObjectProperty *object_class_property_add(ObjectClass *klass, const char *name, > const char *type, > ObjectPropertyAccessor *get, > ObjectPropertyAccessor *set, > - ObjectPropertyRelease *release, > void *opaque, Error **errp); > > /** > diff --git a/qom/object.c b/qom/object.c > index 7a05e35..c6c0255 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -955,7 +955,6 @@ object_class_property_add(ObjectClass *klass, > const char *type, > ObjectPropertyAccessor *get, > ObjectPropertyAccessor *set, > - ObjectPropertyRelease *release, > void *opaque, > Error **errp) > { > @@ -975,7 +974,6 @@ object_class_property_add(ObjectClass *klass, > > prop->get = get; > prop->set = set; > - prop->release = release; > prop->opaque = opaque; > > 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? > @@ -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. > @@ -2134,7 +2128,7 @@ void object_class_property_add_uint8_ptr(ObjectClass *klass, const char *name, > const uint8_t *v, Error **errp) > { > object_class_property_add(klass, name, "uint8", property_get_uint8_ptr, > - NULL, NULL, (void *)v, errp); > + NULL, (void *)v, errp); > } > > void object_property_add_uint16_ptr(Object *obj, const char *name, > @@ -2148,7 +2142,7 @@ void object_class_property_add_uint16_ptr(ObjectClass *klass, const char *name, > const uint16_t *v, Error **errp) > { > object_class_property_add(klass, name, "uint16", property_get_uint16_ptr, > - NULL, NULL, (void *)v, errp); > + NULL, (void *)v, errp); > } > > void object_property_add_uint32_ptr(Object *obj, const char *name, > @@ -2162,7 +2156,7 @@ void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name, > const uint32_t *v, Error **errp) > { > object_class_property_add(klass, name, "uint32", property_get_uint32_ptr, > - NULL, NULL, (void *)v, errp); > + NULL, (void *)v, errp); > } > > void object_property_add_uint64_ptr(Object *obj, const char *name, > @@ -2176,7 +2170,7 @@ void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name, > const uint64_t *v, Error **errp) > { > object_class_property_add(klass, name, "uint64", property_get_uint64_ptr, > - NULL, NULL, (void *)v, errp); > + NULL, (void *)v, errp); > } > > typedef struct { More null releases. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 1/2] qom: Remove release function from class properties 2016-11-17 12:33 ` Markus Armbruster @ 2016-11-17 13:36 ` Eduardo Habkost 2016-11-17 14:45 ` Markus Armbruster 0 siblings, 1 reply; 8+ messages in thread From: Eduardo Habkost @ 2016-11-17 13:36 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, Andreas Färber On Thu, Nov 17, 2016 at 01:33:34PM +0100, Markus Armbruster wrote: > Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com> [...] > > 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. > > > @@ -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. > [...] -- Eduardo ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 1/2] qom: Remove release function from class properties 2016-11-17 13:36 ` Eduardo Habkost @ 2016-11-17 14:45 ` Markus Armbruster 0 siblings, 0 replies; 8+ messages in thread From: Markus Armbruster @ 2016-11-17 14:45 UTC (permalink / raw) To: Eduardo Habkost; +Cc: qemu-devel, Andreas Färber Eduardo Habkost <ehabkost@redhat.com> writes: > On Thu, Nov 17, 2016 at 01:33:34PM +0100, Markus Armbruster wrote: >> Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com> > [...] >> > 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 <armbru@redhat.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH for-2.9 2/2] qdev: Change signature of PropertyInfo::release 2016-11-16 18:57 [Qemu-devel] [PATCH for-2.9 0/2] qom, qdev: Cleanup release functions Eduardo Habkost 2016-11-16 18:57 ` [Qemu-devel] [PATCH for-2.9 1/2] qom: Remove release function from class properties Eduardo Habkost @ 2016-11-16 18:57 ` Eduardo Habkost 2016-11-17 12:26 ` Markus Armbruster 1 sibling, 1 reply; 8+ messages in thread From: Eduardo Habkost @ 2016-11-16 18:57 UTC (permalink / raw) To: qemu-devel, Markus Armbruster, Andreas Färber Change the function signature to make implementations simpler and safer. No void pointers and Object->DeviceState casts inside each release function. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- hw/core/qdev-properties-system.c | 8 ++------ hw/core/qdev-properties.c | 10 +++++----- hw/core/qdev.c | 10 +++++++++- include/hw/qdev-core.h | 2 +- 4 files changed, 17 insertions(+), 13 deletions(-) diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c index 1b7ea50..4f49109 100644 --- a/hw/core/qdev-properties-system.c +++ b/hw/core/qdev-properties-system.c @@ -112,10 +112,8 @@ fail: } } -static void release_drive(Object *obj, const char *name, void *opaque) +static void release_drive(DeviceState *dev, Property *prop) { - DeviceState *dev = DEVICE(obj); - Property *prop = opaque; BlockBackend **ptr = qdev_get_prop_ptr(dev, prop); if (*ptr) { @@ -210,10 +208,8 @@ static void set_chr(Object *obj, Visitor *v, const char *name, void *opaque, g_free(str); } -static void release_chr(Object *obj, const char *name, void *opaque) +static void release_chr(DeviceState *dev, Property *prop) { - DeviceState *dev = DEVICE(obj); - Property *prop = opaque; CharBackend *be = qdev_get_prop_ptr(dev, prop); qemu_chr_fe_deinit(be); diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c index 2a82768..3709050 100644 --- a/hw/core/qdev-properties.c +++ b/hw/core/qdev-properties.c @@ -383,10 +383,9 @@ PropertyInfo qdev_prop_uint64 = { /* --- string --- */ -static void release_string(Object *obj, const char *name, void *opaque) +static void release_string(DeviceState *dev, Property *prop) { - Property *prop = opaque; - g_free(*(char **)qdev_get_prop_ptr(DEVICE(obj), prop)); + g_free(*(char **)qdev_get_prop_ptr(dev, prop)); } static void get_string(Object *obj, Visitor *v, const char *name, @@ -823,7 +822,7 @@ PropertyInfo qdev_prop_pci_host_devaddr = { typedef struct { struct Property prop; char *propname; - ObjectPropertyRelease *release; + void (*release)(DeviceState *dev, Property *prop); } ArrayElementProperty; /* object property release callback for array element properties: @@ -832,9 +831,10 @@ typedef struct { */ static void array_element_release(Object *obj, const char *name, void *opaque) { + DeviceState *dev = DEVICE(obj); ArrayElementProperty *p = opaque; if (p->release) { - p->release(obj, name, opaque); + p->release(dev, &p->prop); } g_free(p->propname); g_free(p); diff --git a/hw/core/qdev.c b/hw/core/qdev.c index 5783442..b859e15 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -774,6 +774,14 @@ static void qdev_property_add_legacy(DeviceState *dev, Property *prop, g_free(name); } +static void qdev_release_prop(Object *obj, const char *name, void *opaque) +{ + DeviceState *dev = DEVICE(obj); + Property *prop = opaque; + + prop->info->release(dev, prop); +} + /** * qdev_property_add_static: * @dev: Device to add the property to. @@ -801,7 +809,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop, object_property_add(obj, prop->name, prop->info->name, prop->info->get, prop->info->set, - prop->info->release, + prop->info->release ? qdev_release_prop : NULL, prop, &local_err); if (local_err) { diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 2c97347..5ea2095 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -251,7 +251,7 @@ struct PropertyInfo { int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len); ObjectPropertyAccessor *get; ObjectPropertyAccessor *set; - ObjectPropertyRelease *release; + void (*release)(DeviceState *dev, Property *prop); }; /** -- 2.7.4 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 2/2] qdev: Change signature of PropertyInfo::release 2016-11-16 18:57 ` [Qemu-devel] [PATCH for-2.9 2/2] qdev: Change signature of PropertyInfo::release Eduardo Habkost @ 2016-11-17 12:26 ` Markus Armbruster 2016-11-17 13:40 ` Eduardo Habkost 0 siblings, 1 reply; 8+ messages in thread From: Markus Armbruster @ 2016-11-17 12:26 UTC (permalink / raw) To: Eduardo Habkost; +Cc: qemu-devel, Andreas Färber Eduardo Habkost <ehabkost@redhat.com> writes: > Change the function signature to make implementations simpler and > safer. No void pointers and Object->DeviceState casts inside each > release function. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > hw/core/qdev-properties-system.c | 8 ++------ > hw/core/qdev-properties.c | 10 +++++----- > hw/core/qdev.c | 10 +++++++++- > include/hw/qdev-core.h | 2 +- > 4 files changed, 17 insertions(+), 13 deletions(-) > > diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c > index 1b7ea50..4f49109 100644 > --- a/hw/core/qdev-properties-system.c > +++ b/hw/core/qdev-properties-system.c > @@ -112,10 +112,8 @@ fail: > } > } > > -static void release_drive(Object *obj, const char *name, void *opaque) > +static void release_drive(DeviceState *dev, Property *prop) > { > - DeviceState *dev = DEVICE(obj); > - Property *prop = opaque; > BlockBackend **ptr = qdev_get_prop_ptr(dev, prop); > > if (*ptr) { > @@ -210,10 +208,8 @@ static void set_chr(Object *obj, Visitor *v, const char *name, void *opaque, > g_free(str); > } > > -static void release_chr(Object *obj, const char *name, void *opaque) > +static void release_chr(DeviceState *dev, Property *prop) > { > - DeviceState *dev = DEVICE(obj); > - Property *prop = opaque; > CharBackend *be = qdev_get_prop_ptr(dev, prop); > > qemu_chr_fe_deinit(be); > diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c > index 2a82768..3709050 100644 > --- a/hw/core/qdev-properties.c > +++ b/hw/core/qdev-properties.c > @@ -383,10 +383,9 @@ PropertyInfo qdev_prop_uint64 = { > > /* --- string --- */ > > -static void release_string(Object *obj, const char *name, void *opaque) > +static void release_string(DeviceState *dev, Property *prop) > { > - Property *prop = opaque; > - g_free(*(char **)qdev_get_prop_ptr(DEVICE(obj), prop)); > + g_free(*(char **)qdev_get_prop_ptr(dev, prop)); > } Type-punning moved from the three release methods to ... > > static void get_string(Object *obj, Visitor *v, const char *name, > @@ -823,7 +822,7 @@ PropertyInfo qdev_prop_pci_host_devaddr = { > typedef struct { > struct Property prop; > char *propname; > - ObjectPropertyRelease *release; > + void (*release)(DeviceState *dev, Property *prop); > } ArrayElementProperty; > > /* object property release callback for array element properties: > @@ -832,9 +831,10 @@ typedef struct { > */ > static void array_element_release(Object *obj, const char *name, void *opaque) > { > + DeviceState *dev = DEVICE(obj); > ArrayElementProperty *p = opaque; > if (p->release) { > - p->release(obj, name, opaque); > + p->release(dev, &p->prop); > } > g_free(p->propname); > g_free(p); ... this old wrapper and ... > diff --git a/hw/core/qdev.c b/hw/core/qdev.c > index 5783442..b859e15 100644 > --- a/hw/core/qdev.c > +++ b/hw/core/qdev.c > @@ -774,6 +774,14 @@ static void qdev_property_add_legacy(DeviceState *dev, Property *prop, > g_free(name); > } > > +static void qdev_release_prop(Object *obj, const char *name, void *opaque) > +{ > + DeviceState *dev = DEVICE(obj); > + Property *prop = opaque; ... a new one. Hmm. > + > + prop->info->release(dev, prop); Roundabout way to say prop->info->release(DEVICE(obj), opaque). Matter of taste. > +} > + > /** > * qdev_property_add_static: > * @dev: Device to add the property to. > @@ -801,7 +809,7 @@ void qdev_property_add_static(DeviceState *dev, Property *prop, > > object_property_add(obj, prop->name, prop->info->name, > prop->info->get, prop->info->set, > - prop->info->release, > + prop->info->release ? qdev_release_prop : NULL, > prop, &local_err); > > if (local_err) { > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 2c97347..5ea2095 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -251,7 +251,7 @@ struct PropertyInfo { > int (*print)(DeviceState *dev, Property *prop, char *dest, size_t len); > ObjectPropertyAccessor *get; > ObjectPropertyAccessor *set; > - ObjectPropertyRelease *release; > + void (*release)(DeviceState *dev, Property *prop); > }; > > /** The transformation looks correct to me, but I'm not sure it's worthwhile. Moreover, it creates an inconsistency between set()/get() and release(), both here and in the concrete implementations. For instance, get_string() and set_string() continue to take obj, name, opaque, while release_string() is changed to take dev, prop. I don't like that. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 2/2] qdev: Change signature of PropertyInfo::release 2016-11-17 12:26 ` Markus Armbruster @ 2016-11-17 13:40 ` Eduardo Habkost 0 siblings, 0 replies; 8+ messages in thread From: Eduardo Habkost @ 2016-11-17 13:40 UTC (permalink / raw) To: Markus Armbruster; +Cc: qemu-devel, Andreas Färber On Thu, Nov 17, 2016 at 01:26:47PM +0100, Markus Armbruster wrote: [...] > The transformation looks correct to me, but I'm not sure it's > worthwhile. > > Moreover, it creates an inconsistency between set()/get() and release(), > both here and in the concrete implementations. For instance, > get_string() and set_string() continue to take obj, name, opaque, while > release_string() is changed to take dev, prop. I don't like that. My main goal was just to make it more difficult to mistakenly use the qdev propery release functions (that should be called at object destruction time) as the release functions for class properties (that should be called at class destruction time). But this is not as important if patch 1/2 removes the class property release functions. We can drop this patch. -- Eduardo ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-11-17 14:45 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-16 18:57 [Qemu-devel] [PATCH for-2.9 0/2] qom, qdev: Cleanup release functions Eduardo Habkost 2016-11-16 18:57 ` [Qemu-devel] [PATCH for-2.9 1/2] qom: Remove release function from class properties Eduardo Habkost 2016-11-17 12:33 ` Markus Armbruster 2016-11-17 13:36 ` Eduardo Habkost 2016-11-17 14:45 ` Markus Armbruster 2016-11-16 18:57 ` [Qemu-devel] [PATCH for-2.9 2/2] qdev: Change signature of PropertyInfo::release Eduardo Habkost 2016-11-17 12:26 ` Markus Armbruster 2016-11-17 13:40 ` Eduardo Habkost
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).