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

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Wednesday, July 11 2018, Pedro Alves wrote:

> Hi,
>
> This version looks mostly good to me.  Only one thing raised
> my eyebrow.  I also noticed a few typos here and there.
> See below.
>
> On 07/07/2018 09:47 PM, Sergio Durigan Junior wrote:
>
>> -/* Open a tcp socket.  */
>> +/* Try to connect to the host represented by AINFO.  If the connection
>> +   succeeds, return its socket.  If we get a ECONNREFUSED error,
>> +   return -1.  Otherwise, return nothing.  POLLS is used when
>> +   'connect' returns EINPROGRESS, and we need to invoke
>> +   'wait_for_connect' to obtain the status.  */
>>  
>> -int
>> -net_open (struct serial *scb, const char *name)
>> +static gdb::optional<int>
>> +try_connect (const struct addrinfo *ainfo, unsigned int *polls)
>>  {
>
> I find this use of gdb::optional<int> more confusing than
> helpful, since it creates multiple "levels" of places
> to check error.  You have:
>
>  #1 - empty optional, check errno for error [1]
>  #2 - non-empty optional, value == -1, means ECONNREFUSED.
>  #3 - non-empty optional, value is socket.
>
> ([1] the errno part is missing in the function's description)
>
> Above, #1 and #2 seem redundant?
>
> You can instead make the return type plain int, return -1 on
> all errors, and the caller can check errno for ECONNREFUSED.
>
> (If we were going to avoid errno, I think something like
> std::expected would be better than std/gdb::optional.  std::optional
> for a type that already has a nullable state (in this case -1)
> tends to be confusing, IMO.  See here for example:
> https://www.boost.org/doc/libs/1_61_0/libs/optional/doc/html/boost_optional/tutorial/when_to_use_optional.html
> )
>
> Notice how your patch left this case incorrectly returning -1:
>
>       if (n < 0)
> 	{
> 	  /* A negative value here means that we either timed out or
> 	     got interrupted by the user.  Just return.  */
> 	  close (sock);
> 	  return -1;
> 	}

No problem.  I've changed the code to use plain int now.

>> -    if [target_info exists sockethost] {
>> +    if { [info exists GDB_TEST_SOCKETHOST] } {
>> +	# The user is not supposed to provide a port number, just a
>> +	# hostname/address, therefore we add the trailing ":" here.
>> +	set debughost "${GDB_TEST_SOCKETHOST}:"
>> +	# Espace open and close square brackets.
>
> Typo: "Espace" -> "Escape"

Fixed.

>> +/* Initialized an unprefixed entry.  In this case, we don't expect
>> +   nothing on the 'struct addrinfo' HINT.  */
>> +#define INIT_UNPREFIXED_ENTRY(ADDR, EXP_HOST, EXP_PORT) \
>> +  INIT_ENTRY (ADDR, EXP_HOST, EXP_PORT, false, 0, 0, 0)
>
> Typo: "Initialized" -> "Initialize".

Fixed.

> Also, double negatives "don't expect nothing" don't work well in English.
>
> Do you mean "don't expect anything", or "expect nothing" ?
> (There's a difference: the former doesn't expect it but
> tolerates, the latter really requires nothing.)

We don't really care about HINT, so "don't expect anything" is the
proper one to use here.  Fixed.

>> +
>> +/* Initialized an unprefixed IPv6 entry.  In this case, we don't
>> +   expect nothing on the 'struct addrinfo' HINT.  */
>> +#define INIT_UNPREFIXED_IPV6_ENTRY(ADDR, EXP_HOST, EXP_PORT) \
>> +  INIT_ENTRY (ADDR, EXP_HOST, EXP_PORT, false, AF_INET6, 0, 0)
>
> Same comments apply here.

Fixed.

>> +
>> +    /* IPv6, host and port present, no brackets.  */
>> +    INIT_UNPREFIXED_ENTRY ("::1:1234", "::1", "1234"),
>> +    /* IPv6, only host, no brackets.  */
>> +    // INIT_UNPREFIXED_ENTRY ("::1", "::1", ""),
>
> Leftover, remove?

Indeed, removed.

>> +    /* IPv6, missing port, no brackets.  */
>> +    INIT_UNPREFIXED_ENTRY ("::1:", "::1", ""),
>
>> +    /* Prefixed "tcp4:" IPv4, host and port presents.  */
>
> Typo: "presents" -> "present".  This appears in several places.

Fixed all of them.

I'll send a v4 soon.

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