* [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
* [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 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 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
* 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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.