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

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Wednesday, July 11 2018, Pedro Alves wrote:

> 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.

Done.

>>  
>>        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.

I don't know why I removed the existing comment.  Sorry about that.  I
readded it and rearranged them accordingly.

>>  	  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.

That's true, mistake on my part.

>  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;

Right, will do that.

>> +      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;

That's true.  And also, in this case, I have to guard the errno checking
that happens on net_open using #ifdef USE_WIN32API...#else...#endif.
Done that as well.

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

Thanks for the reviews.  Pushed.

c7ab0aef11d91b637bf091aa9176b8dc4aadee46

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/