| Message ID | 1347026144-15098-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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
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 > >
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 >> >>
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 >>> >>> > >
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
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
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
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);
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(-)