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.
