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

["Andreas Färber" <afaerber@suse.de>]

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