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

[Pedro Alves <palves@redhat.com>]

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

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

> +/* 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".

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

> +
> +/* 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.

> +
> +    /* 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?

> +    /* 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.

Thanks,
Pedro Alves