"Daniel P. Berrange" <berra...@redhat.com> 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 <alistair.fran...@xilinx.com>
>> ---
>>
>> 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
