[Qemu-devel] [RFC v2 0/3] Implement a warning_report function

[Qemu-devel] [RFC v2 0/3] Implement a warning_report function

[Alistair Francis]

QEMU currently has a standard method to report errors with
error_repot(). This ensure a sane and standard format when printing
errors. This series is attempting to extend this functionality for
warnings and information as well.

At the moment only one error is being converted, I wanted to get the
implementation nailed down a little bit before I started converting
others.

This patch renames error_print_loc() function to be more clear, but I
didn't bother renaming the others. It seems silly to change
error_printf() to error_warning_printf() and printf is already taken so
I just left it as is.

I also didn't change the current error output as that would probably
break backwards compatibilty for anyone who is parsing logs.

RFCv2:
 - Use enums for ERROR, WARN and INFO with a generic report() function
   instead of adding new functions

Alistair Francis (3):
  util/qemu-error: Rename error_print_loc() to be more generic
  qemu-error: Implement a more generic error reporting
  char-socket: Report TCP socket waiting as information

 chardev/char-socket.c       |  2 +-
 hw/virtio/virtio.c          |  2 +-
 include/qemu/error-report.h | 10 +++++++++-
 scripts/checkpatch.pl       |  3 ++-
 util/qemu-error.c           | 37 ++++++++++++++++++++++++++++++++-----
 5 files changed, 45 insertions(+), 9 deletions(-)

-- 
2.11.0

[Qemu-devel] [RFC v2 1/3] util/qemu-error: Rename error_print_loc() to be more generic

[Alistair Francis]

Rename the error_print_loc() function in preparation for using it to
print warnings as well.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---

 util/qemu-error.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/util/qemu-error.c b/util/qemu-error.c
index b331f8f4a4..1c5e35ecdb 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -146,7 +146,7 @@ const char *error_get_progname(void)
 /*
  * Print current location to current monitor if we have one, else to stderr.
  */
-static void error_print_loc(void)
+static void print_loc(void)
 {
     const char *sep = "";
     int i;
@@ -197,7 +197,7 @@ void error_vreport(const char *fmt, va_list ap)
         g_free(timestr);
     }
 
-    error_print_loc();
+    print_loc();
     error_vprintf(fmt, ap);
     error_printf("\n");
 }
-- 
2.11.0

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

[Alistair Francis]

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;
+
 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);
 }
-- 
2.11.0

[Qemu-devel] [RFC v2 3/3] char-socket: Report TCP socket waiting as information

[Alistair Francis]

When QEMU is waiting for a TCP socket connection it reports that message as
an error. This isn't an error it is just informaiton so let's change the
report to use report() instead.

Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
---

 chardev/char-socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index ccc499cfa1..ad1b3d6e93 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -765,7 +765,7 @@ static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
      * in TLS and telnet cases, only wait for an accepted socket */
     while (!s->ioc) {
         if (s->is_listen) {
-            error_report("QEMU waiting for connection on: %s",
+            report(INFO, "QEMU waiting for connection on: %s",
                          chr->filename);
             qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), true, NULL);
             tcp_chr_accept(QIO_CHANNEL(s->listen_ioc), G_IO_IN, chr);
-- 
2.11.0

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1035 bytes --]

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/

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [RFC v2 3/3] char-socket: Report TCP socket waiting as information

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1175 bytes --]

On 06/29/2017 02:42 PM, Alistair Francis wrote:
> When QEMU is waiting for a TCP socket connection it reports that message as
> an error. This isn't an error it is just informaiton so let's change the
> report to use report() instead.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
> ---
> 
>  chardev/char-socket.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index ccc499cfa1..ad1b3d6e93 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -765,7 +765,7 @@ static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
>       * in TLS and telnet cases, only wait for an accepted socket */
>      while (!s->ioc) {
>          if (s->is_listen) {
> -            error_report("QEMU waiting for connection on: %s",
> +            report(INFO, "QEMU waiting for connection on: %s",
>                           chr->filename);

Indentation looks off now; I'd scoot chr->filename to live right under INFO.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [RFC v2 3/3] char-socket: Report TCP socket waiting as information

[Alistair Francis]

On Thu, Jun 29, 2017 at 1:48 PM, Eric Blake <eblake@redhat.com> wrote:
> On 06/29/2017 02:42 PM, Alistair Francis wrote:
>> When QEMU is waiting for a TCP socket connection it reports that message as
>> an error. This isn't an error it is just informaiton so let's change the
>> report to use report() instead.
>>
>> Signed-off-by: Alistair Francis <alistair.francis@xilinx.com>
>> ---
>>
>>  chardev/char-socket.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
>> index ccc499cfa1..ad1b3d6e93 100644
>> --- a/chardev/char-socket.c
>> +++ b/chardev/char-socket.c
>> @@ -765,7 +765,7 @@ static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
>>       * in TLS and telnet cases, only wait for an accepted socket */
>>      while (!s->ioc) {
>>          if (s->is_listen) {
>> -            error_report("QEMU waiting for connection on: %s",
>> +            report(INFO, "QEMU waiting for connection on: %s",
>>                           chr->filename);
>
> Indentation looks off now; I'd scoot chr->filename to live right under INFO.

Woops I missed that. Thanks, I made your fixes to both patches.

Thanks,
Alistair

>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>

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

[Daniel P. Berrange]

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

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

[Markus Armbruster]

"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

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

[Daniel P. Berrange]

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

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

[Markus Armbruster]

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

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

[Alistair Francis]

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