| Message ID | 176c909f6e01409b9dd8d37108f2df5dcf8edd17.1394579440.git.crobinso@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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()? [...]
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.
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
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.
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 >
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
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
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(-)