Patchwork [01/18] error: add error_set_errno and error_setg_errno

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 3, 2012, 2:36 p.m.
Message ID <1349275025-5093-2-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/188788/
State New
Headers show

Comments

Paolo Bonzini - Oct. 3, 2012, 2:36 p.m.
These functions help maintaining homogeneous formatting of error
messages that include strerror values.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 error.c | 28 ++++++++++++++++++++++++++++
 error.h |  9 +++++++++
 2 file modificati, 37 inserzioni(+)
Luiz Capitulino - Oct. 4, 2012, 4:14 p.m.
On Wed,  3 Oct 2012 16:36:48 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> These functions help maintaining homogeneous formatting of error
> messages that include strerror values.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: Luiz Capitulino <lcapitulino@redhat.com>

One small comment below.

> ---
>  error.c | 28 ++++++++++++++++++++++++++++
>  error.h |  9 +++++++++
>  2 file modificati, 37 inserzioni(+)
> 
> diff --git a/error.c b/error.c
> index 1f05fc4..128d88c 100644
> --- a/error.c
> +++ b/error.c
> @@ -43,6 +43,34 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>      *errp = err;
>  }
>  
> +void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
> +                     const char *fmt, ...)

The function's name makes me expect that something else is done with os_errno
other than appending the strerror() string to the user message, but I can't
suggest anything better (and allows for storing errno in the Error object in
the future if we want to).

> +{
> +    Error *err;
> +    char *msg1;
> +    va_list ap;
> +
> +    if (errp == NULL) {
> +        return;
> +    }
> +    assert(*errp == NULL);
> +
> +    err = g_malloc0(sizeof(*err));
> +
> +    va_start(ap, fmt);
> +    msg1 = g_strdup_vprintf(fmt, ap);
> +    if (os_errno != 0) {
> +        err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));
> +        g_free(msg1);
> +    } else {
> +        err->msg = msg1;
> +    }
> +    va_end(ap);
> +    err->err_class = err_class;
> +
> +    *errp = err;
> +}
> +
>  Error *error_copy(const Error *err)
>  {
>      Error *err_new;
> diff --git a/error.h b/error.h
> index da7fed3..4d52e73 100644
> --- a/error.h
> +++ b/error.h
> @@ -30,10 +30,19 @@ typedef struct Error Error;
>  void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
>  
>  /**
> + * Set an indirect pointer to an error given a ErrorClass value and a
> + * printf-style human message, followed by a strerror() string if
> + * @os_error is not zero.
> + */
> +void error_set_errno(Error **err, int os_error, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(4, 5);
> +
> +/**
>   * Same as error_set(), but sets a generic error
>   */
>  #define error_setg(err, fmt, ...) \
>      error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
> +#define error_setg_errno(err, os_error, fmt, ...) \
> +    error_set_errno(err, os_error, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
>  
>  /**
>   * Returns true if an indirect pointer to an error is pointing to a valid
Paolo Bonzini - Oct. 4, 2012, 4:16 p.m.
Il 04/10/2012 18:14, Luiz Capitulino ha scritto:
>> > +void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>> > +                     const char *fmt, ...)
> 
> The function's name makes me expect that something else is done with os_errno
                                                     ^^^^

Why something "else"? :)

> other than appending the strerror() string to the user message, but I can't
> suggest anything better (and allows for storing errno in the Error object in
> the future if we want to).

Who knows... :)

Paolo
Luiz Capitulino - Oct. 4, 2012, 4:21 p.m.
On Thu, 04 Oct 2012 18:16:13 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 04/10/2012 18:14, Luiz Capitulino ha scritto:
> >> > +void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
> >> > +                     const char *fmt, ...)
> > 
> > The function's name makes me expect that something else is done with os_errno
>                                                      ^^^^
> 
> Why something "else"? :)

What I meant is that, it's not clear from the function's name how os_errno
is used. Actually, it gives the impression you're storing errno, but I might
be biased :)

But again, I don't have any better suggestions.
Markus Armbruster - Oct. 17, 2012, 12:47 p.m.
Luiz Capitulino <lcapitulino@redhat.com> writes:

> On Thu, 04 Oct 2012 18:16:13 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> Il 04/10/2012 18:14, Luiz Capitulino ha scritto:
>> >> > +void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>> >> > +                     const char *fmt, ...)
>> > 
>> > The function's name makes me expect that something else is done
>> > with os_errno
>>                                                      ^^^^
>> 
>> Why something "else"? :)
>
> What I meant is that, it's not clear from the function's name how os_errno
> is used. Actually, it gives the impression you're storing errno, but I might
> be biased :)
>
> But again, I don't have any better suggestions.

A bit long for my taste, but here goes anyway: error_set_with_errno()
Markus Armbruster - Oct. 17, 2012, 12:56 p.m.
Paolo Bonzini <pbonzini@redhat.com> writes:

> These functions help maintaining homogeneous formatting of error
> messages that include strerror values.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  error.c | 28 ++++++++++++++++++++++++++++
>  error.h |  9 +++++++++
>  2 file modificati, 37 inserzioni(+)
>
> diff --git a/error.c b/error.c
> index 1f05fc4..128d88c 100644
> --- a/error.c
> +++ b/error.c
> @@ -43,6 +43,34 @@ void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
>      *errp = err;
>  }
>  
> +void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
> +                     const char *fmt, ...)
> +{
> +    Error *err;
> +    char *msg1;
> +    va_list ap;
> +
> +    if (errp == NULL) {
> +        return;
> +    }
> +    assert(*errp == NULL);
> +
> +    err = g_malloc0(sizeof(*err));
> +
> +    va_start(ap, fmt);
> +    msg1 = g_strdup_vprintf(fmt, ap);
> +    if (os_errno != 0) {
> +        err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));
> +        g_free(msg1);
> +    } else {
> +        err->msg = msg1;
> +    }
> +    va_end(ap);
> +    err->err_class = err_class;
> +
> +    *errp = err;
> +}
> +

Duplicates error_set() code without need.  How about:

void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
                     const char *fmt, ...)
{
    char *msg;

    va_start(ap, fmt);
    msg = g_strdup_printf(fmt, ap);
    va_end(ap);
    if (os_errno) {
        error_set(errp, err_class, "%s: %s", msg, strerror(os_errno));
    } else {
        error_set(errp, err_class, "%s", msg);
    }
    g_free(msg);
}

Sketch, not even compile-tested.

Or the other way round, implement error_set() in terms of the more
general error_set_errno():

void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
{
    error_set(errp, err_class, 0, fmt, ...);
}

Except that's not C; real code needs verror_set_errno().

As usual, every variadic function sooner or later needs a buddy that
takes a va_list instead.

>  Error *error_copy(const Error *err)
>  {
>      Error *err_new;
[...]
Paolo Bonzini - Oct. 17, 2012, 1:03 p.m.
Il 17/10/2012 14:56, Markus Armbruster ha scritto:
> Duplicates error_set() code without need.  How about:
> 
> void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
>                      const char *fmt, ...)
> {
>     char *msg;
> 
>     va_start(ap, fmt);
>     msg = g_strdup_printf(fmt, ap);
>     va_end(ap);
>     if (os_errno) {
>         error_set(errp, err_class, "%s: %s", msg, strerror(os_errno));
>     } else {
>         error_set(errp, err_class, "%s", msg);
>     }
>     g_free(msg);
> }
> 
> Sketch, not even compile-tested.
> 
> Or the other way round, implement error_set() in terms of the more
> general error_set_errno():
> 
> void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
> {
>     error_set(errp, err_class, 0, fmt, ...);
> }
> 
> Except that's not C; real code needs verror_set_errno().
> 
> As usual, every variadic function sooner or later needs a buddy that
> takes a va_list instead.

Indeed... lazy me.

Added to my todo list.

Paolo

Patch

diff --git a/error.c b/error.c
index 1f05fc4..128d88c 100644
--- a/error.c
+++ b/error.c
@@ -43,6 +43,34 @@  void error_set(Error **errp, ErrorClass err_class, const char *fmt, ...)
     *errp = err;
 }
 
+void error_set_errno(Error **errp, int os_errno, ErrorClass err_class,
+                     const char *fmt, ...)
+{
+    Error *err;
+    char *msg1;
+    va_list ap;
+
+    if (errp == NULL) {
+        return;
+    }
+    assert(*errp == NULL);
+
+    err = g_malloc0(sizeof(*err));
+
+    va_start(ap, fmt);
+    msg1 = g_strdup_vprintf(fmt, ap);
+    if (os_errno != 0) {
+        err->msg = g_strdup_printf("%s: %s", msg1, strerror(os_errno));
+        g_free(msg1);
+    } else {
+        err->msg = msg1;
+    }
+    va_end(ap);
+    err->err_class = err_class;
+
+    *errp = err;
+}
+
 Error *error_copy(const Error *err)
 {
     Error *err_new;
diff --git a/error.h b/error.h
index da7fed3..4d52e73 100644
--- a/error.h
+++ b/error.h
@@ -30,10 +30,19 @@  typedef struct Error Error;
 void error_set(Error **err, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(3, 4);
 
 /**
+ * Set an indirect pointer to an error given a ErrorClass value and a
+ * printf-style human message, followed by a strerror() string if
+ * @os_error is not zero.
+ */
+void error_set_errno(Error **err, int os_error, ErrorClass err_class, const char *fmt, ...) GCC_FMT_ATTR(4, 5);
+
+/**
  * Same as error_set(), but sets a generic error
  */
 #define error_setg(err, fmt, ...) \
     error_set(err, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
+#define error_setg_errno(err, os_error, fmt, ...) \
+    error_set_errno(err, os_error, ERROR_CLASS_GENERIC_ERROR, fmt, ## __VA_ARGS__)
 
 /**
  * Returns true if an indirect pointer to an error is pointing to a valid