[PATCH] Implement IPv6 support for GDB/gdbserver
Pedro Alves
palves@redhat.com
Wed Jun 6 12:26:00 GMT 2018
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
More information about the Gdb-patches
mailing list