Re: [PATCH v3] Add IPv6 support for outgoing remote TCP connections

[Jan Kratochvil <jan.kratochvil@redhat.com>]

On Tue, 11 Feb 2014 20:48:09 +0100, Paul Fertser wrote:
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -512,6 +512,9 @@ AC_CHECK_FUNC(wctype, [],
>  # Some systems (e.g. Solaris) have `gethostbyname' in libnsl.
>  AC_SEARCH_LIBS(gethostbyname, nsl)
>  
> +# Borrow from gnulib, Solaris might additionally need nsl

Missing trailing dot (.).

> +AC_SEARCH_LIBS(getaddrinfo, [socket network net], [], [], [-lnsl])

Where is it stated in gnulib?  I could not find it.


> +
>  # Some systems (e.g. Solaris) have `socketpair' in libsocket.
>  AC_SEARCH_LIBS(socketpair, socket)
>  
> diff --git a/gdb/ser-tcp.c b/gdb/ser-tcp.c
> index c288ab4..15f7648 100644
> --- a/gdb/ser-tcp.c
> +++ b/gdb/ser-tcp.c
> @@ -39,6 +39,7 @@
>  
>  #ifdef USE_WIN32API
>  #include <winsock2.h>
> +#include <ws2tcpip.h>

Put there AC_CHECK_HEADERS for it unless someone confirms all MS-Windows hosts
really have that include file.  I have no idea.


>  #ifndef ETIMEDOUT
>  #define ETIMEDOUT WSAETIMEDOUT
>  #endif
> @@ -157,9 +158,10 @@ int
>  net_open (struct serial *scb, const char *name)
>  {
>    char *port_str, hostname[100];
> -  int n, port, tmp;
> +  int n, tmp;
>    int use_udp;
> -  struct hostent *hostent;
> +  struct addrinfo hints;
> +  struct addrinfo *result, *rp;
>    struct sockaddr_in sockaddr;
>  #ifdef USE_WIN32API
>    u_long ioarg;
> @@ -177,7 +179,7 @@ net_open (struct serial *scb, const char *name)
>    else if (strncmp (name, "tcp:", 4) == 0)
>      name = name + 4;
>  
> -  port_str = strchr (name, ':');
> +  port_str = strrchr (name, ':');
>  
>    if (!port_str)
>      error (_("net_open: No colon in host name!"));  /* Shouldn't ever
> @@ -186,124 +188,133 @@ net_open (struct serial *scb, const char *name)
>    tmp = min (port_str - name, (int) sizeof hostname - 1);
>    strncpy (hostname, name, tmp);	/* Don't want colon.  */
>    hostname[tmp] = '\000';	/* Tie off host name.  */
> -  port = atoi (port_str + 1);
> +  port_str++;
>  
>    /* Default hostname is localhost.  */
>    if (!hostname[0])
>      strcpy (hostname, "localhost");
>  
> -  hostent = gethostbyname (hostname);
> -  if (!hostent)
> +  memset (&hints, 0, sizeof (struct addrinfo));
> +  hints.ai_family = AF_UNSPEC;    /* Allow IPv4 or IPv6 */
> +  if (use_udp)
> +    hints.ai_socktype = SOCK_DGRAM;
> +  else
> +    hints.ai_socktype = SOCK_STREAM;
> +  hints.ai_flags = 0;
> +  hints.ai_protocol = 0;
> +
> +  tmp = getaddrinfo (hostname, port_str, &hints, &result);
> +  if (tmp)
>      {
>        fprintf_unfiltered (gdb_stderr, "%s: unknown host\n", hostname);

It should use gai_strerror().  I understand you wrote
# In this patch I was trying to change current behaviour as little as
# possible.
It can be a separate patch 2/2 or even in this one.


>        errno = ENOENT;
>        return -1;
>      }
>  
> -  sockaddr.sin_family = PF_INET;
> -  sockaddr.sin_port = htons (port);
> -  memcpy (&sockaddr.sin_addr.s_addr, hostent->h_addr,
> -	  sizeof (struct in_addr));
> -
> - retry:
> -
> -  if (use_udp)
> -    scb->fd = gdb_socket_cloexec (PF_INET, SOCK_DGRAM, 0);
> -  else
> -    scb->fd = gdb_socket_cloexec (PF_INET, SOCK_STREAM, 0);
> +  for (rp = result; ; rp = rp->ai_next ? rp->ai_next : result)
> +    {
> +      scb->fd = gdb_socket_cloexec (rp->ai_family, rp->ai_socktype,
> +                                    rp->ai_protocol);
>  
> -  if (scb->fd == -1)
> -    return -1;
> +      if (scb->fd == -1)
> +        continue;

Neither 'return -1' nor 'continue' are right here.  It should not lock-up if
none of the sockets can be created, it should return so that the error gets
reported.



> -  /* Set socket nonblocking.  */
> -  ioarg = 1;
> -  ioctl (scb->fd, FIONBIO, &ioarg);
> +      /* Set socket nonblocking.  */
> +      ioarg = 1;
> +      ioctl (scb->fd, FIONBIO, &ioarg);
[...]


Thanks,
Jan