Re: [Qemu-devel] [RFC v2 2/3] qemu-error: Implement a more generic error reporting

[Markus Armbruster]

"Daniel P. Berrange" <address@hidden> writes:

> On Thu, Jun 29, 2017 at 12:42:38PM -0700, Alistair Francis wrote:
>> This patch removes the exisinting error_vreport() function and replaces it
>> with a more generic vreport() function that takes an enum describing the
>> information to be reported.

Why remove error_vreport()?

>> As part of this change a report() function is added as well with the
>> same capability.
>> 
>> To maintain full compatibility the original error_report() function is
>> maintained and no changes to the way errors are printed have been made.
>> 
>> Signed-off-by: Alistair Francis <address@hidden>
>> ---
>> 
>>  hw/virtio/virtio.c          |  2 +-
>>  include/qemu/error-report.h | 10 +++++++++-
>>  scripts/checkpatch.pl       |  3 ++-
>>  util/qemu-error.c           | 33 ++++++++++++++++++++++++++++++---
>>  4 files changed, 42 insertions(+), 6 deletions(-)
>> 
>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>> index 464947f76d..bd3d26abb7 100644
>> --- a/hw/virtio/virtio.c
>> +++ b/hw/virtio/virtio.c
>> @@ -2448,7 +2448,7 @@ void GCC_FMT_ATTR(2, 3) virtio_error(VirtIODevice 
>> *vdev, const char *fmt, ...)
>>      va_list ap;
>>  
>>      va_start(ap, fmt);
>> -    error_vreport(fmt, ap);
>> +    vreport(ERROR, fmt, ap);
>>      va_end(ap);
>>  
>>      if (virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
>> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
>> index 3001865896..39b554c3b9 100644
>> --- a/include/qemu/error-report.h
>> +++ b/include/qemu/error-report.h
>> @@ -21,6 +21,12 @@ typedef struct Location {
>>      struct Location *prev;
>>  } Location;
>>  
>> +typedef enum {
>> +    ERROR,
>> +    WARN,
>> +    INFO,
>> +} report_types;
>
> Woah, those are faaar to generic names to be used. There is way too much
> chance of those clashing with definitions from headers we pull in - 
> particularly
> windows which pollutes its system headers with loads of generic names.
>
> I'd suggest  QMSG_ERROR, QMSG_WARN, QMSG_INFO
>
>> +
>>  Location *loc_push_restore(Location *loc);
>>  Location *loc_push_none(Location *loc);
>>  Location *loc_pop(Location *loc);
>> @@ -30,12 +36,14 @@ void loc_set_none(void);
>>  void loc_set_cmdline(char **argv, int idx, int cnt);
>>  void loc_set_file(const char *fname, int lno);
>>  
>> +void vreport(report_types type, const char *fmt, va_list ap) 
>> GCC_FMT_ATTR(2, 0);
>> +void report(report_types type, const char *fmt, ...)  GCC_FMT_ATTR(2, 3);
>
> Those names are too generic too IMHO.  I'd suggest  qmsg_report, qmsg_vreport
>
> As mentioned in the previous review, there should be wrappers which
> call these with suitable enum to make usage less verbose. eg
>
>   qmsg_info(fmt, ....)  should call qmsg_report(QMSG_INFO, fmt, ...)
>   qmsg_vinfo(fmt, ....)  should call qmsg_vreport(QMSG_INFO, fmt, ...)
>
> likewise, for other message levels

We then have qmsg_warning() for warnings, and error_report() for errors.
Ugh!

If I had known back then what I know now, I wouldn't have used the
error_ prefix.

Naming things is hard.

Ideas anyone?

>>  void error_vprintf(const char *fmt, va_list ap) GCC_FMT_ATTR(1, 0);
>>  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 error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>>  const char *error_get_progname(void);
>>  extern bool enable_timestamp_msg;
>
> Regards,
> Daniel