[Qemu-devel] [PATCH v4 0/2] qdev: Detect duplicate device properties

[Qemu-devel] [PATCH v4 0/2] qdev: Detect duplicate device properties

[Peter Maydell]

So, v4 takes a different approach (again). The QOM level patch
should be pretty uncontroversial, it just reports the duplicate
via the Error**. Then in the qdev device_initfn we check for and
report errors via assert_no_error() rather than just throwing
them away. (In an ideal world maybe there'd be a way for the
initfn to report the error up yet another level, but not with
the current qdev API.)

This time for sure!

(I have a theory that passing NULL as an Error** is almost never
a good idea. It's a shame it's the path of least resistance for
using the API, it would be much better if the wrong thing was
the hard thing to do.)


Peter Maydell (2):
  qom: Detect attempts to add a property that already exists
  hw/qdev: Abort rather than ignoring errors adding device properties

 hw/qdev.c    |   10 +++++++---
 qom/object.c |   13 ++++++++++++-
 2 files changed, 19 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH v4 1/2] qom: Detect attempts to add a property that already exists

[Peter Maydell]

Detect attempts to add a property to an object if one of
that name already exists, and report them as critical
errors. In particular, for static properties (eg qdev
Property arrays) this will manifest as an abort() with
a useful error message.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 qom/object.c |   13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/qom/object.c b/qom/object.c
index e3e9242..a9dfc8c 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -620,7 +620,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) {
+            error_setg(errp, "attempt to add duplicate property '%s'"
+                       " to object (type '%s')\n", name,
+                       object_get_typename(obj));
+            return;
+        }
+    }
+
+    prop = g_malloc0(sizeof(*prop));
 
     prop->name = g_strdup(name);
     prop->type = g_strdup(type);
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v4 1/2] qom: Detect attempts to add a property that already exists

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> Detect attempts to add a property to an object if one of
> that name already exists, and report them as critical
> errors. In particular, for static properties (eg qdev
> Property arrays) this will manifest as an abort() with
> a useful error message.

What's critical about this error?  It looks like any other error to
me...  Leftover from previous iterations, perhaps?

Re: [Qemu-devel] [PATCH v4 1/2] qom: Detect attempts to add a property that already exists

[Peter Maydell]

On 25 October 2012 18:27, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Detect attempts to add a property to an object if one of
>> that name already exists, and report them as critical
>> errors. In particular, for static properties (eg qdev
>> Property arrays) this will manifest as an abort() with
>> a useful error message.
>
> What's critical about this error?  It looks like any other error to
> me...  Leftover from previous iterations, perhaps?

Rats, yes, I forgot to update the commit message. Should read:

Detect attempts to add a property to an object if one of
that name already exists, and report them as errors.

-- PMM

Re: [Qemu-devel] [PATCH v4 1/2] qom: Detect attempts to add a property that already exists

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> Detect attempts to add a property to an object if one of
> that name already exists, and report them as critical
> errors. In particular, for static properties (eg qdev
> Property arrays) this will manifest as an abort() with
> a useful error message.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  qom/object.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index e3e9242..a9dfc8c 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -620,7 +620,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) {
> +            error_setg(errp, "attempt to add duplicate property '%s'"
> +                       " to object (type '%s')\n", name,
> +                       object_get_typename(obj));
> +            return;
> +        }
> +    }
> +
> +    prop = g_malloc0(sizeof(*prop));
>  
>      prop->name = g_strdup(name);
>      prop->type = g_strdup(type);
> -- 
> 1.7.9.5

[Qemu-devel] [PATCH v4 2/2] hw/qdev: Abort rather than ignoring errors adding device properties

[Peter Maydell]

Instead of ignoring any errors that occur when adding properties
to a new device in device_initfn(), check for them and abort if any
occur. The most likely cause is accidentally adding a duplicate
property, which is a programming error by the device author.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/qdev.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/hw/qdev.c b/hw/qdev.c
index 9b9aba3..b34d80a 100644
--- a/hw/qdev.c
+++ b/hw/qdev.c
@@ -659,6 +659,7 @@ static void device_initfn(Object *obj)
     DeviceState *dev = DEVICE(obj);
     ObjectClass *class;
     Property *prop;
+    Error *err = NULL;
 
     if (qdev_hotplug) {
         dev->hotplugged = 1;
@@ -671,15 +672,18 @@ static void device_initfn(Object *obj)
     class = object_get_class(OBJECT(dev));
     do {
         for (prop = DEVICE_CLASS(class)->props; prop && prop->name; prop++) {
-            qdev_property_add_legacy(dev, prop, NULL);
-            qdev_property_add_static(dev, prop, NULL);
+            qdev_property_add_legacy(dev, prop, &err);
+            assert_no_error(err);
+            qdev_property_add_static(dev, prop, &err);
+            assert_no_error(err);
         }
         class = object_class_get_parent(class);
     } while (class != object_class_by_name(TYPE_DEVICE));
     qdev_prop_set_globals(dev);
 
     object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
-                             (Object **)&dev->parent_bus, NULL);
+                             (Object **)&dev->parent_bus, &err);
+    assert_no_error(err);
 }
 
 /* Unlink device from bus and free the structure.  */
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v4 2/2] hw/qdev: Abort rather than ignoring errors adding device properties

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> Instead of ignoring any errors that occur when adding properties
> to a new device in device_initfn(), check for them and abort if any
> occur. The most likely cause is accidentally adding a duplicate
> property, which is a programming error by the device author.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

> ---
>  hw/qdev.c |   10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/hw/qdev.c b/hw/qdev.c
> index 9b9aba3..b34d80a 100644
> --- a/hw/qdev.c
> +++ b/hw/qdev.c
> @@ -659,6 +659,7 @@ static void device_initfn(Object *obj)
>      DeviceState *dev = DEVICE(obj);
>      ObjectClass *class;
>      Property *prop;
> +    Error *err = NULL;
>  
>      if (qdev_hotplug) {
>          dev->hotplugged = 1;
> @@ -671,15 +672,18 @@ static void device_initfn(Object *obj)
>      class = object_get_class(OBJECT(dev));
>      do {
>          for (prop = DEVICE_CLASS(class)->props; prop && prop->name; prop++) {
> -            qdev_property_add_legacy(dev, prop, NULL);
> -            qdev_property_add_static(dev, prop, NULL);
> +            qdev_property_add_legacy(dev, prop, &err);
> +            assert_no_error(err);
> +            qdev_property_add_static(dev, prop, &err);
> +            assert_no_error(err);
>          }
>          class = object_class_get_parent(class);
>      } while (class != object_class_by_name(TYPE_DEVICE));
>      qdev_prop_set_globals(dev);
>  
>      object_property_add_link(OBJECT(dev), "parent_bus", TYPE_BUS,
> -                             (Object **)&dev->parent_bus, NULL);
> +                             (Object **)&dev->parent_bus, &err);
> +    assert_no_error(err);
>  }
>  
>  /* Unlink device from bus and free the structure.  */
> -- 
> 1.7.9.5