[Qemu-devel] [PATCH 0/6] error: Misc cleanups and improvements

[Qemu-devel] [PATCH 0/6] error: Misc cleanups and improvements

[Cole Robinson]

Minor error API cleanups, and one error reporting improvement when
connected to qmp (patch #6)

Cole Robinson (6):
  slirp: Remove default_mon usage
  vnc: Remove default_mon usage
  error: Privatize error_print_loc
  monitor: Remove unused monitor_print_filename
  error: Remove redundant error_printf_unless_qmp
  error: Print error_report() to stderr if using qmp

 hw/usb/bus.c                |  2 +-
 hw/usb/hcd-ehci.c           |  4 ++--
 include/monitor/monitor.h   |  1 -
 include/qemu/error-report.h |  2 --
 monitor.c                   | 27 ---------------------------
 slirp/misc.c                | 13 ++-----------
 slirp/slirp.c               |  8 ++++----
 slirp/slirp.h               |  2 --
 stubs/Makefile.objs         |  1 -
 stubs/mon-print-filename.c  |  6 ------
 ui/vnc.c                    |  4 ++--
 util/qemu-error.c           | 15 ++-------------
 util/qemu-option.c          |  7 -------
 13 files changed, 13 insertions(+), 79 deletions(-)
 delete mode 100644 stubs/mon-print-filename.c

-- 
1.8.5.3

[Qemu-devel] [PATCH 1/6] slirp: Remove default_mon usage

[Cole Robinson]

These errors don't seem user initiated, so forcibly printing to the
monitor doesn't seem right. Just print to stderr.

Drop lprint since it's now unused.

Cc: Jan Kiszka <jan.kiszka@siemens.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
checkpatch flags some pre-existing tab issues, but I didn't retab. Should I?

 slirp/misc.c  | 13 ++-----------
 slirp/slirp.c |  8 ++++----
 slirp/slirp.h |  2 --
 3 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/slirp/misc.c b/slirp/misc.c
index 6c1636f..662fb1d 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -136,7 +136,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
 		if ((s = qemu_socket(AF_INET, SOCK_STREAM, 0)) < 0 ||
 		    bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
 		    listen(s, 1) < 0) {
-			lprint("Error: inet socket: %s\n", strerror(errno));
+			fprintf(stderr, "Error: inet socket: %s\n", strerror(errno));
 			closesocket(s);
 
 			return 0;
@@ -146,7 +146,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
 	pid = fork();
 	switch(pid) {
 	 case -1:
-		lprint("Error: fork failed: %s\n", strerror(errno));
+		fprintf(stderr, "Error: fork failed: %s\n", strerror(errno));
 		close(s);
 		return 0;
 
@@ -242,15 +242,6 @@ strdup(str)
 }
 #endif
 
-void lprint(const char *format, ...)
-{
-    va_list args;
-
-    va_start(args, format);
-    monitor_vprintf(default_mon, format, args);
-    va_end(args);
-}
-
 void slirp_connection_info(Slirp *slirp, Monitor *mon)
 {
     const char * const tcpstates[] = {
diff --git a/slirp/slirp.c b/slirp/slirp.c
index bad8dad..3fb48a4 100644
--- a/slirp/slirp.c
+++ b/slirp/slirp.c
@@ -139,7 +139,7 @@ int get_dns_addr(struct in_addr *pdns_addr)
         return -1;
 
 #ifdef DEBUG
-    lprint("IP address of your DNS(s): ");
+    fprintf(stderr, "IP address of your DNS(s): ");
 #endif
     while (fgets(buff, 512, f) != NULL) {
         if (sscanf(buff, "nameserver%*[ \t]%256s", buff2) == 1) {
@@ -153,17 +153,17 @@ int get_dns_addr(struct in_addr *pdns_addr)
             }
 #ifdef DEBUG
             else
-                lprint(", ");
+                fprintf(stderr, ", ");
 #endif
             if (++found > 3) {
 #ifdef DEBUG
-                lprint("(more)");
+                fprintf(stderr, "(more)");
 #endif
                 break;
             }
 #ifdef DEBUG
             else
-                lprint("%s", inet_ntoa(tmp_addr));
+                fprintf(stderr, "%s", inet_ntoa(tmp_addr));
 #endif
         }
     }
diff --git a/slirp/slirp.h b/slirp/slirp.h
index e4a1bd4..6589d7e 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -287,8 +287,6 @@ void if_start(struct ttys *);
  long gethostid(void);
 #endif
 
-void lprint(const char *, ...) GCC_FMT_ATTR(1, 2);
-
 #ifndef _WIN32
 #include <netdb.h>
 #endif
-- 
1.8.5.3

[Qemu-devel] [PATCH 2/6] vnc: Remove default_mon usage

[Cole Robinson]

These errors don't seem user initiated, so forcibly printing to the
monitor doesn't seem right. Just print to stderr.

Cc: Anthony Liguori <aliguori@amazon.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 ui/vnc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 9c84b3e..3c9b6c7 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -992,7 +992,7 @@ static void audio_add(VncState *vs)
     struct audio_capture_ops ops;
 
     if (vs->audio_cap) {
-        monitor_printf(default_mon, "audio already running\n");
+        fprintf(stderr, "audio already running\n");
         return;
     }
 
@@ -1002,7 +1002,7 @@ static void audio_add(VncState *vs)
 
     vs->audio_cap = AUD_add_capture(&vs->as, &ops, vs);
     if (!vs->audio_cap) {
-        monitor_printf(default_mon, "Failed to add audio capture\n");
+        fprintf(stderr, "Failed to add audio capture\n");
     }
 }
 
-- 
1.8.5.3

[Qemu-devel] [PATCH 3/6] error: Privatize error_print_loc

[Cole Robinson]

Cc: Luiz Capitulino <lcapitulino@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 include/qemu/error-report.h | 1 -
 util/qemu-error.c           | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 3b098a9..000eae3 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -37,7 +37,6 @@ void loc_set_file(const char *fname, int lno);
 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_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
-void error_print_loc(void);
 void error_set_progname(const char *argv0);
 void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 const char *error_get_progname(void);
diff --git a/util/qemu-error.c b/util/qemu-error.c
index fec02c6..80df49a 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -165,7 +165,7 @@ const char *error_get_progname(void)
 /*
  * Print current location to current monitor if we have one, else to stderr.
  */
-void error_print_loc(void)
+static void error_print_loc(void)
 {
     const char *sep = "";
     int i;
-- 
1.8.5.3

[Qemu-devel] [PATCH 4/6] monitor: Remove unused monitor_print_filename

[Cole Robinson]

Cc: Luiz Capitulino <lcapitulino@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 include/monitor/monitor.h  |  1 -
 monitor.c                  | 27 ---------------------------
 stubs/Makefile.objs        |  1 -
 stubs/mon-print-filename.c |  6 ------
 4 files changed, 35 deletions(-)
 delete mode 100644 stubs/mon-print-filename.c

diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h
index a49ea11..42d8671 100644
--- a/include/monitor/monitor.h
+++ b/include/monitor/monitor.h
@@ -79,7 +79,6 @@ int monitor_handle_fd_param(Monitor *mon, const char *fdname);
 void monitor_vprintf(Monitor *mon, const char *fmt, va_list ap)
     GCC_FMT_ATTR(2, 0);
 void monitor_printf(Monitor *mon, const char *fmt, ...) GCC_FMT_ATTR(2, 3);
-void monitor_print_filename(Monitor *mon, const char *filename);
 void monitor_flush(Monitor *mon);
 int monitor_set_cpu(int cpu_index);
 int monitor_get_cpu_index(void);
diff --git a/monitor.c b/monitor.c
index 342e83b..c8428a1 100644
--- a/monitor.c
+++ b/monitor.c
@@ -352,33 +352,6 @@ void monitor_printf(Monitor *mon, const char *fmt, ...)
     va_end(ap);
 }
 
-void monitor_print_filename(Monitor *mon, const char *filename)
-{
-    int i;
-
-    for (i = 0; filename[i]; i++) {
-        switch (filename[i]) {
-        case ' ':
-        case '"':
-        case '\\':
-            monitor_printf(mon, "\\%c", filename[i]);
-            break;
-        case '\t':
-            monitor_printf(mon, "\\t");
-            break;
-        case '\r':
-            monitor_printf(mon, "\\r");
-            break;
-        case '\n':
-            monitor_printf(mon, "\\n");
-            break;
-        default:
-            monitor_printf(mon, "%c", filename[i]);
-            break;
-        }
-    }
-}
-
 static int GCC_FMT_ATTR(2, 3) monitor_fprintf(FILE *stream,
                                               const char *fmt, ...)
 {
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index df3aa7a..94ded0c 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -14,7 +14,6 @@ stub-obj-y += iothread-lock.o
 stub-obj-y += migr-blocker.o
 stub-obj-y += mon-is-qmp.o
 stub-obj-y += mon-printf.o
-stub-obj-y += mon-print-filename.o
 stub-obj-y += mon-protocol-event.o
 stub-obj-y += mon-set-error.o
 stub-obj-y += pci-drive-hot-add.o
diff --git a/stubs/mon-print-filename.c b/stubs/mon-print-filename.c
deleted file mode 100644
index 9c93964..0000000
--- a/stubs/mon-print-filename.c
+++ /dev/null
@@ -1,6 +0,0 @@
-#include "qemu-common.h"
-#include "monitor/monitor.h"
-
-void monitor_print_filename(Monitor *mon, const char *filename)
-{
-}
-- 
1.8.5.3

[Qemu-devel] [PATCH 5/6] error: Remove redundant error_printf_unless_qmp

[Cole Robinson]

error_printf is just a wrapper around monitor_printf, which already
drops the requested output if cur_mon is qmp.

Cc: Luiz Capitulino <lcapitulino@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 hw/usb/bus.c                |  2 +-
 hw/usb/hcd-ehci.c           |  4 ++--
 include/qemu/error-report.h |  1 -
 util/qemu-error.c           | 11 -----------
 util/qemu-option.c          |  7 -------
 5 files changed, 3 insertions(+), 22 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index fe70429..f860631 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -348,7 +348,7 @@ int usb_register_companion(const char *masterbus, USBPort *ports[],
         qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus",
                       "an USB masterbus");
         if (bus) {
-            error_printf_unless_qmp(
+            error_printf(
                 "USB bus '%s' does not allow companion controllers\n",
                 masterbus);
         }
diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 355bbd6..81ef01d 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -855,7 +855,7 @@ static int ehci_register_companion(USBBus *bus, USBPort *ports[],
     if (firstport + portcount > NB_PORTS) {
         qerror_report(QERR_INVALID_PARAMETER_VALUE, "firstport",
                       "firstport on masterbus");
-        error_printf_unless_qmp(
+        error_printf(
             "firstport value of %u makes companion take ports %u - %u, which "
             "is outside of the valid range of 0 - %u\n", firstport, firstport,
             firstport + portcount - 1, NB_PORTS - 1);
@@ -866,7 +866,7 @@ static int ehci_register_companion(USBBus *bus, USBPort *ports[],
         if (s->companion_ports[firstport + i]) {
             qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus",
                           "an USB masterbus");
-            error_printf_unless_qmp(
+            error_printf(
                 "port %u on masterbus %s already has a companion assigned\n",
                 firstport + i, bus->qbus.name);
             return -1;
diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
index 000eae3..a08ee95 100644
--- a/include/qemu/error-report.h
+++ b/include/qemu/error-report.h
@@ -36,7 +36,6 @@ void loc_set_file(const char *fname, int lno);
 
 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_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 void error_set_progname(const char *argv0);
 void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
 const char *error_get_progname(void);
diff --git a/util/qemu-error.c b/util/qemu-error.c
index 80df49a..0ccd3e9 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -40,17 +40,6 @@ void error_printf(const char *fmt, ...)
     va_end(ap);
 }
 
-void error_printf_unless_qmp(const char *fmt, ...)
-{
-    va_list ap;
-
-    if (!monitor_cur_is_qmp()) {
-        va_start(ap, fmt);
-        error_vprintf(fmt, ap);
-        va_end(ap);
-    }
-}
-
 static Location std_loc = {
     .kind = LOC_NONE
 };
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 9d898af..552fd06 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -201,10 +201,6 @@ void parse_option_size(const char *name, const char *value,
             break;
         default:
             error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
-#if 0 /* conversion from qerror_report() to error_set() broke this: */
-            error_printf_unless_qmp("You may use k, M, G or T suffixes for "
-                    "kilobytes, megabytes, gigabytes and terabytes.\n");
-#endif
             return;
         }
     } else {
@@ -811,9 +807,6 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
     if (id) {
         if (!id_wellformed(id)) {
             error_set(errp,QERR_INVALID_PARAMETER_VALUE, "id", "an identifier");
-#if 0 /* conversion from qerror_report() to error_set() broke this: */
-            error_printf_unless_qmp("Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.\n");
-#endif
             return NULL;
         }
         opts = qemu_opts_find(list, id);
-- 
1.8.5.3

[Qemu-devel] [PATCH 6/6] error: Print error_report() to stderr if using qmp

[Cole Robinson]

monitor_printf will drop the requested output if cur_mon is qmp (for
good reason). However these messages are often helpful for debugging
issues with via libvirt.

If we know the message won't hit the monitor, send it to stderr.

Cc: Luiz Capitulino <lcapitulino@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Cole Robinson <crobinso@redhat.com>
---
 util/qemu-error.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/qemu-error.c b/util/qemu-error.c
index 0ccd3e9..46871cd 100644
--- a/util/qemu-error.c
+++ b/util/qemu-error.c
@@ -20,7 +20,7 @@
  */
 void error_vprintf(const char *fmt, va_list ap)
 {
-    if (cur_mon) {
+    if (cur_mon && !monitor_cur_is_qmp()) {
         monitor_vprintf(cur_mon, fmt, ap);
     } else {
         vfprintf(stderr, fmt, ap);
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH 1/6] slirp: Remove default_mon usage

[Jan Kiszka]

On 2014-03-12 00:15, Cole Robinson wrote:
> These errors don't seem user initiated, so forcibly printing to the
> monitor doesn't seem right. Just print to stderr.
> 
> Drop lprint since it's now unused.
> 
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
> checkpatch flags some pre-existing tab issues, but I didn't retab. Should I?
> 
>  slirp/misc.c  | 13 ++-----------
>  slirp/slirp.c |  8 ++++----
>  slirp/slirp.h |  2 --
>  3 files changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/slirp/misc.c b/slirp/misc.c
> index 6c1636f..662fb1d 100644
> --- a/slirp/misc.c
> +++ b/slirp/misc.c
> @@ -136,7 +136,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
>  		if ((s = qemu_socket(AF_INET, SOCK_STREAM, 0)) < 0 ||
>  		    bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
>  		    listen(s, 1) < 0) {
> -			lprint("Error: inet socket: %s\n", strerror(errno));
> +			fprintf(stderr, "Error: inet socket: %s\n", strerror(errno));
>  			closesocket(s);
>  
>  			return 0;
> @@ -146,7 +146,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
>  	pid = fork();
>  	switch(pid) {
>  	 case -1:
> -		lprint("Error: fork failed: %s\n", strerror(errno));
> +		fprintf(stderr, "Error: fork failed: %s\n", strerror(errno));
>  		close(s);
>  		return 0;
>  
> @@ -242,15 +242,6 @@ strdup(str)
>  }
>  #endif
>  
> -void lprint(const char *format, ...)
> -{
> -    va_list args;
> -
> -    va_start(args, format);
> -    monitor_vprintf(default_mon, format, args);
> -    va_end(args);
> -}
> -
>  void slirp_connection_info(Slirp *slirp, Monitor *mon)
>  {
>      const char * const tcpstates[] = {
> diff --git a/slirp/slirp.c b/slirp/slirp.c
> index bad8dad..3fb48a4 100644
> --- a/slirp/slirp.c
> +++ b/slirp/slirp.c
> @@ -139,7 +139,7 @@ int get_dns_addr(struct in_addr *pdns_addr)
>          return -1;
>  
>  #ifdef DEBUG
> -    lprint("IP address of your DNS(s): ");
> +    fprintf(stderr, "IP address of your DNS(s): ");
>  #endif
>      while (fgets(buff, 512, f) != NULL) {
>          if (sscanf(buff, "nameserver%*[ \t]%256s", buff2) == 1) {
> @@ -153,17 +153,17 @@ int get_dns_addr(struct in_addr *pdns_addr)
>              }
>  #ifdef DEBUG
>              else
> -                lprint(", ");
> +                fprintf(stderr, ", ");
>  #endif
>              if (++found > 3) {
>  #ifdef DEBUG
> -                lprint("(more)");
> +                fprintf(stderr, "(more)");
>  #endif
>                  break;
>              }
>  #ifdef DEBUG
>              else
> -                lprint("%s", inet_ntoa(tmp_addr));
> +                fprintf(stderr, "%s", inet_ntoa(tmp_addr));
>  #endif
>          }
>      }
> diff --git a/slirp/slirp.h b/slirp/slirp.h
> index e4a1bd4..6589d7e 100644
> --- a/slirp/slirp.h
> +++ b/slirp/slirp.h
> @@ -287,8 +287,6 @@ void if_start(struct ttys *);
>   long gethostid(void);
>  #endif
>  
> -void lprint(const char *, ...) GCC_FMT_ATTR(1, 2);
> -
>  #ifndef _WIN32
>  #include <netdb.h>
>  #endif
> 

Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com>

I suppose this goes through Luiz' queue?

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH 2/6] vnc: Remove default_mon usage

[Gerd Hoffmann]

On Di, 2014-03-11 at 19:15 -0400, Cole Robinson wrote:
> These errors don't seem user initiated, so forcibly printing to the
> monitor doesn't seem right. Just print to stderr.
> 
> Cc: Anthony Liguori <aliguori@amazon.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>

Reviewed-by: Gerd Hoffmann <kraxel@gmail.com>

Re: [Qemu-devel] [PATCH 1/6] slirp: Remove default_mon usage

[Markus Armbruster]

Cole Robinson <crobinso@redhat.com> writes:

> These errors don't seem user initiated, so forcibly printing to the
> monitor doesn't seem right. Just print to stderr.
>
> Drop lprint since it's now unused.
>
> Cc: Jan Kiszka <jan.kiszka@siemens.com>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
> checkpatch flags some pre-existing tab issues, but I didn't retab. Should I?
>
>  slirp/misc.c  | 13 ++-----------
>  slirp/slirp.c |  8 ++++----
>  slirp/slirp.h |  2 --
>  3 files changed, 6 insertions(+), 17 deletions(-)
>
> diff --git a/slirp/misc.c b/slirp/misc.c
> index 6c1636f..662fb1d 100644
> --- a/slirp/misc.c
> +++ b/slirp/misc.c
> @@ -136,7 +136,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
>  		if ((s = qemu_socket(AF_INET, SOCK_STREAM, 0)) < 0 ||
>  		    bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
>  		    listen(s, 1) < 0) {
> -			lprint("Error: inet socket: %s\n", strerror(errno));
> +			fprintf(stderr, "Error: inet socket: %s\n", strerror(errno));
>  			closesocket(s);
>  
>  			return 0;

Why not error_report()?

[...]

Re: [Qemu-devel] [PATCH 5/6] error: Remove redundant error_printf_unless_qmp

[Markus Armbruster]

Cole Robinson <crobinso@redhat.com> writes:

> error_printf is just a wrapper around monitor_printf, which already
> drops the requested output if cur_mon is qmp.

Since commit 74ee59a:

    monitor: drop unused monitor debug code
    
    In the old QMP days, this code was used to find out QMP commands that
    might be calling monitor_printf() down its call chain.
    
    This is almost impossible to happen today, because the qapi converted
    commands don't even have a monitor object. Besides, it's been more than
    a year since I used this last time.
    
    Let's just drop it.
    
    Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
    Reviewed-by: Markus Armbruster <armbru@redhat.com>

I gave my R-by then, but I'm no longer sure it was a sensible move.
Attempting to print in QMP context is as much a sign of trouble as it
ever was.  I hate sweeping signs of trouble under the carpet.  If misuse
is really "almost impossible", then we should assert() it is, and fix
the bugs caught by it.

I can see error_printf() / error_printf_unless_qmp() used in a couple of
ways:

1. Print hints after qerror_report(), with error_printf_unless_qmp()

   qerror_report() is a transitional interface to help with converting
   existing HMP commands to QMP.  It should not be used elsewhere.

   We suppress the hints in QMP, because the QMP wire format doesn't
   provide for hints.

   We can't add hints to an error when using error_set(), because that
   API lacks support for hints.  Pity.

   Examples: see your patch below.

2. Print hints after error_report(), with error_printf()

   Use of error_report() in QMP context is a sign of trouble just like
   any other non-JSON output to the monitor.

   error_printf() rather than error_printf_unless_qmp(), because the
   latter explicitly signals intent "skip this in QMP", while the former
   signals "QMP should not happen".

   The difference in intent is what makes me wary of this patch.

   Example: vfio_pci_load_rom().

3. Ordinary output in code shared between command line and HMP, with
   error_printf()

   error_printf() was pressed into use as convenient "print to monitor
   in HMP context, else to tty" function.  Inappropriate, because it
   prints to stderr rather than stdout.

   Examples: many help texts under is_help_option().

4. Warnings, with error_printf()

   I figure these should use error_report() instead.

   Examples: block/ssh.c, hw/misc/vfio.c, ...

> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
>  hw/usb/bus.c                |  2 +-
>  hw/usb/hcd-ehci.c           |  4 ++--
>  include/qemu/error-report.h |  1 -
>  util/qemu-error.c           | 11 -----------
>  util/qemu-option.c          |  7 -------
>  5 files changed, 3 insertions(+), 22 deletions(-)
>
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index fe70429..f860631 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -348,7 +348,7 @@ int usb_register_companion(const char *masterbus, USBPort *ports[],
>          qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus",
>                        "an USB masterbus");
>          if (bus) {
> -            error_printf_unless_qmp(
> +            error_printf(
>                  "USB bus '%s' does not allow companion controllers\n",
>                  masterbus);
>          }
> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
> index 355bbd6..81ef01d 100644
> --- a/hw/usb/hcd-ehci.c
> +++ b/hw/usb/hcd-ehci.c
> @@ -855,7 +855,7 @@ static int ehci_register_companion(USBBus *bus, USBPort *ports[],
>      if (firstport + portcount > NB_PORTS) {
>          qerror_report(QERR_INVALID_PARAMETER_VALUE, "firstport",
>                        "firstport on masterbus");
> -        error_printf_unless_qmp(
> +        error_printf(
>              "firstport value of %u makes companion take ports %u - %u, which "
>              "is outside of the valid range of 0 - %u\n", firstport, firstport,
>              firstport + portcount - 1, NB_PORTS - 1);
> @@ -866,7 +866,7 @@ static int ehci_register_companion(USBBus *bus, USBPort *ports[],
>          if (s->companion_ports[firstport + i]) {
>              qerror_report(QERR_INVALID_PARAMETER_VALUE, "masterbus",
>                            "an USB masterbus");
> -            error_printf_unless_qmp(
> +            error_printf(
>                  "port %u on masterbus %s already has a companion assigned\n",
>                  firstport + i, bus->qbus.name);
>              return -1;
> diff --git a/include/qemu/error-report.h b/include/qemu/error-report.h
> index 000eae3..a08ee95 100644
> --- a/include/qemu/error-report.h
> +++ b/include/qemu/error-report.h
> @@ -36,7 +36,6 @@ void loc_set_file(const char *fname, int lno);
>  
>  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_printf_unless_qmp(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>  void error_set_progname(const char *argv0);
>  void error_report(const char *fmt, ...) GCC_FMT_ATTR(1, 2);
>  const char *error_get_progname(void);
> diff --git a/util/qemu-error.c b/util/qemu-error.c
> index 80df49a..0ccd3e9 100644
> --- a/util/qemu-error.c
> +++ b/util/qemu-error.c
> @@ -40,17 +40,6 @@ void error_printf(const char *fmt, ...)
>      va_end(ap);
>  }
>  
> -void error_printf_unless_qmp(const char *fmt, ...)
> -{
> -    va_list ap;
> -
> -    if (!monitor_cur_is_qmp()) {
> -        va_start(ap, fmt);
> -        error_vprintf(fmt, ap);
> -        va_end(ap);
> -    }
> -}
> -
>  static Location std_loc = {
>      .kind = LOC_NONE
>  };
> diff --git a/util/qemu-option.c b/util/qemu-option.c
> index 9d898af..552fd06 100644
> --- a/util/qemu-option.c
> +++ b/util/qemu-option.c
> @@ -201,10 +201,6 @@ void parse_option_size(const char *name, const char *value,
>              break;
>          default:
>              error_set(errp, QERR_INVALID_PARAMETER_VALUE, name, "a size");
> -#if 0 /* conversion from qerror_report() to error_set() broke this: */
> -            error_printf_unless_qmp("You may use k, M, G or T suffixes for "
> -                    "kilobytes, megabytes, gigabytes and terabytes.\n");
> -#endif
>              return;
>          }
>      } else {

We haven't been able to repair this breakage for a while, so giving up
and removing its debris isn't unreasonable.  However, I'd rather keep it
for now as a reminder of the deficiency in the error_set() API.

> @@ -811,9 +807,6 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
>      if (id) {
>          if (!id_wellformed(id)) {
>              error_set(errp,QERR_INVALID_PARAMETER_VALUE, "id", "an identifier");
> -#if 0 /* conversion from qerror_report() to error_set() broke this: */
> -            error_printf_unless_qmp("Identifiers consist of letters, digits, '-', '.', '_', starting with a letter.\n");
> -#endif
>              return NULL;
>          }
>          opts = qemu_opts_find(list, id);

Likewise.

Re: [Qemu-devel] [PATCH 1/6] slirp: Remove default_mon usage

[Luiz Capitulino]

On Wed, 12 Mar 2014 08:22:05 +0100
Jan Kiszka <jan.kiszka@siemens.com> wrote:

> On 2014-03-12 00:15, Cole Robinson wrote:
> > These errors don't seem user initiated, so forcibly printing to the
> > monitor doesn't seem right. Just print to stderr.
> > 
> > Drop lprint since it's now unused.
> > 
> > Cc: Jan Kiszka <jan.kiszka@siemens.com>
> > Signed-off-by: Cole Robinson <crobinso@redhat.com>
> > ---
> > checkpatch flags some pre-existing tab issues, but I didn't retab. Should I?
> > 
> >  slirp/misc.c  | 13 ++-----------
> >  slirp/slirp.c |  8 ++++----
> >  slirp/slirp.h |  2 --
> >  3 files changed, 6 insertions(+), 17 deletions(-)
> > 
> > diff --git a/slirp/misc.c b/slirp/misc.c
> > index 6c1636f..662fb1d 100644
> > --- a/slirp/misc.c
> > +++ b/slirp/misc.c
> > @@ -136,7 +136,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
> >  		if ((s = qemu_socket(AF_INET, SOCK_STREAM, 0)) < 0 ||
> >  		    bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
> >  		    listen(s, 1) < 0) {
> > -			lprint("Error: inet socket: %s\n", strerror(errno));
> > +			fprintf(stderr, "Error: inet socket: %s\n", strerror(errno));
> >  			closesocket(s);
> >  
> >  			return 0;
> > @@ -146,7 +146,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
> >  	pid = fork();
> >  	switch(pid) {
> >  	 case -1:
> > -		lprint("Error: fork failed: %s\n", strerror(errno));
> > +		fprintf(stderr, "Error: fork failed: %s\n", strerror(errno));
> >  		close(s);
> >  		return 0;
> >  
> > @@ -242,15 +242,6 @@ strdup(str)
> >  }
> >  #endif
> >  
> > -void lprint(const char *format, ...)
> > -{
> > -    va_list args;
> > -
> > -    va_start(args, format);
> > -    monitor_vprintf(default_mon, format, args);
> > -    va_end(args);
> > -}
> > -
> >  void slirp_connection_info(Slirp *slirp, Monitor *mon)
> >  {
> >      const char * const tcpstates[] = {
> > diff --git a/slirp/slirp.c b/slirp/slirp.c
> > index bad8dad..3fb48a4 100644
> > --- a/slirp/slirp.c
> > +++ b/slirp/slirp.c
> > @@ -139,7 +139,7 @@ int get_dns_addr(struct in_addr *pdns_addr)
> >          return -1;
> >  
> >  #ifdef DEBUG
> > -    lprint("IP address of your DNS(s): ");
> > +    fprintf(stderr, "IP address of your DNS(s): ");
> >  #endif
> >      while (fgets(buff, 512, f) != NULL) {
> >          if (sscanf(buff, "nameserver%*[ \t]%256s", buff2) == 1) {
> > @@ -153,17 +153,17 @@ int get_dns_addr(struct in_addr *pdns_addr)
> >              }
> >  #ifdef DEBUG
> >              else
> > -                lprint(", ");
> > +                fprintf(stderr, ", ");
> >  #endif
> >              if (++found > 3) {
> >  #ifdef DEBUG
> > -                lprint("(more)");
> > +                fprintf(stderr, "(more)");
> >  #endif
> >                  break;
> >              }
> >  #ifdef DEBUG
> >              else
> > -                lprint("%s", inet_ntoa(tmp_addr));
> > +                fprintf(stderr, "%s", inet_ntoa(tmp_addr));
> >  #endif
> >          }
> >      }
> > diff --git a/slirp/slirp.h b/slirp/slirp.h
> > index e4a1bd4..6589d7e 100644
> > --- a/slirp/slirp.h
> > +++ b/slirp/slirp.h
> > @@ -287,8 +287,6 @@ void if_start(struct ttys *);
> >   long gethostid(void);
> >  #endif
> >  
> > -void lprint(const char *, ...) GCC_FMT_ATTR(1, 2);
> > -
> >  #ifndef _WIN32
> >  #include <netdb.h>
> >  #endif
> > 
> 
> Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> I suppose this goes through Luiz' queue?

Yes.

Re: [Qemu-devel] [PATCH 1/6] slirp: Remove default_mon usage

[Cole Robinson]

On 03/12/2014 04:13 AM, Markus Armbruster wrote:
> Cole Robinson <crobinso@redhat.com> writes:
> 
>> These errors don't seem user initiated, so forcibly printing to the
>> monitor doesn't seem right. Just print to stderr.
>>
>> Drop lprint since it's now unused.
>>
>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>> ---
>> checkpatch flags some pre-existing tab issues, but I didn't retab. Should I?
>>
>>  slirp/misc.c  | 13 ++-----------
>>  slirp/slirp.c |  8 ++++----
>>  slirp/slirp.h |  2 --
>>  3 files changed, 6 insertions(+), 17 deletions(-)
>>
>> diff --git a/slirp/misc.c b/slirp/misc.c
>> index 6c1636f..662fb1d 100644
>> --- a/slirp/misc.c
>> +++ b/slirp/misc.c
>> @@ -136,7 +136,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
>>  		if ((s = qemu_socket(AF_INET, SOCK_STREAM, 0)) < 0 ||
>>  		    bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
>>  		    listen(s, 1) < 0) {
>> -			lprint("Error: inet socket: %s\n", strerror(errno));
>> +			fprintf(stderr, "Error: inet socket: %s\n", strerror(errno));
>>  			closesocket(s);
>>  
>>  			return 0;
> 
> Why not error_report()?
> 
> [...]
> 

AFAICT this is only guest initiated, never from the monitor. error_report
sends output to cur_mon if a monitor command is being processed. If this
message triggers while a different monitor command is being processed, do we
really want this unrelated output to go to the monitor?

(Maybe that can't happen in practice, I don't know enough about qemu's
threading model, but I was being cautious)

- Cole

Re: [Qemu-devel] [PATCH 1/6] slirp: Remove default_mon usage

[Markus Armbruster]

Cole Robinson <crobinso@redhat.com> writes:

> On 03/12/2014 04:13 AM, Markus Armbruster wrote:
>> Cole Robinson <crobinso@redhat.com> writes:
>> 
>>> These errors don't seem user initiated, so forcibly printing to the
>>> monitor doesn't seem right. Just print to stderr.
>>>
>>> Drop lprint since it's now unused.
>>>
>>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>>> ---
>>> checkpatch flags some pre-existing tab issues, but I didn't retab. Should I?
>>>
>>>  slirp/misc.c  | 13 ++-----------
>>>  slirp/slirp.c |  8 ++++----
>>>  slirp/slirp.h |  2 --
>>>  3 files changed, 6 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/slirp/misc.c b/slirp/misc.c
>>> index 6c1636f..662fb1d 100644
>>> --- a/slirp/misc.c
>>> +++ b/slirp/misc.c
>>> @@ -136,7 +136,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
>>>  		if ((s = qemu_socket(AF_INET, SOCK_STREAM, 0)) < 0 ||
>>>  		    bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
>>>  		    listen(s, 1) < 0) {
>>> -			lprint("Error: inet socket: %s\n", strerror(errno));
>>> +			fprintf(stderr, "Error: inet socket: %s\n", strerror(errno));
>>>  			closesocket(s);
>>>  
>>>  			return 0;
>> 
>> Why not error_report()?
>> 
>> [...]
>> 
>
> AFAICT this is only guest initiated, never from the monitor. error_report
> sends output to cur_mon if a monitor command is being processed. If this
> message triggers while a different monitor command is being processed, do we
> really want this unrelated output to go to the monitor?
>
> (Maybe that can't happen in practice, I don't know enough about qemu's
> threading model, but I was being cautious)

If cur_mon is non-null while we're executing anything but a monitor in
the main thread, it's a bug.

That leaves signal handlers (which shouldn't care), and other threads.
Other threads used not to exist, and then our first ones didn't care,
but with more of them around now, I wonder whether we should make
cur_mon thread-local.

error_report() should remain usable when fprintf() is.

Re: [Qemu-devel] [PATCH 5/6] error: Remove redundant error_printf_unless_qmp

[Cole Robinson]

On 03/12/2014 04:56 AM, Markus Armbruster wrote:
> Cole Robinson <crobinso@redhat.com> writes:
> 
>> error_printf is just a wrapper around monitor_printf, which already
>> drops the requested output if cur_mon is qmp.
> 
> Since commit 74ee59a:
> 
>     monitor: drop unused monitor debug code
>     
>     In the old QMP days, this code was used to find out QMP commands that
>     might be calling monitor_printf() down its call chain.
>     
>     This is almost impossible to happen today, because the qapi converted
>     commands don't even have a monitor object. Besides, it's been more than
>     a year since I used this last time.
>     
>     Let's just drop it.
>     
>     Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com>
>     Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> I gave my R-by then, but I'm no longer sure it was a sensible move.
> Attempting to print in QMP context is as much a sign of trouble as it
> ever was.  I hate sweeping signs of trouble under the carpet.  If misuse
> is really "almost impossible", then we should assert() it is, and fix
> the bugs caught by it.
> 
> I can see error_printf() / error_printf_unless_qmp() used in a couple of
> ways:
> 
> 1. Print hints after qerror_report(), with error_printf_unless_qmp()
> 
>    qerror_report() is a transitional interface to help with converting
>    existing HMP commands to QMP.  It should not be used elsewhere.
> 
>    We suppress the hints in QMP, because the QMP wire format doesn't
>    provide for hints.
> 
>    We can't add hints to an error when using error_set(), because that
>    API lacks support for hints.  Pity.
> 
>    Examples: see your patch below.
> 
> 2. Print hints after error_report(), with error_printf()
> 
>    Use of error_report() in QMP context is a sign of trouble just like
>    any other non-JSON output to the monitor.
> 
>    error_printf() rather than error_printf_unless_qmp(), because the
>    latter explicitly signals intent "skip this in QMP", while the former
>    signals "QMP should not happen".
> 
>    The difference in intent is what makes me wary of this patch.
> 
>    Example: vfio_pci_load_rom().
> 
> 3. Ordinary output in code shared between command line and HMP, with
>    error_printf()
> 
>    error_printf() was pressed into use as convenient "print to monitor
>    in HMP context, else to tty" function.  Inappropriate, because it
>    prints to stderr rather than stdout.
> 
>    Examples: many help texts under is_help_option().
> 
> 4. Warnings, with error_printf()
> 
>    I figure these should use error_report() instead.
> 
>    Examples: block/ssh.c, hw/misc/vfio.c, ...

Thanks, I didn't think about any of that. I'll drop this patch

- Cole

Re: [Qemu-devel] [PATCH 1/6] slirp: Remove default_mon usage

[Andreas Färber]

Am 12.03.2014 08:22, schrieb Jan Kiszka:
> On 2014-03-12 00:15, Cole Robinson wrote:
>> These errors don't seem user initiated, so forcibly printing to the
>> monitor doesn't seem right. Just print to stderr.
>>
>> Drop lprint since it's now unused.
>>
>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>> ---
>> checkpatch flags some pre-existing tab issues, but I didn't retab. Should I?
>>
>>  slirp/misc.c  | 13 ++-----------
>>  slirp/slirp.c |  8 ++++----
>>  slirp/slirp.h |  2 --
>>  3 files changed, 6 insertions(+), 17 deletions(-)
>>
>> diff --git a/slirp/misc.c b/slirp/misc.c
>> index 6c1636f..662fb1d 100644
>> --- a/slirp/misc.c
>> +++ b/slirp/misc.c
>> @@ -136,7 +136,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
>>  		if ((s = qemu_socket(AF_INET, SOCK_STREAM, 0)) < 0 ||
>>  		    bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
>>  		    listen(s, 1) < 0) {
>> -			lprint("Error: inet socket: %s\n", strerror(errno));
>> +			fprintf(stderr, "Error: inet socket: %s\n", strerror(errno));
>>  			closesocket(s);
>>  
>>  			return 0;
>> @@ -146,7 +146,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
>>  	pid = fork();
>>  	switch(pid) {
>>  	 case -1:
>> -		lprint("Error: fork failed: %s\n", strerror(errno));
>> +		fprintf(stderr, "Error: fork failed: %s\n", strerror(errno));
>>  		close(s);
>>  		return 0;
>>  
>> @@ -242,15 +242,6 @@ strdup(str)
>>  }
>>  #endif
>>  
>> -void lprint(const char *format, ...)
>> -{
>> -    va_list args;
>> -
>> -    va_start(args, format);
>> -    monitor_vprintf(default_mon, format, args);
>> -    va_end(args);
>> -}
>> -
>>  void slirp_connection_info(Slirp *slirp, Monitor *mon)
>>  {
>>      const char * const tcpstates[] = {
>> diff --git a/slirp/slirp.c b/slirp/slirp.c
>> index bad8dad..3fb48a4 100644
>> --- a/slirp/slirp.c
>> +++ b/slirp/slirp.c
>> @@ -139,7 +139,7 @@ int get_dns_addr(struct in_addr *pdns_addr)
>>          return -1;
>>  
>>  #ifdef DEBUG
>> -    lprint("IP address of your DNS(s): ");
>> +    fprintf(stderr, "IP address of your DNS(s): ");
>>  #endif
>>      while (fgets(buff, 512, f) != NULL) {
>>          if (sscanf(buff, "nameserver%*[ \t]%256s", buff2) == 1) {
>> @@ -153,17 +153,17 @@ int get_dns_addr(struct in_addr *pdns_addr)
>>              }
>>  #ifdef DEBUG
>>              else
>> -                lprint(", ");
>> +                fprintf(stderr, ", ");
>>  #endif
>>              if (++found > 3) {
>>  #ifdef DEBUG
>> -                lprint("(more)");
>> +                fprintf(stderr, "(more)");
>>  #endif
>>                  break;
>>              }
>>  #ifdef DEBUG
>>              else
>> -                lprint("%s", inet_ntoa(tmp_addr));
>> +                fprintf(stderr, "%s", inet_ntoa(tmp_addr));
>>  #endif
>>          }
>>      }
>> diff --git a/slirp/slirp.h b/slirp/slirp.h
>> index e4a1bd4..6589d7e 100644
>> --- a/slirp/slirp.h
>> +++ b/slirp/slirp.h
>> @@ -287,8 +287,6 @@ void if_start(struct ttys *);
>>   long gethostid(void);
>>  #endif
>>  
>> -void lprint(const char *, ...) GCC_FMT_ATTR(1, 2);
>> -
>>  #ifndef _WIN32
>>  #include <netdb.h>
>>  #endif
>>
> 
> Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com>

The first two are not OK. Instead of fprintf(stderr), error_report()
should be used, which prints an optional timestamp and executable name
and doesn't need to be terminated with \n.

For the loop where it's protected by #ifdef DEBUG anyway, fprintf()
should be fine, but I do wonder why there's only separators being
converted inside the loop and no value... guessing the original code was
inconsistent?

Andreas

> 
> I suppose this goes through Luiz' queue?
> 
> Jan
> 


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 3/6] error: Privatize error_print_loc

[Andreas Färber]

Am 12.03.2014 00:15, schrieb Cole Robinson:
> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> ---
>  include/qemu/error-report.h | 1 -
>  util/qemu-error.c           | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)

Looks sensible to me if otherwise unused,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 4/6] monitor: Remove unused monitor_print_filename

[Andreas Färber]

Am 12.03.2014 00:15, schrieb Cole Robinson:
> Cc: Luiz Capitulino <lcapitulino@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Cole Robinson <crobinso@redhat.com>

Given that it even had its own stub file, the commit message should
please explain why it is no longer needed and thus okay to drop.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/6] vnc: Remove default_mon usage

[Andreas Färber]

Am 12.03.2014 08:35, schrieb Gerd Hoffmann:
> On Di, 2014-03-11 at 19:15 -0400, Cole Robinson wrote:
>> These errors don't seem user initiated, so forcibly printing to the
>> monitor doesn't seem right. Just print to stderr.
>>
>> Cc: Anthony Liguori <aliguori@amazon.com>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> 
> Reviewed-by: Gerd Hoffmann <kraxel@gmail.com>

Both should use error_report(), no? We really need clear-to-understand
rules of when to use which API and then stick with them.

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 1/6] slirp: Remove default_mon usage

[Cole Robinson]

On 03/22/2014 02:27 PM, Andreas Färber wrote:
> Am 12.03.2014 08:22, schrieb Jan Kiszka:
>> On 2014-03-12 00:15, Cole Robinson wrote:
>>> These errors don't seem user initiated, so forcibly printing to the
>>> monitor doesn't seem right. Just print to stderr.
>>>
>>> Drop lprint since it's now unused.
>>>
>>> Cc: Jan Kiszka <jan.kiszka@siemens.com>
>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>>> ---
>>> checkpatch flags some pre-existing tab issues, but I didn't retab. Should I?
>>>
>>>  slirp/misc.c  | 13 ++-----------
>>>  slirp/slirp.c |  8 ++++----
>>>  slirp/slirp.h |  2 --
>>>  3 files changed, 6 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/slirp/misc.c b/slirp/misc.c
>>> index 6c1636f..662fb1d 100644
>>> --- a/slirp/misc.c
>>> +++ b/slirp/misc.c
>>> @@ -136,7 +136,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
>>>  		if ((s = qemu_socket(AF_INET, SOCK_STREAM, 0)) < 0 ||
>>>  		    bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
>>>  		    listen(s, 1) < 0) {
>>> -			lprint("Error: inet socket: %s\n", strerror(errno));
>>> +			fprintf(stderr, "Error: inet socket: %s\n", strerror(errno));
>>>  			closesocket(s);
>>>  
>>>  			return 0;
>>> @@ -146,7 +146,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
>>>  	pid = fork();
>>>  	switch(pid) {
>>>  	 case -1:
>>> -		lprint("Error: fork failed: %s\n", strerror(errno));
>>> +		fprintf(stderr, "Error: fork failed: %s\n", strerror(errno));
>>>  		close(s);
>>>  		return 0;
>>>  
>>> @@ -242,15 +242,6 @@ strdup(str)
>>>  }
>>>  #endif
>>>  
>>> -void lprint(const char *format, ...)
>>> -{
>>> -    va_list args;
>>> -
>>> -    va_start(args, format);
>>> -    monitor_vprintf(default_mon, format, args);
>>> -    va_end(args);
>>> -}
>>> -
>>>  void slirp_connection_info(Slirp *slirp, Monitor *mon)
>>>  {
>>>      const char * const tcpstates[] = {
>>> diff --git a/slirp/slirp.c b/slirp/slirp.c
>>> index bad8dad..3fb48a4 100644
>>> --- a/slirp/slirp.c
>>> +++ b/slirp/slirp.c
>>> @@ -139,7 +139,7 @@ int get_dns_addr(struct in_addr *pdns_addr)
>>>          return -1;
>>>  
>>>  #ifdef DEBUG
>>> -    lprint("IP address of your DNS(s): ");
>>> +    fprintf(stderr, "IP address of your DNS(s): ");
>>>  #endif
>>>      while (fgets(buff, 512, f) != NULL) {
>>>          if (sscanf(buff, "nameserver%*[ \t]%256s", buff2) == 1) {
>>> @@ -153,17 +153,17 @@ int get_dns_addr(struct in_addr *pdns_addr)
>>>              }
>>>  #ifdef DEBUG
>>>              else
>>> -                lprint(", ");
>>> +                fprintf(stderr, ", ");
>>>  #endif
>>>              if (++found > 3) {
>>>  #ifdef DEBUG
>>> -                lprint("(more)");
>>> +                fprintf(stderr, "(more)");
>>>  #endif
>>>                  break;
>>>              }
>>>  #ifdef DEBUG
>>>              else
>>> -                lprint("%s", inet_ntoa(tmp_addr));
>>> +                fprintf(stderr, "%s", inet_ntoa(tmp_addr));
>>>  #endif
>>>          }
>>>      }
>>> diff --git a/slirp/slirp.h b/slirp/slirp.h
>>> index e4a1bd4..6589d7e 100644
>>> --- a/slirp/slirp.h
>>> +++ b/slirp/slirp.h
>>> @@ -287,8 +287,6 @@ void if_start(struct ttys *);
>>>   long gethostid(void);
>>>  #endif
>>>  
>>> -void lprint(const char *, ...) GCC_FMT_ATTR(1, 2);
>>> -
>>>  #ifndef _WIN32
>>>  #include <netdb.h>
>>>  #endif
>>>
>>
>> Reviewed-by: Jan Kiszka <jan.kiszka@siemens.com>
> 
> The first two are not OK. Instead of fprintf(stderr), error_report()
> should be used, which prints an optional timestamp and executable name
> and doesn't need to be terminated with \n.
> 

Markus had the same comment. I posted v2 of this series yesterday which
addresses this.

- Cole

Re: [Qemu-devel] [PATCH 4/6] monitor: Remove unused monitor_print_filename

[Cole Robinson]

On 03/22/2014 04:06 PM, Andreas Färber wrote:
> Am 12.03.2014 00:15, schrieb Cole Robinson:
>> Cc: Luiz Capitulino <lcapitulino@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
> 
> Given that it even had its own stub file, the commit message should
> please explain why it is no longer needed and thus okay to drop.
> 
> Regards,
> Andreas
> 

The last usage of it was dropped with:

commit fbe2e26c15af35e4d157874dc80f6a19eebaa83b
Author: Kevin Wolf <kwolf@redhat.com>
Date:   Wed Jun 19 16:10:55 2013 +0200

    hmp: Make "info block" output more readable

Previously it was used to escape file= and backing_file= in 'info block' output.

I sent this patch because I noticed it was unused, maybe someone else can
comment on whether it's still useful?

- Cole

Re: [Qemu-devel] [PATCH 4/6] monitor: Remove unused monitor_print_filename

[Luiz Capitulino]

On Sat, 22 Mar 2014 21:06:41 +0100
Andreas Färber <afaerber@suse.de> wrote:

> Am 12.03.2014 00:15, schrieb Cole Robinson:
> > Cc: Luiz Capitulino <lcapitulino@redhat.com>
> > Cc: Markus Armbruster <armbru@redhat.com>
> > Signed-off-by: Cole Robinson <crobinso@redhat.com>
> 
> Given that it even had its own stub file, the commit message should
> please explain why it is no longer needed and thus okay to drop.

Because it's unused?

Re: [Qemu-devel] [PATCH 4/6] monitor: Remove unused monitor_print_filename

[Andreas Färber]

Am 24.03.2014 14:09, schrieb Luiz Capitulino:
> On Sat, 22 Mar 2014 21:06:41 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> Am 12.03.2014 00:15, schrieb Cole Robinson:
>>> Cc: Luiz Capitulino <lcapitulino@redhat.com>
>>> Cc: Markus Armbruster <armbru@redhat.com>
>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>>
>> Given that it even had its own stub file, the commit message should
>> please explain why it is no longer needed and thus okay to drop.
> 
> Because it's unused?

That's no real answer. Why did it need to be escaped before the commit
Cole pointed to and now no longer is?

My point was that someone went throught the hassle of creating a stub
file just for this functionality, so it's not some random unused
function in some file. (But I'm not particularly attached to it, just
cautioning about removing too much in this non-verbose way, making it
hard to follow for outsiders.)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 4/6] monitor: Remove unused monitor_print_filename

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 24.03.2014 14:09, schrieb Luiz Capitulino:
>> On Sat, 22 Mar 2014 21:06:41 +0100
>> Andreas Färber <afaerber@suse.de> wrote:
>> 
>>> Am 12.03.2014 00:15, schrieb Cole Robinson:
>>>> Cc: Luiz Capitulino <lcapitulino@redhat.com>
>>>> Cc: Markus Armbruster <armbru@redhat.com>
>>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>
>>>
>>> Given that it even had its own stub file, the commit message should
>>> please explain why it is no longer needed and thus okay to drop.
>> 
>> Because it's unused?
>
> That's no real answer. Why did it need to be escaped before the commit
> Cole pointed to and now no longer is?

Valid question, but it applies to the commits that dropped uses, not to
Cole's patch.

The function goes back to 2006, commit fef3074 "Escape filname printout
properly".  This made "info block" print some but not all funny
characters in filenames more legibly.  Dropped in Kevin's commit fbe2e26
"hmp: Make "info block" output more readable" from last June.  Could be
regarded as regression.  Not sure we care.

A second use was introduced a couple of months later, in commit a9ce859
"info vnc command".  Reason for escaping isn't obvious to me.  Dropped
in commit 1ff7df1 "Enhance 'info vnc' monitor output", which prints
something else entirely.

> My point was that someone went throught the hassle of creating a stub
> file just for this functionality, so it's not some random unused
> function in some file. (But I'm not particularly attached to it, just
> cautioning about removing too much in this non-verbose way, making it
> hard to follow for outsiders.)

Sounds like you'd like a more vebose commit message.  What information
would you like to see?