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

Jan Kratochvil
2014-02-12T16:53:33Z

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