Re: [Qemu-devel] [PATCH 1/5] slirp: closesocket must be called to close sockets on windows

[Mark Pizzolato]

On Tuesday, October 27, 2015 at 7:28 AM, Thomas Huth wrote:
> On 22/10/15 01:15, Mark Pizzolato wrote:
> > Signed-off-by: Mark Pizzolato <address@hidden>
> > ---
> >  slirp/slirp.c  | 2 +-
> >  slirp/socket.c | 3 ++-
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/slirp/slirp.c b/slirp/slirp.c index 35f819a..d18faa8
> > 100644
> > --- a/slirp/slirp.c
> > +++ b/slirp/slirp.c
> > @@ -846,7 +846,7 @@ int slirp_remove_hostfwd(Slirp *slirp, int is_udp,
> struct in_addr host_addr,
> >              getsockname(so->s, (struct sockaddr *)&addr, &addr_len) == 0 &&
> >              addr.sin_addr.s_addr == host_addr.s_addr &&
> >              addr.sin_port == port) {
> > -            close(so->s);
> > +            closesocket(so->s);
> >              sofree(so);
> >              return 0;
> >          }
> > diff --git a/slirp/socket.c b/slirp/socket.c index 37ac5cf..4a20e08
> > 100644
> > --- a/slirp/socket.c
> > +++ b/slirp/socket.c
> > @@ -632,8 +632,9 @@ tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport,
> uint32_t laddr,
> >         (listen(s,1) < 0)) {
> >             int tmperrno = errno; /* Don't clobber the real reason we
> failed */
> >
> > -           close(s);
> > +           closesocket(s);
> >             sofree(so);
> > +           fprintf (stderr, "Socket Error %d", tmperrno);
> 
> Looks like you've left some debugging code in here? I think that should be
> removed.

Well, I thought of that, but then I followed the example in soreadbuf() in this 
same 
module where these relatively rare, maybe never, conditions happen.  If a bug 
report 
came with some stderr output maybe a useful solution could be provided.  In 
both 
cases, these are error path situations which the code should handle cleanly 
(without 
leaking memory or sockets).  I did not actually encounter this issue during 
execution.  
I merely noticed the problem during a code review while porting to windows.

I'm not stuck on this, but having these there (in both cases) seems at least 
consistent 
with the other code in the same module.

A different effort could be pursued to enable these and other cases where 
stderr is 
written to in the slirp code and only produce it in debug mode.  This patch is 
trying to 
minimize changes now.

Other commits supplied with this one generalized the existing explicit debug 
output 
calls produced by the DEBUG_CALL, DEBUG_ARG, DEBUG_ARGS, DEBUG_MISC, 
DEBUG_ERROR and DPRINTF macros.

If more comments come back on the rest of those patches, I'll either add an 
additional patch set to the existing ones to remove the explicit references to 
stderr 
and use the various debug macros as needed or I redo the whole patch set.  
Please
suggest the best course of action.

- Mark