Patchwork [03/22] qapi: add Error object

login
register
mail settings
Submitter Anthony Liguori
Date March 7, 2011, 1:22 a.m.
Message ID <1299460984-15849-4-git-send-email-aliguori@us.ibm.com>
Download mbox | patch
Permalink /patch/85617/
State New
Headers show

Comments

Anthony Liguori - March 7, 2011, 1:22 a.m.
The Error class is similar to QError (now deprecated) except that it supports
propagation.  This allows for higher quality error handling.  It's losely
modeled after glib style GErrors.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
Daniel P. Berrange - March 7, 2011, 11:06 a.m.
On Sun, Mar 06, 2011 at 07:22:45PM -0600, Anthony Liguori wrote:
> The Error class is similar to QError (now deprecated) except that it supports
> propagation.  This allows for higher quality error handling.  It's losely
> modeled after glib style GErrors.

I know this offers more functionality than GError, but if we're going
to be using GLib IMHO it would be very desirable to just use GError
everywhere. Are the extra arbitary key,value pair fields really a
compelling enough addition to make us not use GError ? libvirt started
off with a very flexible error object that allowed extra string & int
values to be passed with each error, but after 5 years dev there is
not a single place in our code where we use this. Simple error code
and formatted strings have been sufficient, much like GError would
allow.

Regards,
Daniel
Stefan Hajnoczi - March 7, 2011, 11:38 a.m.
On Mon, Mar 7, 2011 at 1:22 AM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> +struct Error
> +{
> +    QDict *obj;
> +    const char *fmt;
> +    char *msg;
> +};

I wonder why fmt is const char * but msg is char *.  Users should use
error_get_pretty() instead of accessing msg directly and that function
returns const char * so it seems that msg should be const char * to
start with.

> +
> +void error_set(Error **errp, const char *fmt, ...)
> +{
> +    Error *err;
> +    va_list ap;
> +
> +    if (errp == NULL) {
> +        return;
> +    }
> +
> +    err = qemu_mallocz(sizeof(*err));
> +
> +    va_start(ap, fmt);
> +    err->obj = qobject_to_qdict(qobject_from_jsonv(fmt, &ap));

vsprintf() and friends pass va_list by value, they don't use a
pointer.  Perhaps you want to follow that idiom?

> +bool error_is_type(Error *err, const char *fmt)
> +{
> +    char *ptr;
> +    char *end;
> +    char classname[1024];
> +
> +    ptr = strstr(fmt, "'class': '");
> +    assert(ptr != NULL);
> +    ptr += strlen("'class': '");
> +
> +    end = strchr(ptr, '\'');
> +    assert(end != NULL);
> +
> +    memcpy(classname, ptr, (end - ptr));
> +    classname[(end - ptr)] = 0;
> +
> +    return strcmp(classname, error_get_field(err, "class")) == 0;

I'd get rid of the buffer/memcpy and use strncmp in-place instead:

const char *error_class = error_get_field(err, "class");
if (strlen(error_class) != end - ptr) {
    return false;
}
return strncmp(ptr, error_class, end - ptr) == 0;

Stefan
Anthony Liguori - March 7, 2011, 1:36 p.m.
On 03/07/2011 05:38 AM, Stefan Hajnoczi wrote:
> On Mon, Mar 7, 2011 at 1:22 AM, Anthony Liguori<aliguori@us.ibm.com>  wrote:
>    
>> +struct Error
>> +{
>> +    QDict *obj;
>> +    const char *fmt;
>> +    char *msg;
>> +};
>>      
> I wonder why fmt is const char * but msg is char *.  Users should use
> error_get_pretty() instead of accessing msg directly and that function
> returns const char * so it seems that msg should be const char * to
> start with.
>    

fmt doesn't need to be free'd whereas msg does.  If you make msg const 
char *, the compiler will complain when you pass that to qemu_free().  I 
tend to think of the difference between 'const char *' and 'char *' as a 
string that I don't own vs. a string that I do own the reference to.

It's not universally true but it tends to work nicely most of the time.

>> +
>> +void error_set(Error **errp, const char *fmt, ...)
>> +{
>> +    Error *err;
>> +    va_list ap;
>> +
>> +    if (errp == NULL) {
>> +        return;
>> +    }
>> +
>> +    err = qemu_mallocz(sizeof(*err));
>> +
>> +    va_start(ap, fmt);
>> +    err->obj = qobject_to_qdict(qobject_from_jsonv(fmt,&ap));
>>      
> vsprintf() and friends pass va_list by value, they don't use a
> pointer.  Perhaps you want to follow that idiom?
>    

This va_list is passed to a recursive decent parser.  The nature of 
va_list is such that if you pass by value, you cannot access it within a 
function after you've passed it to another function.  Passing by 
reference seems to fix this (at least with GCC).  I'm not 100% confident 
this is a strictly standards compliant solution but it's been working so 
far.  Note that this isn't introduecd by this series.

>> +bool error_is_type(Error *err, const char *fmt)
>> +{
>> +    char *ptr;
>> +    char *end;
>> +    char classname[1024];
>> +
>> +    ptr = strstr(fmt, "'class': '");
>> +    assert(ptr != NULL);
>> +    ptr += strlen("'class': '");
>> +
>> +    end = strchr(ptr, '\'');
>> +    assert(end != NULL);
>> +
>> +    memcpy(classname, ptr, (end - ptr));
>> +    classname[(end - ptr)] = 0;
>> +
>> +    return strcmp(classname, error_get_field(err, "class")) == 0;
>>      
> I'd get rid of the buffer/memcpy and use strncmp in-place instead:
>
> const char *error_class = error_get_field(err, "class");
> if (strlen(error_class) != end - ptr) {
>      return false;
> }
> return strncmp(ptr, error_class, end - ptr) == 0;
>    

Yeah, that's definitely better.

Regards,

Anthony Liguori

> Stefan
>
>
Stefan Hajnoczi - March 7, 2011, 1:55 p.m.
On Mon, Mar 7, 2011 at 1:36 PM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 03/07/2011 05:38 AM, Stefan Hajnoczi wrote:
>>
>> On Mon, Mar 7, 2011 at 1:22 AM, Anthony Liguori<aliguori@us.ibm.com>
>>  wrote:
>>
>>>
>>> +struct Error
>>> +{
>>> +    QDict *obj;
>>> +    const char *fmt;
>>> +    char *msg;
>>> +};
>>>
>>
>> I wonder why fmt is const char * but msg is char *.  Users should use
>> error_get_pretty() instead of accessing msg directly and that function
>> returns const char * so it seems that msg should be const char * to
>> start with.
>>
>
> fmt doesn't need to be free'd whereas msg does.  If you make msg const char
> *, the compiler will complain when you pass that to qemu_free().  I tend to
> think of the difference between 'const char *' and 'char *' as a string that
> I don't own vs. a string that I do own the reference to.
>
> It's not universally true but it tends to work nicely most of the time.

Thanks, that makes sense.

Stefan
Anthony Liguori - March 7, 2011, 1:59 p.m.
On 03/07/2011 05:06 AM, Daniel P. Berrange wrote:
> On Sun, Mar 06, 2011 at 07:22:45PM -0600, Anthony Liguori wrote:
>    
>> The Error class is similar to QError (now deprecated) except that it supports
>> propagation.  This allows for higher quality error handling.  It's losely
>> modeled after glib style GErrors.
>>      
> I know this offers more functionality than GError, but if we're going
> to be using GLib IMHO it would be very desirable to just use GError
> everywhere. Are the extra arbitary key,value pair fields really a
> compelling enough addition to make us not use GError ? libvirt started
> off with a very flexible error object that allowed extra string&  int
> values to be passed with each error, but after 5 years dev there is
> not a single place in our code where we use this. Simple error code
> and formatted strings have been sufficient, much like GError would
> allow.
>    

I wrote Error without even thinking of using GError.  My motivation was 
to move us off of QError.

We would lose key/value pairs which are actually quite useful.  Consider 
executing the qmp_cont() command after starting a guest with -S.  Right 
now, we'll throw a DeviceEncrypted error message that contains the drive 
name and encrypted filename which means a management app can immediately 
prompt the user for a password and then execute block_passwd.

Without key/value pairs, a management tool would need another round trip 
to do an info block and find out which devices are encrypted before 
doing the prompt.

I need to think a bit more about it.

Regards,

Anthony Liguori

> Regards,
> Daniel
>
Anthony Liguori - March 7, 2011, 2:24 p.m.
On 03/07/2011 05:06 AM, Daniel P. Berrange wrote:
> On Sun, Mar 06, 2011 at 07:22:45PM -0600, Anthony Liguori wrote:
>    
>> The Error class is similar to QError (now deprecated) except that it supports
>> propagation.  This allows for higher quality error handling.  It's losely
>> modeled after glib style GErrors.
>>      
> I know this offers more functionality than GError, but if we're going
> to be using GLib IMHO it would be very desirable to just use GError
> everywhere. Are the extra arbitary key,value pair fields really a
> compelling enough addition to make us not use GError ?

So here's the rational for not using GError (or really any message based 
Error object).

GError uses a domain, code, and message.  Since theses errors escape 
over the wire, the message part ends up not being very useful for clients.

There's no way to exhaustively enumerate all of the possible messages 
from the client's perspective which means there's no way to provide a 
localized version of the message.  That means that if a management tool 
wishes to expose these messages, they'd have to expose the English 
string which is extremely undesirable.

With the current Error design, a management tool can create a localized 
error message table to use to generate proper messages for end users.  
It can also make designs based on additional error content.

Using GError would basically devolve into having an error code.  It's 
doable, but if we were going to use an error code, I wouldn't want to 
have a custom message parameter and instead use a generated string.  My 
concern is that people will put critical information in the message 
parameter that a management tool is unable to use.  Worse yet, a 
management tool may end up parsing these strings and then we're stuck 
maintaining yet another format.

Just imagine the head ache of needed to worry about a formatted string 
that contains a file name and the file name contains special 
characters...  It's the monitor all over again.

Regards,

Anthony Liguori

> libvirt started
> off with a very flexible error object that allowed extra string&  int
> values to be passed with each error, but after 5 years dev there is
> not a single place in our code where we use this. Simple error code
> and formatted strings have been sufficient, much like GError would
> allow.
>
> Regards,
> Daniel
>

Patch

diff --git a/Makefile.objs b/Makefile.objs
index 0ba02c7..da31530 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -15,6 +15,7 @@  oslib-obj-$(CONFIG_POSIX) += oslib-posix.o
 
 block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o
 block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o
+block-obj-y += error.o
 block-obj-$(CONFIG_POSIX) += posix-aio-compat.o
 block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o
 
diff --git a/error.c b/error.c
new file mode 100644
index 0000000..5d84106
--- /dev/null
+++ b/error.c
@@ -0,0 +1,122 @@ 
+/*
+ * QEMU Error Objects
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.  See
+ * the COPYING.LIB file in the top-level directory.
+ */
+#include "error.h"
+#include "error_int.h"
+#include "qemu-objects.h"
+#include "qerror.h"
+#include <assert.h>
+
+struct Error
+{
+    QDict *obj;
+    const char *fmt;
+    char *msg;
+};
+
+void error_set(Error **errp, const char *fmt, ...)
+{
+    Error *err;
+    va_list ap;
+
+    if (errp == NULL) {
+        return;
+    }
+
+    err = qemu_mallocz(sizeof(*err));
+
+    va_start(ap, fmt);
+    err->obj = qobject_to_qdict(qobject_from_jsonv(fmt, &ap));
+    va_end(ap);
+    err->fmt = fmt;
+
+    *errp = err;
+}
+
+bool error_is_set(Error **errp)
+{
+    return (errp && *errp);
+}
+
+const char *error_get_pretty(Error *err)
+{
+    if (err->msg == NULL) {
+        QString *str;
+        str = qerror_format(err->fmt, err->obj);
+        err->msg = qemu_strdup(qstring_get_str(str));
+    }
+
+    return err->msg;
+}
+
+const char *error_get_field(Error *err, const char *field)
+{
+    if (strcmp(field, "class") == 0) {
+        return qdict_get_str(err->obj, field);
+    } else {
+        QDict *dict = qdict_get_qdict(err->obj, "data");
+        return qdict_get_str(dict, field);
+    }
+}
+
+void error_free(Error *err)
+{
+    QDECREF(err->obj);
+    qemu_free(err->msg);
+    qemu_free(err);
+}
+
+bool error_is_type(Error *err, const char *fmt)
+{
+    char *ptr;
+    char *end;
+    char classname[1024];
+
+    ptr = strstr(fmt, "'class': '");
+    assert(ptr != NULL);
+    ptr += strlen("'class': '");
+
+    end = strchr(ptr, '\'');
+    assert(end != NULL);
+    
+    memcpy(classname, ptr, (end - ptr));
+    classname[(end - ptr)] = 0;
+
+    return strcmp(classname, error_get_field(err, "class")) == 0;
+}
+
+void error_propagate(Error **dst_err, Error *local_err)
+{
+    if (dst_err) {
+        *dst_err = local_err;
+    } else if (local_err) {
+        error_free(local_err);
+    }
+}
+
+QObject *error_get_qobject(Error *err)
+{
+    QINCREF(err->obj);
+    return QOBJECT(err->obj);
+}
+
+void error_set_qobject(Error **errp, QObject *obj)
+{
+    Error *err;
+    if (errp == NULL) {
+        return;
+    }
+    err = qemu_mallocz(sizeof(*err));
+    err->obj = qobject_to_qdict(obj);
+    qobject_incref(obj);
+
+    *errp = err;
+}
diff --git a/error.h b/error.h
new file mode 100644
index 0000000..317d487
--- /dev/null
+++ b/error.h
@@ -0,0 +1,65 @@ 
+/*
+ * QEMU Error Objects
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.  See
+ * the COPYING.LIB file in the top-level directory.
+ */
+#ifndef ERROR_H
+#define ERROR_H
+
+#include <stdbool.h>
+
+/**
+ * A class representing internal errors within QEMU.  An error has a string
+ * typename and optionally a set of named string parameters.
+ */
+typedef struct Error Error;
+
+/**
+ * Set an indirect pointer to an error given a printf-style format parameter.
+ * Currently, qerror.h defines these error formats.  This function is not
+ * meant to be used outside of QEMU.
+ */
+void error_set(Error **err, const char *fmt, ...)
+    __attribute__((format(printf, 2, 3)));
+
+/**
+ * Returns true if an indirect pointer to an error is pointing to a valid
+ * error object.
+ */
+bool error_is_set(Error **err);
+
+/**
+ * Get a human readable representation of an error object.
+ */
+const char *error_get_pretty(Error *err);
+
+/**
+ * Get an individual named error field.
+ */
+const char *error_get_field(Error *err, const char *field);
+
+/**
+ * Propagate an error to an indirect pointer to an error.  This function will
+ * always transfer ownership of the error reference and handles the case where
+ * dst_err is NULL correctly.
+ */
+void error_propagate(Error **dst_err, Error *local_err);
+
+/**
+ * Free an error object.
+ */
+void error_free(Error *err);
+
+/**
+ * Determine if an error is of a speific type (based on the qerror format).
+ * Non-QEMU users should get the `class' field to identify the error type.
+ */
+bool error_is_type(Error *err, const char *fmt);
+
+#endif
diff --git a/error_int.h b/error_int.h
new file mode 100644
index 0000000..eaba65e
--- /dev/null
+++ b/error_int.h
@@ -0,0 +1,27 @@ 
+/*
+ * QEMU Error Objects
+ *
+ * Copyright IBM, Corp. 2011
+ *
+ * Authors:
+ *  Anthony Liguori   <aliguori@us.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.  See
+ * the COPYING.LIB file in the top-level directory.
+ */
+#ifndef QEMU_ERROR_INT_H
+#define QEMU_ERROR_INT_H
+
+#include "qemu-common.h"
+#include "qobject.h"
+#include "error.h"
+
+/**
+ * Internal QEMU functions for working with Error.
+ *
+ * These are used to convert QErrors to Errors
+ */
+QObject *error_get_qobject(Error *err);
+void error_set_qobject(Error **errp, QObject *obj);
+  
+#endif