[RFC,1/7] error reporting: Introduce errnoval parameter to vreport

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.

No functional change yet.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 util/qemu-error.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

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 <Ian.Jackson@eu.citrix.com>
> ---
>  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 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.

On 04/26/2018 12:32 PM, Ian Jackson wrote:

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

No, POSIX requires:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/strerror.html
"if the value of errnum is zero, the message string shall either be an
empty string or indicate that no error occurred;"

and at least in glibc, strerror(0) returns "Success", which looks dumb
in an error message.

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

Passing 0 as an errno value to indicate an error is almost always wrong.
But if you don't have an errno value to report, having the error string
disappear is preferable to outputting garbage or even assert()ing that
the error value was nonzero.

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

You are correct that in the past, we've had code passing or returning
the wrong sign without realizing it.  So the question is whether, at
least for reporting purposes, it looks better to report "Operation not
permitted" instead of "Unknown error -1", or whether munging the error
message as a convenience for better output makes it harder for the
programmer to realize that they are returning the wrong sign up the
stack to the caller.

On 04/26/2018 12:24 PM, Eric Blake wrote:

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

> 
> 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'm also using, as my reference, the glibc 'man 3 error' function, which
specifically documents:

  if  errnum  is  nonzero,  a second colon and a space followed by the
string
       given by strerror(errnum).

making it a very versatile function for outputting messages to stderr
whether or not you also have an errno value to convert via strerror(),
by using 0 (not -1) as the sentinel value.

Consistency argues that even if we are going to invent our own qemu
functions, we're better off sticking to well-known paradigms already in
the wild (such as 0 to suppress appending strerror()) rather than making
readers learn yet another paradigm.

Eric Blake writes ("Re: [RFC PATCH 1/7] error reporting: Introduce errnoval parameter to vreport"):
> On 04/26/2018 12:32 PM, Ian Jackson wrote:
> > 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".
> 
> No, POSIX requires:
> http://pubs.opengroup.org/onlinepubs/9699919799/functions/strerror.html
> "if the value of errnum is zero, the message string shall either be an
> empty string or indicate that no error occurred;"
> 
> and at least in glibc, strerror(0) returns "Success", which looks dumb
> in an error message.

I agree that it looks dumb.  About as dumb as "Error 0".

> > 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.
> 
> Passing 0 as an errno value to indicate an error is almost always wrong.
> But if you don't have an errno value to report, having the error string
> disappear is preferable to outputting garbage or even assert()ing that
> the error value was nonzero.

"Success" is not garbage.

> You are correct that in the past, we've had code passing or returning
> the wrong sign without realizing it.  So the question is whether, at
> least for reporting purposes, it looks better to report "Operation not
> permitted" instead of "Unknown error -1", or whether munging the error
> message as a convenience for better output makes it harder for the
> programmer to realize that they are returning the wrong sign up the
> stack to the caller.

I think in both the above bug cases it is preferable to continue to
print an errno value message, and to print one which indicates that
there is a bug in the program's error handling.

In ordinary code it is not common to want to print an error message
with an error code which is an augmented errno value type containing a
sentinel.  Using 0 as a sentinel for this purpose would mostly
disguise bugs rather than be a convenience.

If you disapprove of the use of -1 as the internal sentinel because of
the possibility of -errno confusion bugs, we could use INT_MIN as the
sentinel.

Ian.