"Daniel P. Berrange" <berra...@redhat.com> writes:
> On Mon, Jul 03, 2017 at 04:07:21PM +0200, Markus Armbruster wrote:
>> "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?
>
> I guess implicit in my suggestion would be to switch to qmsg_error()
> over some (long) period of time, but that would be a massive amount
> of churn that would harm backporting.
>
> So perhaps, just have error_report warning_report, info_report, and
> accept that the naming convention is slightly reversed from "normality"
That's not half bad.
The matching enum would be
typedef enum {
REPORT_TYPE_ERROR,
REPORT_TYPE_WARNING,
REPORT_TYPE_INFO,
} report_type;
Note the typedef name is *singular*. Compare
report_type rtype;
to
report_types rtype;
@rtype is *one* report type, not multiple.
Let's stick report_type, report() and vreport() into qemu-error.c
(static linkage) until we have a genuine need for them elsewhere.
