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

[Cole Robinson <crobinso@redhat.com>]

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