* [Qemu-devel] [PATCH 1/2] error: Distinguish critical and non-critical errors
2012-10-19 17:19 [Qemu-devel] [PATCH 0/2] qom: detect attempts to add a property that already exists Peter Maydell
@ 2012-10-19 17:19 ` Peter Maydell
2012-10-19 17:19 ` [Qemu-devel] [PATCH 2/2] qom: Detect attempts to add a property that already exists Peter Maydell
2012-10-22 15:35 ` [Qemu-devel] [PATCH 0/2] qom: detect " Luiz Capitulino
2 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2012-10-19 17:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Anthony Liguori, patches
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/2] qom: Detect attempts to add a property that already exists
2012-10-19 17:19 [Qemu-devel] [PATCH 0/2] qom: detect attempts to add a property that already exists Peter Maydell
2012-10-19 17:19 ` [Qemu-devel] [PATCH 1/2] error: Distinguish critical and non-critical errors Peter Maydell
@ 2012-10-19 17:19 ` Peter Maydell
2012-10-22 15:35 ` [Qemu-devel] [PATCH 0/2] qom: detect " Luiz Capitulino
2 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2012-10-19 17:19 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Anthony Liguori, patches
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 0/2] qom: detect attempts to add a property that already exists
2012-10-19 17:19 [Qemu-devel] [PATCH 0/2] qom: detect attempts to add a property that already exists Peter Maydell
2012-10-19 17:19 ` [Qemu-devel] [PATCH 1/2] error: Distinguish critical and non-critical errors Peter Maydell
2012-10-19 17:19 ` [Qemu-devel] [PATCH 2/2] qom: Detect attempts to add a property that already exists Peter Maydell
@ 2012-10-22 15:35 ` Luiz Capitulino
2012-10-22 16:26 ` Peter Maydell
2 siblings, 1 reply; 6+ messages in thread
From: Luiz Capitulino @ 2012-10-22 15:35 UTC (permalink / raw)
To: Peter Maydell; +Cc: Paolo Bonzini, Anthony Liguori, qemu-devel, patches
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(-)
>
^ permalink raw reply [flat|nested] 6+ messages in thread