[RFC,5/7] error reporting: Provide error_report_errnoval (and error_vreport_errnoval)

This will let us replace more open coded calls to error_report and
strerror.

I have chosen to provide all of
   error_report_errno       error_vreport_errno
   error_report_errnoval    error_vreport_errnoval
because the former are much more common, and deserve a short spelling;
whereas there are still at least 30-40 potential callers of the latter.

No callers yet so no functional change.

Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 include/qemu/error-report.h |  5 +++++
 util/qemu-error.c           | 28 ++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

On 04/26/2018 11:53 AM, Ian Jackson wrote:
> This will let us replace more open coded calls to error_report and
> strerror.
> 
> I have chosen to provide all of
>    error_report_errno       error_vreport_errno
>    error_report_errnoval    error_vreport_errnoval
> because the former are much more common, and deserve a short spelling;
> whereas there are still at least 30-40 potential callers of the latter.

As mentioned in 2/7, that's inconsistent with error_setg_errno().  I'd
MUCH rather see us have JUST error_[v]report_errno with a mandatory
error parameter, rather than blindly relying on implicit use of errno;
it's not that much harder to write:

error_report_errno(errno, "fmt string...", args);

in the common case when directly using errno is intended.

>  
> +void error_vreport_errnoval(int errnoval,
> +                            const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
> +void error_report_errnoval(int errnoval,
> +                           const char *fmt, ...) GCC_FMT_ATTR(2, 3);

Bikeshedding - can we name the parameter 'os_error' (as in
error_seg_errno), or 'err' or 'errval', rather than the longer 'errnoval'?


> +++ b/util/qemu-error.c
> @@ -257,6 +257,18 @@ void error_vreport_errno(const char *fmt, va_list ap)
>  }
>  
>  /*
> + * Print an error message to current monitor if we have one, else to stderr.
> + * Format arguments like vsprintf().  The resulting message should be
> + * a single phrase, with no newline or trailing punctuation.
> + * Prepend the current location and append ": " strerror(errnoval) "\n".
> + * It's wrong to call this in a QMP monitor.  Use error_setg() there.
> + */
> +void error_vreport_errnoval(int errnoval, const char *fmt, va_list ap)
> +{
> +    vreport(REPORT_TYPE_ERROR, errnoval, fmt, ap);
> +}

Should this explicitly document that passing 0 for errnoval is
acceptable or forbidden?  If acceptable, does that mean no suffix (other
than \n) is added?  If forbidden, do we want to assert() that?

Eric Blake writes ("Re: [RFC PATCH 5/7] error reporting: Provide error_report_errnoval (and error_vreport_errnoval)"):
> On 04/26/2018 11:53 AM, Ian Jackson wrote:
> > I have chosen to provide all of
> >    error_report_errno       error_vreport_errno
> >    error_report_errnoval    error_vreport_errnoval
> > because the former are much more common, and deserve a short spelling;
> > whereas there are still at least 30-40 potential callers of the latter.
> 
> As mentioned in 2/7, that's inconsistent with error_setg_errno().

I didn't see error_setg_errno().  But I don't mind your alternative
approach, with the explicit errno, if that's what people prefer.  I
agree about the names of the functions, in that case.

> > +void error_vreport_errnoval(int errnoval,
> > +                            const char *fmt, va_list ap) GCC_FMT_ATTR(2, 0);
> > +void error_report_errnoval(int errnoval,
> > +                           const char *fmt, ...) GCC_FMT_ATTR(2, 3);
> 
> Bikeshedding - can we name the parameter 'os_error' (as in
> error_seg_errno), or 'err' or 'errval', rather than the longer 'errnoval'?

In speech, one always refers to these things as `errno values'.
`os_error' and `errnoval' are the same length of course.  See my other
mail about this particular bikeshed.

> Should this explicitly document that passing 0 for errnoval is
> acceptable or forbidden?  If acceptable, does that mean no suffix (other
> than \n) is added?  If forbidden, do we want to assert() that?

My intend was that passing 0 for errnoval is exactly as permitted or
forbidden as passing 0 to strerror.  I think that this is neatly
implied by the specification:

> > + * Prepend the current location and append ": " strerror(errnoval) "\n".

These functions already have a problem with excessive boilerplate in
their language.  I'm loathe to make that worse by being other than
terse.  I wouldn't mind a note somewhere else.

I would be open to asserting that the value is not negative, because
we do have some of the confusing -errno convention in some of qemu.
OTOH that turns a mishandling of -errno on an error path from a wrong
error message into a crash which is a bit unfortunate.

Ian.