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

[Qemu-devel] [PATCH v2] 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>
---
Changes v1->v2:
 * use abort() rather than assert(0), as suggested by Paolo
 * make the diagnostic message a little more helpful by
   including the type name, and adding '' around names
 * the patches fixing bugs which this patch makes fatal errors
   have both now been committed to master, so there's no
   barrier to committing it now

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

diff --git a/qom/object.c b/qom/object.c
index e3e9242..3da4c0e 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) {
+            /* This is always a bug in the caller */
+            fprintf(stderr, "attempt to add duplicate property '%s'"
+                    " to object (type '%s')\n", name, object_get_typename(obj));
+            abort();
+        }
+    }
+
+    prop = g_malloc0(sizeof(*prop));
 
     prop->name = g_strdup(name);
     prop->type = g_strdup(type);
-- 
1.7.9.5

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

[Andreas Färber]

Am 07.09.2012 15:55, schrieb 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>

Looks fine to me,

Reviewed-by: Andreas Färber <afaerber@suse.de>

/-F

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

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

[Peter Maydell]

Ping!

-- PMM

On 7 September 2012 14:55, Peter Maydell <peter.maydell@linaro.org> wrote:
> 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>
> ---
> Changes v1->v2:
>  * use abort() rather than assert(0), as suggested by Paolo
>  * make the diagnostic message a little more helpful by
>    including the type name, and adding '' around names
>  * the patches fixing bugs which this patch makes fatal errors
>    have both now been committed to master, so there's no
>    barrier to committing it now
>
>  qom/object.c |   13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/qom/object.c b/qom/object.c
> index e3e9242..3da4c0e 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) {
> +            /* This is always a bug in the caller */
> +            fprintf(stderr, "attempt to add duplicate property '%s'"
> +                    " to object (type '%s')\n", name, object_get_typename(obj));
> +            abort();
> +        }
> +    }
> +
> +    prop = g_malloc0(sizeof(*prop));
>
>      prop->name = g_strdup(name);
>      prop->type = g_strdup(type);
> --
> 1.7.9.5
>
>

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

[Anthony Liguori]

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

> Ping!

This is wrong.

Container properties are added by the user.  You will turn a gracefully
failure (during hotplug) into an abort().

Please limit this to static properties as they are not added by a user.

Regards,

Anthony Liguori

>
> -- PMM
>
> On 7 September 2012 14:55, Peter Maydell <peter.maydell@linaro.org> wrote:
>> 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>
>> ---
>> Changes v1->v2:
>>  * use abort() rather than assert(0), as suggested by Paolo
>>  * make the diagnostic message a little more helpful by
>>    including the type name, and adding '' around names
>>  * the patches fixing bugs which this patch makes fatal errors
>>    have both now been committed to master, so there's no
>>    barrier to committing it now
>>
>>  qom/object.c |   13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index e3e9242..3da4c0e 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) {
>> +            /* This is always a bug in the caller */
>> +            fprintf(stderr, "attempt to add duplicate property '%s'"
>> +                    " to object (type '%s')\n", name, object_get_typename(obj));
>> +            abort();
>> +        }
>> +    }
>> +
>> +    prop = g_malloc0(sizeof(*prop));
>>
>>      prop->name = g_strdup(name);
>>      prop->type = g_strdup(type);
>> --
>> 1.7.9.5
>>
>>

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

[Peter Crosthwaite]

On Mon, Oct 8, 2012 at 11:29 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> Ping!
>
> This is wrong.
>
> Container properties are added by the user.  You will turn a gracefully
> failure (during hotplug) into an abort().
>

Can we just populate errp with a nice meaningful error (perhaps the
contents of that printf), then the caller can decide if failure is
tolerable?

Regards,
Peter

> Please limit this to static properties as they are not added by a user.
>
> Regards,
>
> Anthony Liguori
>
>>
>> -- PMM
>>
>> On 7 September 2012 14:55, Peter Maydell <peter.maydell@linaro.org> wrote:
>>> 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>
>>> ---
>>> Changes v1->v2:
>>>  * use abort() rather than assert(0), as suggested by Paolo
>>>  * make the diagnostic message a little more helpful by
>>>    including the type name, and adding '' around names
>>>  * the patches fixing bugs which this patch makes fatal errors
>>>    have both now been committed to master, so there's no
>>>    barrier to committing it now
>>>
>>>  qom/object.c |   13 ++++++++++++-
>>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qom/object.c b/qom/object.c
>>> index e3e9242..3da4c0e 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) {
>>> +            /* This is always a bug in the caller */
>>> +            fprintf(stderr, "attempt to add duplicate property '%s'"
>>> +                    " to object (type '%s')\n", name, object_get_typename(obj));
>>> +            abort();
>>> +        }
>>> +    }
>>> +
>>> +    prop = g_malloc0(sizeof(*prop));
>>>
>>>      prop->name = g_strdup(name);
>>>      prop->type = g_strdup(type);
>>> --
>>> 1.7.9.5
>>>
>>>
>
>

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

[Peter Maydell]

On 8 October 2012 14:38, Peter Crosthwaite
<peter.crosthwaite@petalogix.com> wrote:
> Can we just populate errp with a nice meaningful error (perhaps the
> contents of that printf), then the caller can decide if failure is
> tolerable?

I would find this approach more plausible if it wasn't that so many
places in qemu just happily pass NULL as an errp.

-- PMM

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

[Peter Maydell]

On 8 October 2012 14:29, Anthony Liguori <aliguori@us.ibm.com> wrote:
> This is wrong.
>
> Container properties are added by the user.  You will turn a gracefully
> failure (during hotplug) into an abort().

No, it's turning a bug into an abort -- we don't handle trying to
create two identically named properties correctly today.

> Please limit this to static properties as they are not added by a user.

Adding two dynamic properties of the same name is also not
going to work and we need to do something with it...

What is the code path for properties being added by a user?
If it's qdev_device_add() then that code presumably doesn't
care about graceful failures because it passes NULL as an
error pointer. container_get() seems to assume that adding the
child property will always succeed and will not do the right
thing if there already exists a child property of the relevant
name but wrong type.

Basically it seems to me that any code which might actually
be hit by this assert() rather needs examination and rewriting
to handle the error case anyway...

-- PMM

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

[Anthony Liguori]

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

> On 8 October 2012 14:29, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> This is wrong.
>>
>> Container properties are added by the user.  You will turn a gracefully
>> failure (during hotplug) into an abort().
>
> No, it's turning a bug into an abort -- we don't handle trying to
> create two identically named properties correctly today.

Killing a guest because of something a user mistypes is not very friendly.

>
>> Please limit this to static properties as they are not added by a user.
>
> Adding two dynamic properties of the same name is also not
> going to work and we need to do something with it...

Raise an error.

> What is the code path for properties being added by a user?

qdev_device_add().

> If it's qdev_device_add() then that code presumably doesn't
> care about graceful failures because it passes NULL as an
> error pointer.

Then we should handle the error there gracefully.

> container_get() seems to assume that adding the
> child property will always succeed and will not do the right
> thing if there already exists a child property of the relevant
> name but wrong type.
>
> Basically it seems to me that any code which might actually
> be hit by this assert() rather needs examination and rewriting
> to handle the error case anyway...

There are only two cases that actually matter today:

1) static properties

2) qdev_device_add().

Yes, (2) is not doign error checking today.  It should.  I would be very
happy with an abort() in (1) since that's clearly a programming bug.

Regards,

Anthony Liguori

>
> -- PMM