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

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

[Mark Pizzolato]

Signed-off-by: Mark Pizzolato <mark@infocomm.com>
---
 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);
 		/* Restore the real errno */
 #ifdef _WIN32
 		WSASetLastError(tmperrno);
-- 
1.9.5.msysgit.0

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

[Thomas Huth]

On 22/10/15 01:15, Mark Pizzolato wrote:
> Signed-off-by: Mark Pizzolato <mark@infocomm.com>
> ---
>  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.

 Thomas

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 <mark@infocomm.com>
> > ---
> >  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