Re: [Qemu-devel] [PATCH v1 2/6] error: Functions to report warnings and informational messages

[Alistair Francis <alistair.francis@xilinx.com>]

On Fri, Jul 7, 2017 at 5:59 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Alistair Francis <alistair.francis@xilinx.com> writes:
>
>> Add warn_report(), warn_vreport() for reporting warnings, and
>> info_report(), info_vreport() for informational messages.
>>
>> These are implemented them with a helper function factored out of
>> error_vreport(), suitably generalized. As we don't regard error
>> messages as a stable API the original error messages now have an
>> 'error: ' prefix.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>
> The patch squashes two changes together: the new functions and the error
> message format change.  Please split it, so we can debate either change
> on its merit more easily, and to make them both properly visible in the
> commit log.

Ok,  I have split the patches.

>
>> ---
>> v1:
>>  - Don't expose the generic report and vreport() functions
>>  - Prefix error messages
>>  - Use vreport instead of qmsg_vreport()
>> RFC V3:
>>  - Change the function and enum names to be more descriptive
>>  - Add wrapper functions for *_report() and *_vreport()
>>
>>  include/qemu/error-report.h |  7 ++++
>>  scripts/checkpatch.pl       |  7 +++-
>>  util/qemu-error.c           | 78 +++++++++++++++++++++++++++++++++++++++++----
>>  3 files changed, 84 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
>> index 3001865896..e1c8ae1a52 100644
>> --- a/include/qemu/error-report.h
>> +++ b/include/qemu/error-report.h
>> @@ -35,8 +35,15 @@ void error_printf(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>>  void error_vprintf_unless_qmp(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
>>  void error_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>>  void error_set_progname(const char *argv0);
>> +
>>  void error_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
>> +void warn_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
>> +void info_vreport(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
>> +
>>  void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>> +void warn_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>> +void info_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>> +
>>  const char *error_get_progname(void);
>>  extern bool enable_timestamp_msg;
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 73efc927a9..1fdd7f624a 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -2534,8 +2534,13 @@ sub process {
>>                               error_set|
>>                               error_prepend|
>>                               error_reportf_err|
>> +                             vreport|
>
> Is this one needed?

No, none of the vreport() functions are needed. I have removed them.

>
>>                               error_vreport|
>> -                             error_report}x;
>> +                             warn_vreport|
>> +                             info_vreport|
>> +                             error_report|
>> +                             warn_report|
>> +                             info_report}x;
>>
>>       if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(.*\".*\\n/) {
>>               ERROR("Error messages should not contain newlines\n" . $herecurr);
>> diff --git a/util/qemu-error.c b/util/qemu-error.c
>> index 1c5e35ecdb..f2fc9d5a1e 100644
>> --- a/util/qemu-error.c
>> +++ b/util/qemu-error.c
>> @@ -14,6 +14,12 @@
>>  #include "monitor/monitor.h"
>>  #include "qemu/error-report.h"
>>
>> +typedef enum {
>> +    REPORT_TYPE_ERROR,
>> +    REPORT_TYPE_WARNING,
>> +    REPORT_TYPE_INFO,
>> +} report_type;
>> +
>>  void error_printf(const char *fmt, ...)
>>  {
>>      va_list ap;
>> @@ -179,17 +185,29 @@ static void print_loc(void)
>>
>>  bool enable_timestamp_msg;
>>  /*
>> - * Print an error message to current monitor if we have one, else to stderr.
>> + * Print a message to current monitor if we have one, else to stderr.
>
> Need a sentence on @report_type right here.  Perhaps
>
>     * @report_type is the type of message: error, warning or
>     * informational.
>
>>   * 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.
>>   */
>> -void error_vreport(const char *fmt, va_list ap)
>> +static void vreport(report_type type, const char *fmt, va_list ap)
>>  {
>>      GTimeVal tv;
>>      gchar *timestr;
>>
>> +    switch (type) {
>> +    case REPORT_TYPE_ERROR:
>> +        error_printf("error: ");
>> +        break;
>> +    case REPORT_TYPE_WARNING:
>> +        error_printf("warning: ");
>> +        break;
>> +    case REPORT_TYPE_INFO:
>> +        error_printf("info: ");
>> +        break;
>> +    }
>> +
>>      if (enable_timestamp_msg && !cur_mon) {
>>          g_get_current_time(&tv);
>>          timestr = g_time_val_to_iso8601(&tv);
>> @@ -204,16 +222,62 @@ void error_vreport(const char *fmt, va_list ap)
>>
>>  /*
>>   * Print an error message to current monitor if we have one, else to stderr.
>> - * Format arguments like sprintf().  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.
>> + */
>
> Please keep error_vreport()'s and error_report()'s function comments, so
> their contract is immediately visible when you jump to the function
> definition.
>
>> +void error_vreport(const char *fmt, va_list ap)
>> +{
>> +    vreport(REPORT_TYPE_ERROR, fmt, ap);
>> +}
>> +
>> +/*
>> + * Print a warning message to current monitor if we have one, else to stderr.
>> + */
>
> Repeat the full contract, or at least include it by reference, like
> "Works like error_vreport(), which see."  Same for the other new
> functions.

Fixed!

Thanks,
Alistair