Re: [Qemu-devel] [RFC PATCH 1/7] error reporting: Introduce errnoval parameter to vreport

[Eric Blake]

On 04/26/2018 11:53 AM, Ian Jackson wrote:
> This will allow new callers of vreport to specify that an errno value
> should be printed too.  Update all existing callers.
> 
> We use strerror rather than strerror_r because strerror_r presents
> portability difficulties.  Replacing strerror with strerror_r (or
> something else) is left to the future.

Is g_strerror() suitably easy to use, which would at least avoid the
portability difficulties?

> 
> No functional change yet.
> 
> Signed-off-by: Ian Jackson <address@hidden>
> ---
>  util/qemu-error.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/util/qemu-error.c b/util/qemu-error.c
> index a25d3b9..9acc4b5 100644
> --- a/util/qemu-error.c
> +++ b/util/qemu-error.c
> @@ -191,12 +191,14 @@ bool enable_timestamp_msg;
>  /*
>   * Print a message to current monitor if we have one, else to stderr.
>   * @report_type is the type of message: error, warning or informational.
> + * If @errnoval is nonnegative it is fed to strerror and printed too.

That implies 0 is fed to strerror(), which is not the case.  Better
would be "If @errnoval is positive...".  Or, if we wanted, we could say
"If @errnoval is not zero, its absolute value is fed to strerror" to let
people pass in both EIO and -EIO for the same output (something that
strerror() can't do, but our wrapper can) - but I don't know if that
convenience is a good thing.

>   * Format arguments like vsprintf().  The resulting message should be
>   * a single phrase, with no newline or trailing punctuation.
>   * Prepend the current location and append a newline.
>   * It's wrong to call this in a QMP monitor.  Use error_setg() there.
>   */
> -static void vreport(report_type type, const char *fmt, va_list ap)
> +static void vreport(report_type type, int errnoval,

Bikeshedding: Is 'err' or 'errval' a better name in terms of length and
legibility?

> +                    const char *fmt, va_list ap)
>  {
>      GTimeVal tv;
>      gchar *timestr;
> @@ -222,6 +224,11 @@ static void vreport(report_type type, const char *fmt, 
> va_list ap)
>      }
>  
>      error_vprintf(fmt, ap);
> +
> +    if (errnoval >= 0) {
> +        error_printf(": %s", strerror(errnoval));

Off-by-one.  You do NOT want to print strerror(0) (that results in
appending ": Success" or similar to what is supposed to be an error
message).

> +    }
> +
>      error_printf("\n");
>  }
>  
> @@ -234,7 +241,7 @@ static void vreport(report_type type, const char *fmt, 
> va_list ap)
>   */
>  void error_vreport(const char *fmt, va_list ap)
>  {
> -    vreport(REPORT_TYPE_ERROR, fmt, ap);
> +    vreport(REPORT_TYPE_ERROR, -1, fmt, ap);

Passing -1 to suppress the output feels awkward, especially if 0 can be
used for the same purpose and would then let us use -errno and errno
interchangeable by passing the absolute value to sterror.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature