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

[Ian Jackson]

Eric Blake writes ("Re: [RFC PATCH 1/7] error reporting: Introduce errnoval 
parameter to vreport"):
> On 04/26/2018 11:53 AM, Ian Jackson wrote:
> > 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?

Possibly.  I would prefer to leave that to a future cleanup so as not
to block the centralisation of the strerror calls...

> >   * 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.

But, 0 might be fed to strerror.  This is not normally deliberate, but
it does occor.  It is not unusual for people to write code which can
feed 0 to strerror.  strerror then conventionally returns "Error 0".

This is why I chose -1 as the sentinel value meaning "there is no
errno being passed".

> "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.

I think it is more important not to turn inintended situations where 0
would previously have been passed to strerror, into situations where
the errno value string simply vanishes.

> > -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?

`err' could be some kind of qemu-specific or library-specific error
code.  `errno' is the name for the specific error code space for Unix
errno.

> > +    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).

See above.

> > -    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.

I think no good can come of taking the absolute value.  The confusion
resulting from -errnoval is bad enough already IMO.

Regards,
Ian.