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

[Pedro Alves <palves@redhat.com>]

Hi Sergio,

I noticed this when applying the patch:

 $ git am /tmp/sergio.mbox
 Applying: Implement IPv6 support for GDB/gdbserver
 .git/rebase-apply/patch:982: trailing whitespace.
           do 
 .git/rebase-apply/patch:985: trailing whitespace.
             } 
 warning: 2 lines add whitespace errors.

You can check it locally with:

 $ git show --check

 gdb/ser-tcp.c:256: trailing whitespace.
 +         do 
 gdb/ser-tcp.c:259: trailing whitespace.
 +           } 

Comments on the patch below.

> 
> Another thing worth mentioning is the new 'GDB_TEST_IPV6' testcase
> parameter, which instructs GDB and gdbserver to use IPv6 for
> connections.  This way, if you want to run IPv6 tests, you do:
> 
>   $ make check-gdb RUNTESTFLAGS='GDB_TEST_IPV6=1'

That sounds useful, but:

#1 - I don't see how that works without also passing
     --target_board= pointing at one of the native-gdbserver and
     native-extended-gdbserver board files.  
     Can you expand on why you took this approach instead of:
 
  a) handling GDB_TEST_IPV6 somewhere central, like
     in gdb/testsuite/gdbserver-support.exp, where we
     default to "localhost:".  That would exercise the gdb.server/
     tests with ipv6, when testing with the default/unix board file.

  b) add new board files to test with ipv6, like native-gdbserver-v6
     or something like that.

  c) both?

#2 - I think it'd be also useful to have some gdb.server/ test that
     runs with the default board and exercises / smoke tests ipv6.
     (And if we have that, we might as well iterate the test on udp/udpv6
     too.)

#3 - Actually, this makes me wonder about changing the variable's
     spelling from GDB_TEST_IPV6=1 to something like
     GDB_TEST_SOCKETHOST and then one would be able to set it to:

      "localhost:",
      "localhost6:"
      "tcp:localhost6:"
      "\[::1\]:"
      "udp:127.0.0.1:"

     or whatever one would like.

#4 - Why do we need to override get_comm_port too, here? :

     -set_board_info sockethost "localhost:"
     +if { [info exists GDB_TEST_IPV6] } {
     +    set_board_info sockethost "tcp6:\[::1\]:"
     +    set_board_info gdbserver,get_comm_port get_comm_port_localhost_ipv6
     +} else {
     +    set_board_info sockethost "localhost:"
     +}

    Doesn't overriding "sockethost" alone work?  Why not?



>  gdb/Makefile.in                                    |   2 +
>  gdb/NEWS                                           |   4 +
>  gdb/common/netstuff.c                              | 136 +++++++++++++
>  gdb/common/netstuff.h                              |  52 +++++
>  gdb/doc/gdb.texinfo                                |  48 ++++-
>  gdb/gdbserver/Makefile.in                          |   2 +
>  gdb/gdbserver/gdbreplay.c                          | 181 +++++++++++++----
>  gdb/gdbserver/remote-utils.c                       | 119 +++++++----
>  gdb/ser-tcp.c                                      | 217 ++++++++++-----------
>  gdb/testsuite/README                               |   7 +
>  gdb/testsuite/boards/gdbserver-base.exp            |   5 +
>  gdb/testsuite/boards/native-extended-gdbserver.exp |   7 +-
>  gdb/testsuite/boards/native-gdbserver.exp          |   7 +-
>  .../gdb.server/run-without-local-binary.exp        |   2 +-
>  14 files changed, 602 insertions(+), 187 deletions(-)
>  create mode 100644 gdb/common/netstuff.c
>  create mode 100644 gdb/common/netstuff.h
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index df6ebab851..06ce12a4ee 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -962,6 +962,7 @@ COMMON_SFILES = \
>  	common/job-control.c \
>  	common/gdb_tilde_expand.c \
>  	common/gdb_vecs.c \
> +	common/netstuff.c \
>  	common/new-op.c \
>  	common/pathstuff.c \
>  	common/print-utils.c \
> @@ -1443,6 +1444,7 @@ HFILES_NO_SRCDIR = \
>  	common/gdb_vecs.h \
>  	common/gdb_wait.h \
>  	common/common-inferior.h \
> +	common/netstuff.h \
>  	common/host-defs.h \
>  	common/pathstuff.h \
>  	common/print-utils.h \
> diff --git a/gdb/NEWS b/gdb/NEWS
> index cef558039e..1f95ced912 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -3,6 +3,10 @@
>  
>  *** Changes since GDB 8.1
>  
> +* GDB and GDBserver now support IPv6 connections.  IPv6 hostnames
> +  can be passed using the '[ADDRESS]:PORT' notation, or the regular
> +  'ADDRESS:PORT' method.

Saying "IPv6 hostnames" and then talking about "ADDRESS:PORT" is
confusing, I think.  If we're talking about host names then saying
HOST:PORT would more accurate.  But I think that what you really
mean is to say "IPv6 _addresses_ can be passed".

Does connecting with "localhost6:port" default to IPv6, BTW?
At least fedora includes "localhost6" in /etc/hosts.


> +/* See common/netstuff.h.  */
> +
> +void
> +parse_hostname_without_prefix (const char *hostname, std::string &host_str,
> +			       std::string &port_str, struct addrinfo *hint)
> +{
> +  std::string strname (hostname);

I suspect the local parsing can be written using
gdb::string_view to avoid copying?

> +
> +/* See common/netstuff.h.  */
> +
> +void
> +parse_hostname (const char *hostname, std::string &host_str,
> +		std::string &port_str, struct addrinfo *hint)
> +{
> +  /* Struct to hold the association between valid prefixes, their
> +     family and socktype.  */
> +  struct host_prefix
> +    {
> +      /* The prefix.  */
> +      const char *prefix;
> +
> +      /* The 'ai_family'.  */
> +      int family;
> +
> +      /* The 'ai_socktype'.  */
> +      int socktype;
> +    };
> +  static const struct host_prefix prefixes[] =
> +    {
> +      { "udp:",  AF_UNSPEC, SOCK_DGRAM },
> +      { "tcp:",  AF_UNSPEC, SOCK_STREAM },
> +      { "udp4:", AF_INET,   SOCK_DGRAM },
> +      { "tcp4:", AF_INET,   SOCK_STREAM },
> +      { "udp6:", AF_INET6,  SOCK_DGRAM },
> +      { "tcp6:", AF_INET6,  SOCK_STREAM },
> +      { NULL, 0, 0 },
> +    };
> +
> +  for (const struct host_prefix *prefix = prefixes;
> +       prefix->prefix != NULL;
> +       ++prefix)

I think you could drop the last/null entry and use range-for ?

> +    if (startswith (hostname, prefix->prefix))
> +      {
> +	hostname += strlen (prefix->prefix);
> +	hint->ai_family = prefix->family;
> +	hint->ai_socktype = prefix->socktype;
> +	hint->ai_protocol
> +	  = hint->ai_socktype == SOCK_DGRAM ? IPPROTO_UDP : IPPROTO_TCP;
> +	break;
> +      }
> +
> +  parse_hostname_without_prefix (hostname, host_str, port_str, hint);
> +}


> +/* Parse HOSTNAME (which is a string in the of "ADDR:PORT") and fill

Missing "form" in "in the of".

> +   in HOST_STR, PORT_STR and HINT accordingly.  */
> +extern void parse_hostname_without_prefix (const char *hostname,
> +					   std::string &host_str,
> +					   std::string &port_str,
> +					   struct addrinfo *hint);
> +
> +/* Parse HOSTNAME (which is a string in the form of
> +   "[tcp[6]:|udp[6]:]ADDR:PORT") and fill in HOST_STR, PORT_STR and
> +   HINT accordingly.  */
> +extern void parse_hostname (const char *hostname, std::string &host_str,
> +			    std::string &port_str, struct addrinfo *hint);

Really not a big deal, but instead of output parameters, I'd
consider returning all outputs via return.  Something like:

struct parsed_hostname
{
  std::string host_str;
  std::string port_str;
  struct addrinfo addrinfo;
};
extern parsed_hostname parse_hostname (const char *hostname,
                                       const struct addrinfo &hint);



>  For example, to connect to port 2828 on a terminal server named
>  @code{manyfarms}:
> @@ -20514,6 +20525,24 @@ For example, to connect to port 2828 on a terminal server named
>  target remote manyfarms:2828
>  @end smallexample
>  
> +To connect to port 2828 on a terminal server whose address is
> +@code{2001::f8ff::67cf}, you can either use the square bracket syntax:
> +
> +@smallexample
> +target remote [2001::f8ff::67cf]:2828
> +@end smallexample
> +
> +Or explicitly specify the @acronym{IPv6} protocol:
> +
> +@smallexample
> +target remote tcp6:2001::f8ff::67cf:2828
> +@end smallexample
> +
> +This last example may be confusing to the reader, because there is no
> +visible separation between the hostname and the port number.

Is that really true?  It seems there's visible separation to me -- the
address/hosthoname part uses double colon, while the port name is
separated by a single colon?


> +Therefore, we recommend the user to provide @acronym{IPv6} addresses
> +using square brackets for clarity.
> +

>  #ifndef HAVE_SOCKLEN_T
> @@ -175,56 +176,159 @@ remote_close (void)
>  static void
>  remote_open (char *name)
>  {
> -  if (!strchr (name, ':'))
> +  char *last_colon = strrchr (name, ':');
> +
> +  if (last_colon == NULL)
>      {
>        fprintf (stderr, "%s: Must specify tcp connection as host:addr\n", name);
>        fflush (stderr);
>        exit (1);
>      }
> -  else
> -    {
> +
>  #ifdef USE_WIN32API
> -      static int winsock_initialized;
> +  static int winsock_initialized;
>  #endif
> -      char *port_str;
> -      int port;
> -      struct sockaddr_in sockaddr;
> -      socklen_t tmp;
> -      int tmp_desc;
> +  char *port_str;
> +  int tmp;
> +  int tmp_desc;
> +  struct addrinfo hint;
> +  struct addrinfo *ainfo;
> +  char *orig_name = strdup (name);

Do we need a deep copy?  And if we do, how about
using std::string to avoid having to call free further
down?

> +
> +  struct prefix
> +  {
> +    /* The prefix to be parsed.  */
> +    const char *str;
> +
> +    /* The address family.  */
> +    int ai_family;
> +
> +    /* The socktype.  */
> +    int ai_socktype;
> +  };
> +  static const struct prefix prefixes[]
> +    = { { "udp:",  AF_UNSPEC, SOCK_DGRAM },
> +	{ "tcp:",  AF_UNSPEC, SOCK_STREAM },
> +	{ "udp4:", AF_INET,   SOCK_DGRAM },
> +	{ "tcp4:", AF_INET,   SOCK_STREAM },
> +	{ "udp6:", AF_INET6,  SOCK_DGRAM },
> +	{ "tcp6:", AF_INET6,  SOCK_STREAM },
> +	{ NULL, 0, 0 } };

That seems like unusual formatting.  In common/netstuff.c
you broke the starting and ending '{' }' differently.

I wonder though, shouldn't this be using the new
netstuff.c shared routines?  It looks like duplicated code?

> +
> +  memset (&hint, 0, sizeof (hint));
> +  /* Assume no prefix will be passed, therefore we should use
> +     AF_UNSPEC.  */
> +  hint.ai_family = AF_UNSPEC;
> +  hint.ai_socktype = SOCK_STREAM;
> +  hint.ai_protocol = IPPROTO_TCP;
> +
> +  for (const struct prefix *p = prefixes; p->str != NULL; ++p)

Same comment about range-for.

> +
> +  if (bind (tmp_desc, p->ai_addr, p->ai_addrlen) != 0)
> +    perror_with_name ("Can't bind address");
> +
> +  if (p->ai_socktype == SOCK_DGRAM)
> +    remote_desc = tmp_desc;
> +  else
> +    {
> +      struct sockaddr_storage sockaddr;
> +      socklen_t sockaddrsize = sizeof (sockaddr);
> +      char orig_host[64], orig_port[16];

I guess these magic sizes are garanteed to be enough, since
you specify NI_NUMERICHOST | NI_NUMERICSERV.  Correct?
A comment or giving those constants names or comments
would be good.  Something like:

/* Like NI_MAXHOST/NI_MAXSERV, but enough for numeric forms.  */
#define GDB_NI_MAX_ADDR 64
#define GDB_NI_MAX_PORT 16

>  #if __QNX__
> @@ -156,19 +159,18 @@ enable_async_notification (int fd)
>  static int
>  handle_accept_event (int err, gdb_client_data client_data)
>  {
> -  struct sockaddr_in sockaddr;
> -  socklen_t tmp;
> +  struct sockaddr_storage sockaddr;
> +  socklen_t len = sizeof (sockaddr);
>  
>    if (debug_threads)
>      debug_printf ("handling possible accept event\n");
>  
> -  tmp = sizeof (sockaddr);
> -  remote_desc = accept (listen_desc, (struct sockaddr *) &sockaddr, &tmp);
> +  remote_desc = accept (listen_desc, (struct sockaddr *) &sockaddr, &len);
>    if (remote_desc == -1)
>      perror_with_name ("Accept failed");
>  
>    /* Enable TCP keep alive process. */
> -  tmp = 1;
> +  socklen_t tmp = 1;
>    setsockopt (remote_desc, SOL_SOCKET, SO_KEEPALIVE,
>  	      (char *) &tmp, sizeof (tmp));
>  
> @@ -197,8 +199,19 @@ handle_accept_event (int err, gdb_client_data client_data)
>    delete_file_handler (listen_desc);
>  
>    /* Convert IP address to string.  */
> -  fprintf (stderr, "Remote debugging from host %s\n",
> -	   inet_ntoa (sockaddr.sin_addr));
> +  char orig_host[64], orig_port[16];

Same comment as for gdbreplay.

> +
> +  int r = getnameinfo ((struct sockaddr *) &sockaddr, len,
> +		       orig_host, sizeof (orig_host),
> +		       orig_port, sizeof (orig_port),
> +		       NI_NUMERICHOST | NI_NUMERICSERV);
> +
> +  if (r != 0)
> +    fprintf (stderr, _("Could not obtain remote address: %s\n"),
> +	     gai_strerror (r));
> +  else
> +    fprintf (stderr, "Remote debugging from host %s, port %s\n", orig_host,
> +	     orig_port);

While at it, couple you please add the missing _() for i18n.
BTW, is that line too long?  Can't tell from email client.


>  
> @@ -354,18 +399,24 @@ remote_open (const char *name)
>  #endif /* USE_WIN32API */
>    else
>      {
> -      int port;
> -      socklen_t len;
> -      struct sockaddr_in sockaddr;
> -
> -      len = sizeof (sockaddr);
> -      if (getsockname (listen_desc,
> -		       (struct sockaddr *) &sockaddr, &len) < 0
> -	  || len < sizeof (sockaddr))
> +      char listen_port[16];
> +      struct sockaddr_storage sockaddr;
> +      socklen_t len = sizeof (sockaddr);
> +
> +      if (getsockname (listen_desc, (struct sockaddr *) &sockaddr, &len) < 0)
>  	perror_with_name ("Can't determine port");
> -      port = ntohs (sockaddr.sin_port);
>  
> -      fprintf (stderr, "Listening on port %d\n", port);
> +      int r = getnameinfo ((struct sockaddr *) &sockaddr, len,
> +			   NULL, 0,
> +			   listen_port, sizeof (listen_port),
> +			   NI_NUMERICSERV);
> +
> +      if (r != 0)
> +	fprintf (stderr, _("Can't obtain port where we are listening: %s"),
> +		 gai_strerror (r));
> +      else
> +	fprintf (stderr, "Listening on port %s\n", listen_port);

Preexisting, but while at it, adding the _() wouldn't hurt.

Thanks,
Pedro Alves