From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1973 invoked by alias); 6 Jun 2018 12:26:22 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 926 invoked by uid 89); 6 Jun 2018 12:26:21 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.3 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=connection, 985, alive, boards X-HELO: mx1.redhat.com Received: from mx3-rdu2.redhat.com (HELO mx1.redhat.com) (66.187.233.73) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 06 Jun 2018 12:26:18 +0000 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 05AB1818BAF1; Wed, 6 Jun 2018 12:26:17 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id 58DC720284D6; Wed, 6 Jun 2018 12:26:15 +0000 (UTC) Subject: Re: [PATCH] Implement IPv6 support for GDB/gdbserver To: Sergio Durigan Junior , GDB Patches References: <20180523185719.22832-1-sergiodj@redhat.com> Cc: Eli Zaretskii , Jan Kratochvil , Paul Fertser , Tsutomu Seki From: Pedro Alves Message-ID: <307a63d3-703d-5611-1508-c80daa86fbbf@redhat.com> Date: Wed, 06 Jun 2018 12:26:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 MIME-Version: 1.0 In-Reply-To: <20180523185719.22832-1-sergiodj@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-SW-Source: 2018-06/txt/msg00136.txt.bz2 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