diff mbox

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

Message ID 0b14290d85eceaa145e0333c26826b14304e23e4.1498761179.git.alistair.francis@xilinx.com (mailing list archive)
State New, archived
Headers show

Commit Message

Alistair Francis June 29, 2017, 7:42 p.m. UTC
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.

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.francis@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(-)

Comments

Eric Blake June 29, 2017, 8:47 p.m. UTC | #1
On 06/29/2017 02:42 PM, Alistair Francis wrote:
> This patch removes the exisinting error_vreport() function and replaces it

s/exisinting/existing/

> with a more generic vreport() function that takes an enum describing the
> information to be reported.
> 
> 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.francis@xilinx.com>
> ---
> 

> -void error_vreport(const char *fmt, va_list ap)
> +void vreport(report_types type, const char *fmt, va_list ap)
>  {
>      GTimeVal tv;
>      gchar *timestr;
>  
> +    switch (type) {
> +    case ERROR:
> +        /* To maintin compatibility we don't add anything here */

s/maintin/maintain/
Daniel P. Berrangé June 30, 2017, 8:54 a.m. UTC | #2
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.
> 
> 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.francis@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

>  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
Markus Armbruster July 3, 2017, 2:07 p.m. UTC | #3
"Daniel P. Berrange" <berrange@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.francis@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
Daniel P. Berrangé July 3, 2017, 2:15 p.m. UTC | #4
On Mon, Jul 03, 2017 at 04:07:21PM +0200, Markus Armbruster wrote:
> "Daniel P. Berrange" <berrange@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.francis@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"


Regards,
Daniel
Markus Armbruster July 4, 2017, 6:53 a.m. UTC | #5
"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Mon, Jul 03, 2017 at 04:07:21PM +0200, Markus Armbruster wrote:
>> "Daniel P. Berrange" <berrange@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.francis@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.
Alistair Francis July 5, 2017, 3:56 p.m. UTC | #6
On Mon, Jul 3, 2017 at 11:53 PM, Markus Armbruster <armbru@redhat.com> wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
>
>> On Mon, Jul 03, 2017 at 04:07:21PM +0200, Markus Armbruster wrote:
>>> "Daniel P. Berrange" <berrange@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()?

I just thought that the name didn't make sense now that we are using
it for info and warning reports as well.

>>>
>>> >> 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.francis@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

Ok, I was trying to make them as short as possible to allow less line
wrapping, but I agree report() is too generic.

>>> >
>>> > 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.

Ok, I'll send out another version later today then.

Thanks,
Alistair
diff mbox

Patch

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;
+
 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);
+
 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;
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 45027b9281..4a3074e758 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2530,7 +2530,8 @@  sub process {
 				error_set|
 				error_prepend|
 				error_reportf_err|
-				error_vreport|
+				vreport|
+				report|
 				error_report}x;
 
 	if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(.*\".*\\n/) {
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 1c5e35ecdb..83206532dd 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -179,17 +179,28 @@  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.
  * 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)
+void vreport(report_types type, const char *fmt, va_list ap)
 {
     GTimeVal tv;
     gchar *timestr;
 
+    switch (type) {
+    case ERROR:
+        /* To maintin compatibility we don't add anything here */
+        break;
+    case WARN:
+        error_printf("warning: ");
+        break;
+    case INFO:
+        error_printf("info: ");
+        break;
+    }
     if (enable_timestamp_msg && !cur_mon) {
         g_get_current_time(&tv);
         timestr = g_time_val_to_iso8601(&tv);
@@ -203,6 +214,22 @@  void error_vreport(const char *fmt, va_list ap)
 }
 
 /*
+ * Print a 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.
+ */
+void report(report_types type, const char *fmt, ...)
+{
+    va_list ap;
+
+    va_start(ap, fmt);
+    vreport(type, fmt, ap);
+    va_end(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.
@@ -214,6 +241,6 @@  void error_report(const char *fmt, ...)
     va_list ap;
 
     va_start(ap, fmt);
-    error_vreport(fmt, ap);
+    vreport(ERROR, fmt, ap);
     va_end(ap);
 }