[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC v2 2/3] qemu-error: Implement a more generic error
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [RFC v2 2/3] qemu-error: Implement a more generic error reporting |
Date: |
Mon, 3 Jul 2017 15:15:37 +0100 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Mon, Jul 03, 2017 at 04:07:21PM +0200, Markus Armbruster wrote:
> "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?
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"
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|