Re: [PATCH v4] Implement IPv6 support for GDB/gdbserver

[Pedro Alves <palves at redhat dot com>]

On 07/11/2018 08:16 PM, Sergio Durigan Junior wrote:
> Changes since v3:
> 
> - No longer use gdb::optional as a return type for try_connect; use
>   int instead.
> 
> - Fix a bunch of typos and thinkos.
> 

Thanks.  

> +/* Helper class to guarantee that we always call 'freeaddrinfo'.  */
> +
> +class scoped_free_addrinfo
> +{
> +public:
> +  /* Default constructor.  */
> +  scoped_free_addrinfo (struct addrinfo *ainfo)
> +    : m_res (ainfo)

explicit.

>  
>        if (
>  #ifdef USE_WIN32API
> -	  /* Under Windows, calling "connect" with a non-blocking socket
> -	     results in WSAEWOULDBLOCK, not WSAEINPROGRESS.  */
> +	  /* Any other error (except EINPROGRESS) will be "swallowed"
> +	     here.  We return without specifying a return value, and
> +	     set errno if the caller wants to inspect what
> +	     happened.  */

This comment should be outside "#ifdef USE_WIN32API", and the
existing comment "Under Windows, ... WSAEWOULDBLOCK" should be
preserved.

>  	  err != WSAEWOULDBLOCK
>  #else
>  	  err != EINPROGRESS
>  #endif
>  	  )
>  	{
> +	  close (sock);
>  	  errno = err;
> -	  net_close (scb);
>  	  return -1;
>  	}
>  

> +  /* On Windows, the fourth parameter to getsockopt is a "char *";
> +     on UNIX systems it is generally "void *".  The cast to "char *"
> +     is OK everywhere, since in C++ any data pointer type can be
> +     implicitly converted to "void *".  */
> +  int ret = getsockopt (sock, SOL_SOCKET, SO_ERROR, (char *) &err, &len);
> +
> +  if (ret < 0)
> +    {
> +      close (sock);
> +      errno = ret;

I don't think this "errno = ret" here is right.  getsockopt returns
-1 on error with errno already set.  Pedantically, close can 
fail and set errno, so the correct thing to do is:

 int save_errno = errno;
 close (sock);
 errno = save_errno;
 return -1;

> +      return -1;
> +    }
> +  else if (ret == 0 && err != 0)
> +    {
> +      close (sock);
> +      errno = err;
> +
> +      /* Check if the connection was refused.  */
> +      if (
>  #ifdef USE_WIN32API
> -	    && err == WSAECONNREFUSED
> +	  err == WSAECONNREFUSED
>  #else
> -	    && err == ECONNREFUSED
> +	  err == ECONNREFUSED
>  #endif
> -	    && wait_for_connect (NULL, &polls) >= 0)
> -	  {
> -	    close (scb->fd);
> -	    goto retry;
> -	  }
> -	if (err)
> -	  errno = err;
> -	net_close (scb);
> +	  )
>  	return -1;> -      }
> -  } 
> +      else
> +	{
> +	  /* If we have any other kind of error, just return nothing.  */
> +	  return -1;
> +	}

Aren't both the then/else branches exactly the same, i.e.,
just "return -1;" ?

Seems like you can all the "if then/else", and just return -1;

> +    }
> +
> +  /* The connection succeeded.  Return the socket.  */
> +  return sock;
> +}
> +
OK with the issues above fixed.

Thanks,
Pedro Alves