[Qemu-devel] [PATCH] qom: Reject attempts to add a property that already exists

[Qemu-devel] [PATCH] qom: Reject attempts to add a property that already exists

[Peter Maydell]

Reject attempts to add a property to an object if one of
that name already exists. This is always a bug in the caller;
this is merely diagnosing it gracefully rather than behaving
oddly later.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This patch can't be applied until these two have been:
http://patchwork.ozlabs.org/patch/172885/
http://patchwork.ozlabs.org/patch/172820/
but I think we're probably going to argue about it anyway.

The question really is whether we want to treat attempts to add
a duplicate property as a programming bug (ie the calling code
should either know there are no duplicates or check first with
object_property_find) or whether we want to return a helpful
failure code from this function. In the code at the moment:
 * object_property_add() makes no attempt to cope with duplicate
   adds (they get added at the end of the list so will be
   never found on a subsequent search, and can't be dereferenced
   without first deleting the earlier of the two duplicates)
 * there's no attempt to handle failure-to-add in any of the
   utility helpers like object_property_add_child() (which
   will ref the child object regardless)
 * no caller of object_property_add() that I can find passes
   in anything except NULL for the Error** so if we were to
   do an error_set() here rather than assert() then the error
   just vanishes into the ether.
 * the documentation in object.h for these functions doesnt' define
   semantics for attempts to add duplicate properties

So in theory we could define some semantics (eg "return error
if property already exists"), define a new QERR_* constants since
as usual the existing cast of thousands of QERR_* constants are
unsuitable, fix all the callers to correctly handle and propagate
the error, make device_initfn() print an error, and so on.

But I think this patch is much simpler and more effective :-)


 qom/object.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 00bb3b0..dceabc0 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -659,7 +659,18 @@ void object_property_add(Object *obj, const char *name, const char *type,
                          ObjectPropertyRelease *release,
                          void *opaque, Error **errp)
 {
-    ObjectProperty *prop = g_malloc0(sizeof(*prop));
+    ObjectProperty *prop;
+
+    QTAILQ_FOREACH(prop, &obj->properties, node) {
+        if (strcmp(prop->name, name) == 0) {
+            /* This is always a bug in the caller */
+            fprintf(stderr, "attempt to set duplicate property %s on object\n",
+                    name);
+            assert(0);
+        }
+    }
+
+    prop = g_malloc0(sizeof(*prop));
 
     prop->name = g_strdup(name);
     prop->type = g_strdup(type);
-- 
1.7.5.4

Re: [Qemu-devel] [PATCH] qom: Reject attempts to add a property that already exists

[Paolo Bonzini]

Il 24/07/2012 16:30, Peter Maydell ha scritto:
> +    ObjectProperty *prop;
> +
> +    QTAILQ_FOREACH(prop, &obj->properties, node) {
> +        if (strcmp(prop->name, name) == 0) {
> +            /* This is always a bug in the caller */
> +            fprintf(stderr, "attempt to set duplicate property %s on object\n",
> +                    name);
> +            assert(0);

abort();

That's all I have to say. :)

Paolo

> +        }
> +    }
> +
> +    prop = g_malloc0(sizeof(*prop));