[Qemu-devel] [PATCH 0/2] qom: detect attempts to add a property that already exists

[Qemu-devel] [PATCH 0/2] qom: detect attempts to add a property that already exists

[Peter Maydell]

The aim of this patch series is to make QEMU exit with a helpful
error message for bugs where multiple properties of the same name
are accidentally added to a QOM object. In order to achieve this
for static properties whilst still allowing the hotplug case
to gracefully fail without killing QEMU, we add the concept
of a 'critical' error. A critical error is one which must be
handled somehow -- if we encounter a NULL Error** either when
the error is raised or later when it is propagated, we will
abort() rather than throwing the error away.

(This is a bit different from the idea we initially discussed on
IRC, which was to have an error_propagate_or_abort() function,
but that approach would mean that the error would get thrown
away by intermediate functions such as qdev_property_add_static()
unless all those functions were changed too. I think this way
is more reliable.)

Peter Maydell (2):
  error: Distinguish critical and non-critical errors
  qom: Detect attempts to add a property that already exists

 error.c      |   38 ++++++++++++++++++++++++++++++++++----
 error.h      |   12 ++++++++++++
 qom/object.c |   13 ++++++++++++-
 3 files changed, 58 insertions(+), 5 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/2] error: Distinguish critical and non-critical errors

[Peter Maydell]

Add the concept of a 'critical' error, which is one that must not
be ignored. If, at the point when the error is raised or at any
subsequent point while propagating it, we find that we would be
throwing away the error because of a NULL Error**, we print
the error message to stderr and abort().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 error.c |   38 ++++++++++++++++++++++++++++++++++----
 error.h |   12 ++++++++++++
 2 files changed, 46 insertions(+), 4 deletions(-)

diff --git a/error.c b/error.c
index 1f05fc4..3f76fd5 100644
--- a/error.c
+++ b/error.c
@@ -21,12 +21,13 @@ struct Error
 {
     char *msg;
     ErrorClass err_class;
+    bool is_critical;
 };
 
-void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
+static void do_error_set(Error **errp, ErrorClass err_class,
+                         const char *fmt, va_list ap)
 {
     Error *err;
-    va_list ap;
 
     if (errp == NULL) {
         return;
@@ -35,14 +36,38 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
 
     err = g_malloc0(sizeof(*err));
 
-    va_start(ap, fmt);
     err->msg = g_strdup_vprintf(fmt, ap);
-    va_end(ap);
     err->err_class = err_class;
 
     *errp = err;
 }
 
+void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
+{
+    va_list ap;
+    va_start(ap, fmt);
+    do_error_set(errp, err_class, fmt, ap);
+    va_end(ap);
+}
+
+void error_set_critical(Error **errp, ErrorClass err_class,
+                        const char *fmt, ...)
+{
+    va_list ap;
+    va_start(ap, fmt);
+    if (!errp) {
+        /* Critical error which would be ignored: print and abort now */
+        vfprintf(stderr, fmt, ap);
+        fputc('\n', stderr);
+        abort();
+    }
+
+    do_error_set(errp, err_class, fmt, ap);
+    (*errp)->is_critical = true;
+
+    va_end(ap);
+}
+
 Error *error_copy(const Error *err)
 {
     Error *err_new;
@@ -50,6 +75,7 @@ Error *error_copy(const Error *err)
     err_new = g_malloc0(sizeof(*err));
     err_new->msg = g_strdup(err->msg);
     err_new->err_class = err->err_class;
+    err_new->is_critical = err->is_critical;
 
     return err_new;
 }
@@ -82,6 +108,10 @@ void error_propagate(Error **dst_err, Error *local_err)
     if (dst_err && !*dst_err) {
         *dst_err = local_err;
     } else if (local_err) {
+        if (local_err->is_critical) {
+            fprintf(stderr, "%s\n", error_get_pretty(local_err));
+            abort();
+        }
         error_free(local_err);
     }
 }
diff --git a/error.h b/error.h
index da7fed3..4be0893 100644
--- a/error.h
+++ b/error.h
@@ -36,6 +36,18 @@ void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_
     error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
 
 /**
+ * Same as error_set(), but mark the error as critical
+ */
+void error_set_critical(Error **err, ErrorClass err_class,
+                        const char *fmt, ...) GCC_FMT_ATTR(3, 4);
+
+/**
+ * Same as error_setg(), but mark the error as critical
+ */
+#define error_setg_critical(err, fmt, ...) \
+    error_set_critical(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
+
+/**
  * Returns true if an indirect pointer to an error is pointing to a valid
  * error object.
  */
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/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..228ca92 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_critical(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 0/2] qom: detect attempts to add a property that already exists

[Luiz Capitulino]

On Fri, 19 Oct 2012 18:19:04 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> The aim of this patch series is to make QEMU exit with a helpful
> error message for bugs where multiple properties of the same name
> are accidentally added to a QOM object. 

Does this happen only at build-time or can it happen at command-line
too? What about QMP/HMP?

> In order to achieve this
> for static properties whilst still allowing the hotplug case
> to gracefully fail without killing QEMU, we add the concept
> of a 'critical' error. A critical error is one which must be
> handled somehow -- if we encounter a NULL Error** either when
> the error is raised or later when it is propagated, we will
> abort() rather than throwing the error away.

This gives me the impression that we're fixing it in the wrong layer.
Besides, all code calling error_propagate() today can now abort
(at least in theory), but that's something we really don't want to happen
in QMP.

An alternative would be to let users set is_critical, but add a
error_is_critical() function and let the code that wants to abort
to check for it.

But, how difficult it's to add a flag to QPM objects to allow/disallow
multiple properties?

> 
> (This is a bit different from the idea we initially discussed on
> IRC, which was to have an error_propagate_or_abort() function,
> but that approach would mean that the error would get thrown
> away by intermediate functions such as qdev_property_add_static()
> unless all those functions were changed too. I think this way
> is more reliable.)
> 
> Peter Maydell (2):
>   error: Distinguish critical and non-critical errors
>   qom: Detect attempts to add a property that already exists
> 
>  error.c      |   38 ++++++++++++++++++++++++++++++++++----
>  error.h      |   12 ++++++++++++
>  qom/object.c |   13 ++++++++++++-
>  3 files changed, 58 insertions(+), 5 deletions(-)
> 

Re: [Qemu-devel] [PATCH 0/2] qom: detect attempts to add a property that already exists

[Peter Maydell]

On 22 October 2012 16:35, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> wrote:
>
>> The aim of this patch series is to make QEMU exit with a helpful
>> error message for bugs where multiple properties of the same name
>> are accidentally added to a QOM object.
>
> Does this happen only at build-time or can it happen at command-line
> too? What about QMP/HMP?

Anything that cares about not abort()ing needs to pass a valid
Error** in.

>> In order to achieve this
>> for static properties whilst still allowing the hotplug case
>> to gracefully fail without killing QEMU, we add the concept
>> of a 'critical' error. A critical error is one which must be
>> handled somehow -- if we encounter a NULL Error** either when
>> the error is raised or later when it is propagated, we will
>> abort() rather than throwing the error away.
>
> This gives me the impression that we're fixing it in the wrong layer.
> Besides, all code calling error_propagate() today can now abort
> (at least in theory), but that's something we really don't want to happen
> in QMP.

That's why QMP gets to pass in an Error ** and handle the result.

I'm open to better ways to handle this. Perhaps all Errors should
be critical? :-)

Mostly what this patch is trying to do is deal with the fact that
huge amounts of code using the Error interface just throws away
the error. I'd rather not have lots of boilerplate at the device
and board model level that's just sitting there asserting "this
will always succeed". So instead we say "if you pass in a NULL
Error ** then you are saying that you know this won't fail".

> An alternative would be to let users set is_critical, but add a
> error_is_critical() function and let the code that wants to abort
> to check for it.
>
> But, how difficult it's to add a flag to QPM objects to allow/disallow
> multiple properties?

Why would you want to have multiple properties with the same name?
It's not meaningful because property lookup is by name so there's
no way to distinguish them. It's always an error, either in QEMU
itself or an error by the user on the command line.

-- PMM

Re: [Qemu-devel] [PATCH 0/2] qom: detect attempts to add a property that already exists

[Luiz Capitulino]

On Mon, 22 Oct 2012 17:26:10 +0100
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 22 October 2012 16:35, Luiz Capitulino <lcapitulino@redhat.com> wrote:
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> >
> >> The aim of this patch series is to make QEMU exit with a helpful
> >> error message for bugs where multiple properties of the same name
> >> are accidentally added to a QOM object.
> >
> > Does this happen only at build-time or can it happen at command-line
> > too? What about QMP/HMP?
> 
> Anything that cares about not abort()ing needs to pass a valid
> Error** in.

That's not clear for a function called error_propagate(). Besides, we've
been using error_propagate() without assuming it could abort() for some
time now, I really don't feel this is safe to do.

> >> In order to achieve this
> >> for static properties whilst still allowing the hotplug case
> >> to gracefully fail without killing QEMU, we add the concept
> >> of a 'critical' error. A critical error is one which must be
> >> handled somehow -- if we encounter a NULL Error** either when
> >> the error is raised or later when it is propagated, we will
> >> abort() rather than throwing the error away.
> >
> > This gives me the impression that we're fixing it in the wrong layer.
> > Besides, all code calling error_propagate() today can now abort
> > (at least in theory), but that's something we really don't want to happen
> > in QMP.
> 
> That's why QMP gets to pass in an Error ** and handle the result.
> 
> I'm open to better ways to handle this. Perhaps all Errors should
> be critical? :-)

What about moving the decision to abort or not abort to the call sites
instead? Ie. you add the is_critical bool plus error_is_critical(), but
drop the automatic abort? Or add error_abort_if_critical().

> Mostly what this patch is trying to do is deal with the fact that
> huge amounts of code using the Error interface just throws away
> the error.

It's up to the caller of a function taking an Error ** object to
decide whether or not to ignore an error. If certain callers choose to
do so, than I can only assume that that was the correct behavior chosen
by whom wrote the code.

If this turns out not be right, than randomly aborting is not the
right thing to do either. My suggestion is to either fix the code
paths not to ignore errors or move the abort() to the call sites
where aborting is the right thing to do.