| Message ID | 1349275025-5093-2-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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.
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()
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; [...]
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
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
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(+)