error: Misc cleanups and improvements
cover error: Misc cleanups and improvements
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
Signature · 1 line
1.8.5.3
PATCH 1/6 slirp: Remove default_mon usage
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.
---
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(-)
slirp/misc.c +2 -11
@@ -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[] = {
slirp/slirp.c +4 -4
@@ -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 } }
slirp/slirp.h +0 -2
@@ -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
Signature · 1 line
1.8.5.3
Re: [Qemu-devel] [PATCH 1/6] slirp: Remove default_mon usage
Quoted 93 lines · 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 <[email protected]>Signed-off-by Cole Robinson <[email protected]>---
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(-)slirp/misc.c +2
-11@@ -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[] = {slirp/slirp.c +4
-4@@ -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 } }slirp/slirp.h +0
-0
slirp/slirp.h void if_start(struct ttys *); · line 292
Quoted 8 lines
slirp/slirp.h +0
-2@@ -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
I suppose this goes through Luiz' queue?
Jan
Signature · 2 lines
Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux
Re: [Qemu-devel] [PATCH 1/6] slirp: Remove default_mon usage
Quoted 105 lines · On Wed, 12 Mar 2014 08:22:05 +0100 Jan Kiszka <[email protected]> wrote:
Quoted 93 lines · 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 <[email protected]>Signed-off-by Cole Robinson <[email protected]>---
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(-)slirp/misc.c +2
-11@@ -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[] = {slirp/slirp.c +4
-4@@ -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 } }slirp/slirp.h +0
-0slirp/slirp.h void if_start(struct ttys *); · line 292
Quoted 8 lines
slirp/slirp.h +0
-2@@ -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 <[email protected]>
Quoted 1 line
I suppose this goes through Luiz' queue?
Yes.
Re: [Qemu-devel] [PATCH 1/6] slirp: Remove default_mon usage
In reply to Jan KiszkaAm 12.03.2014 08:22, schrieb Jan Kiszka:
Quoted 96 lines · Cole Robinson
Quoted 95 lines
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 <[email protected]>Signed-off-by Cole Robinson <[email protected]>---
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(-)slirp/misc.c +2
-11@@ -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[] = {slirp/slirp.c +4
-4@@ -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 } }slirp/slirp.h +0
-0@@ -287,8 +287,6 @@ void if_start(struct ttys *);long gethostid (void );#endif
Quoted 8 lines
slirp/slirp.h void if_start(struct ttys *); · line 292
Quoted 6 lines
slirp/slirp.h +0
-2-void lprint(const char *, ...) GCC_FMT_ATTR(1, 2); -#ifndef _WIN32#include <netdb.h> #endif Reviewed-by Jan Kiszka <[email protected]>
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
Quoted 5 lines
I suppose this goes through Luiz' queue?
Jan
Signature · 2 lines
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
Quoted 106 lines · Andreas Färber
Am 12.03.2014 08:22, schrieb Jan Kiszka:
Quoted 96 lines · Cole Robinson
Quoted 95 lines
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 <[email protected]>Signed-off-by Cole Robinson <[email protected]>---
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(-)slirp/misc.c +2
-11@@ -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[] = {slirp/slirp.c +4
-4@@ -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 } }slirp/slirp.h +0
-0@@ -287,8 +287,6 @@ void if_start(struct ttys *);long gethostid (void );#endif Quoted 8 lines
slirp/slirp.h void if_start(struct ttys *); · line 292
Quoted 6 lines
slirp/slirp.h +0
-2-void lprint(const char *, ...) GCC_FMT_ATTR(1, 2); -#ifndef _WIN32#include <netdb.h> #endif Reviewed-by Jan Kiszka <[email protected]>
Quoted 4 lines
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 1/6] slirp: Remove default_mon usage
In reply to Cole RobinsonQuoted 20 lines · 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 <[email protected]>Signed-off-by Cole Robinson <[email protected]>---
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(-)slirp/misc.c +0
-0
slirp/misc.c fork_exec(struct socket *so, const char *ex, int do_pty) · line 142
Quoted 8 lines
slirp/misc.c +1
-1@@ -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 1/6] slirp: Remove default_mon usage
Quoted 29 lines · Markus Armbruster
Quoted 27 lines · 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 <[email protected]>Signed-off-by Cole Robinson <[email protected]>---
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(-)slirp/misc.c +1
-1@@ -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);
Quoted 6 lines
slirp/misc.c fork_exec(struct socket *so, const char *ex, int do_pty) · line 142
Quoted 1 line
slirp/misc.c +0
-0return 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
Quoted 37 lines · Cole Robinson
Quoted 29 lines · Markus Armbruster
Quoted 27 lines · 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 <[email protected]>Signed-off-by Cole Robinson <[email protected]>---
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(-)slirp/misc.c +1
-1@@ -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);Quoted 6 lines
slirp/misc.c fork_exec(struct socket *so, const char *ex, int do_pty) · line 142
Quoted 1 line
slirp/misc.c +0
-0return 0 ;Why not
error_report()?[...]
Quoted 7 lines
AFAICT this is only guest initiated, never from the monitor.
error_reportsends output tocur_monif 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.
PATCH 2/6 vnc: Remove default_mon usage
In reply to Cole RobinsonThese errors don't seem user initiated, so forcibly printing to the monitor doesn't seem right. Just print to stderr.
--- ui/vnc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ui/vnc.c +2 -2
@@ -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" ); } }
Signature · 1 line
1.8.5.3
Re: [Qemu-devel] [PATCH 2/6] vnc: Remove default_mon usage
Quoted 6 lines · 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 <[email protected]>Cc Gerd Hoffmann <[email protected]>Signed-off-by Cole Robinson <[email protected]>
Re: [Qemu-devel] [PATCH 2/6] vnc: Remove default_mon usage
Am 12.03.2014 08:35, schrieb Gerd Hoffmann:
Quoted 9 lines · Cole Robinson
Quoted 6 lines
These errors don't seem user initiated, so forcibly printing to the monitor doesn't seem right. Just print to stderr.
Cc Anthony Liguori <[email protected]>Cc Gerd Hoffmann <[email protected]>Signed-off-by Cole Robinson <[email protected]>Reviewed-by Gerd Hoffmann <[email protected]>
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
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
PATCH 3/6 error: Privatize error_print_loc
In reply to Cole Robinson--- include/qemu/error-report.h | 1 - util/qemu-error.c | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-)
include/qemu/error-report.h +0 -1
@@ -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 );
util/qemu-error.c +1 -1
@@ -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;
Signature · 1 line
1.8.5.3
Re: [Qemu-devel] [PATCH 3/6] error: Privatize error_print_loc
Am 12.03.2014 00:15, schrieb Cole Robinson:
Quoted 7 lines
Cc Luiz Capitulino <[email protected]>Cc Markus Armbruster <[email protected]>Signed-off-by Cole Robinson <[email protected]>--- 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,
Andreas
Signature · 2 lines
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
PATCH 4/6 monitor: Remove unused monitor_print_filename
In reply to Cole Robinson--- 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
include/monitor/monitor.h +0 -1
@@ -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 );
monitor.c +0 -27
@@ -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, ...) {
stubs/Makefile.objs +0 -1
@@ -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
stubs/mon-print-filename.c deleted +0 -6
@@ -1,6 +0,0 @@ -#include "qemu-common.h" -#include "monitor/monitor.h" - -void monitor_print_filename (Monitor *mon,const char *filename) -{ -}
Signature · 1 line
1.8.5.3
Re: [Qemu-devel] [PATCH 4/6] monitor: Remove unused monitor_print_filename
Am 12.03.2014 00:15, schrieb Cole Robinson:
Quoted 3 lines
Cc Luiz Capitulino <[email protected]>Cc Markus Armbruster <[email protected]>Signed-off-by Cole Robinson <[email protected]>
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
Signature · 2 lines
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
Quoted 11 lines · Andreas Färber
Am 12.03.2014 00:15, schrieb Cole Robinson:
Quoted 3 lines
Cc Luiz Capitulino <[email protected]>Cc Markus Armbruster <[email protected]>Signed-off-by Cole Robinson <[email protected]>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 <[email protected]> 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
In reply to Andreas FärberQuoted 7 lines · On Sat, 22 Mar 2014 21:06:41 +0100 Andreas Färber <[email protected]> wrote:
Am 12.03.2014 00:15, schrieb Cole Robinson:
Quoted 3 lines
Cc Luiz Capitulino <[email protected]>Cc Markus Armbruster <[email protected]>Signed-off-by Cole Robinson <[email protected]>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
Am 24.03.2014 14:09, schrieb Luiz Capitulino:
Quoted 4 lines
Quoted 1 line · On Sat, 22 Mar 2014 21:06:41 +0100 Andreas Färber <[email protected]> wrote:
Am 12.03.2014 00:15, schrieb Cole Robinson:
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Cc Luiz Capitulino <[email protected]>Cc Markus Armbruster <[email protected]>Signed-off-by Cole Robinson <[email protected]>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
Signature · 2 lines
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
Quoted 9 lines · Andreas Färber
Am 24.03.2014 14:09, schrieb Luiz Capitulino:
Quoted 8 lines
Quoted 5 lines · On Sat, 22 Mar 2014 21:06:41 +0100 Andreas Färber <[email protected]> wrote:
Am 12.03.2014 00:15, schrieb Cole Robinson:
Quoted 3 lines
Cc Luiz Capitulino <[email protected]>Cc Markus Armbruster <[email protected]>Signed-off-by Cole Robinson <[email protected]>
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 5 lines
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?
PATCH 5/6 error: Remove redundant error_printf_unless_qmp
In reply to Cole Robinsonerror_printf is just a wrapper around monitor_printf, which already drops the requested output if cur_mon is qmp.
--- 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(-)
hw/usb/bus.c +1 -1
@@ -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); }
hw/usb/hcd-ehci.c +2 -2
@@ -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 ;
include/qemu/error-report.h +0 -1
@@ -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 );
util/qemu-error.c +0 -11
@@ -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 };
util/qemu-option.c +0 -7
@@ -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);
Signature · 1 line
1.8.5.3
Re: [Qemu-devel] [PATCH 5/6] error: Remove redundant error_printf_unless_qmp
Quoted 2 lines · Cole Robinson
error_printfis just a wrapper aroundmonitor_printf, which already drops the requested output ifcur_monis 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.
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, ...
Quoted 85 lines
Cc Luiz Capitulino <[email protected]>Cc Markus Armbruster <[email protected]>Signed-off-by Cole Robinson <[email protected]>--- 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(-)hw/usb/bus.c +1
-1@@ -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); }hw/usb/hcd-ehci.c +2
-2@@ -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 ;include/qemu/error-report.h +0
-1@@ -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 );util/qemu-error.c +0
-11@@ -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 };util/qemu-option.c +0
-0
util/qemu-option.c void parse_option_size(const char *name, const char *value, · line 206
Quoted 10 lines
util/qemu-option.c +0
-4@@ -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.
util/qemu-option.c QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id, · line 812
Quoted 10 lines
util/qemu-option.c +0
-3@@ -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 5/6] error: Remove redundant error_printf_unless_qmp
Quoted 63 lines · Markus Armbruster
Quoted 2 lines · Cole Robinson
error_printfis just a wrapper aroundmonitor_printf, which already drops the requested output ifcur_monis 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 <[email protected]>Reviewed-by Markus Armbruster <[email protected]>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(), witherror_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(), witherror_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 thanerror_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.
Quoted 7 lines
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
PATCH 6/6 error: Print error_report() to stderr if using qmp
In reply to Cole Robinsonmonitor_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.
--- util/qemu-error.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
util/qemu-error.c +1 -1
@@ -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);
Signature · 1 line
1.8.5.3