[1/4] GDBSERVER: Listen on a unix domain (instead of TCP) socket if requested.

  When invoking gdbserver, if the COMM parameter takes the form "unix::/path/name"
then a local (unix) domain socket will be created with that name and gdbserver
will listen for connections on that.

    gdb/
    * NEWS: Mention new feature.

    gdb/gdbserver/
    * configure.ac (AC_CHECK_HEADERS): Add sys/un.h.
    * configure: Regenerate.
    * remote-utils.c (remote_prepare): Create a local socket if requested.
     (remote_open):  Don't attempt to open a file if it's a socket.
     (handle_accept_event): Display the name of the socket on connection.

   gdb/common/
   * netstuff.c (parse_connection_spec)[prefixes]: New member for local domain sockets.
---
 gdb/NEWS                     |   4 ++
 gdb/common/netstuff.c        |   9 +++
 gdb/gdbserver/configure.ac   |   2 +-
 gdb/gdbserver/remote-utils.c | 148 ++++++++++++++++++++++++++++++-------------
 4 files changed, 117 insertions(+), 46 deletions(-)
  

> From: John Darrington <john@darrington.wattle.id.au>
> Cc: John Darrington <john@darrington.wattle.id.au>
> Date: Sat, 13 Oct 2018 19:57:58 +0200
> 
> When invoking gdbserver, if the COMM parameter takes the form "unix::/path/name"
> then a local (unix) domain socket will be created with that name and gdbserver
> will listen for connections on that.
> 
>     gdb/
>     * NEWS: Mention new feature.
> 
>     gdb/gdbserver/
>     * configure.ac (AC_CHECK_HEADERS): Add sys/un.h.
>     * configure: Regenerate.
>     * remote-utils.c (remote_prepare): Create a local socket if requested.
>      (remote_open):  Don't attempt to open a file if it's a socket.
>      (handle_accept_event): Display the name of the socket on connection.
> 
>    gdb/common/
>    * netstuff.c (parse_connection_spec)[prefixes]: New member for local domain sockets.

OK for the NEWS part.

Thanks.
  

On Saturday, October 13 2018, John Darrington wrote:

> When invoking gdbserver, if the COMM parameter takes the form "unix::/path/name"
> then a local (unix) domain socket will be created with that name and gdbserver
> will listen for connections on that.

Thanks for the patch.

>     gdb/
>     * NEWS: Mention new feature.
>
>     gdb/gdbserver/
>     * configure.ac (AC_CHECK_HEADERS): Add sys/un.h.
>     * configure: Regenerate.
>     * remote-utils.c (remote_prepare): Create a local socket if requested.
>      (remote_open):  Don't attempt to open a file if it's a socket.
>      (handle_accept_event): Display the name of the socket on connection.
>
>    gdb/common/
>    * netstuff.c (parse_connection_spec)[prefixes]: New member for local domain sockets.
> ---
>  gdb/NEWS                     |   4 ++
>  gdb/common/netstuff.c        |   9 +++
>  gdb/gdbserver/configure.ac   |   2 +-
>  gdb/gdbserver/remote-utils.c | 148 ++++++++++++++++++++++++++++++-------------
>  4 files changed, 117 insertions(+), 46 deletions(-)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 126e61e282..23de349324 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -15,6 +15,10 @@
>    can be passed using the '[ADDRESS]:PORT' notation, or the regular
>    'ADDRESS:PORT' method.
>  
> +* GDB and GDBserver now support local domain socket connections.  The
> +  name of a local domain socket may be provided instead of the
> +  [ADDRESS]:PORT notation.

Is it worth explicitly mentioning "UNIX domain socket"?

> +
>  * DWARF index cache: GDB can now automatically save indices of DWARF
>    symbols on disk to speed up further loading of the same binaries.
>  
> diff --git a/gdb/common/netstuff.c b/gdb/common/netstuff.c
> index c1c401ccb2..8da6aba24e 100644
> --- a/gdb/common/netstuff.c
> +++ b/gdb/common/netstuff.c
> @@ -57,6 +57,8 @@ parse_connection_spec_without_prefix (std::string spec, struct addrinfo *hint)
>  			  || std::count (spec.begin (),
>  					 spec.end (), ':') > 1)));
>  
> +  bool is_unix = hint->ai_family == AF_UNIX;

No need for a newline between the declarations of is_ipv6 and is_unix.

Here, and everywhere else, AF_UNIX may be undefined if building GDB on a
non-UNIX environment.  I'm afraid you may have to guard this code with
"HAVE_SYS_UN_H".

> +
>    if (is_ipv6)
>      {
>        if (spec[0] == '[')
> @@ -109,6 +111,12 @@ parse_connection_spec_without_prefix (std::string spec, struct addrinfo *hint)
>    if (ret.host_str.empty ())
>      ret.host_str = "localhost";
>  
> +  if (is_unix && ret.host_str != "localhost")
> +    error (_("The host name must be empty or 'localhost' for a unix domain socket."));

This line has more than 80 chars, so you will need to break it.

Also, IIRC the official name for the system is "UNIX" (all caps).

> +
> +  if (is_unix && ret.port_str.empty ())
> +    error (_("A path name must be specified for a unix domain socket."));

So "port_str" is actually a path name if we're dealing with a UNIX
socket?  What do you think about changing the name of the field to
"resource_str" or some such?

> +
>    return ret;
>  }
>  
> @@ -138,6 +146,7 @@ parse_connection_spec (const char *spec, struct addrinfo *hint)
>        { "tcp4:", AF_INET,   SOCK_STREAM },
>        { "udp6:", AF_INET6,  SOCK_DGRAM },
>        { "tcp6:", AF_INET6,  SOCK_STREAM },
> +      { "unix:", AF_LOCAL,  SOCK_STREAM },

The comment I made about AF_UNIX above also applies to AF_LOCAL.

>      };
>  
>    for (const host_prefix prefix : prefixes)
> diff --git a/gdb/gdbserver/configure.ac b/gdb/gdbserver/configure.ac
> index fa3ca53efd..de7e7d4103 100644
> --- a/gdb/gdbserver/configure.ac
> +++ b/gdb/gdbserver/configure.ac
> @@ -98,7 +98,7 @@ ACX_CONFIGURE_DIR(["../../libiberty"], ["build-libiberty-gdbserver"])
>  AC_CHECK_HEADERS(termios.h sys/reg.h string.h dnl
>  		 proc_service.h sys/procfs.h linux/elf.h dnl
>  		 fcntl.h signal.h sys/file.h dnl
> -		 sys/ioctl.h netinet/in.h sys/socket.h netdb.h dnl
> +		 sys/ioctl.h netinet/in.h sys/socket.h sys/un.h netdb.h dnl
>  		 netinet/tcp.h arpa/inet.h)
>  AC_FUNC_FORK
>  AC_CHECK_FUNCS(getauxval pread pwrite pread64 setns)

The change for "gdb/gdbserver/configure" hasn't been included in the
patch.  Perhaps you forgot to regenerate it?

> diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
> index 9199a9c7ad..0eb53c2116 100644
> --- a/gdb/gdbserver/remote-utils.c
> +++ b/gdb/gdbserver/remote-utils.c
> @@ -38,6 +38,9 @@
>  #if HAVE_NETINET_IN_H
>  #include <netinet/in.h>
>  #endif
> +#if HAVE_SYS_UN_H
> +#include <sys/un.h>
> +#endif
>  #if HAVE_SYS_SOCKET_H
>  #include <sys/socket.h>
>  #endif
> @@ -193,20 +196,33 @@ handle_accept_event (int err, gdb_client_data client_data)
>       descriptor open for add_file_handler to wait for a new connection.  */
>    delete_file_handler (listen_desc);
>  
> -  /* Convert IP address to string.  */
> -  char orig_host[GDB_NI_MAX_ADDR], orig_port[GDB_NI_MAX_PORT];
> -
> -  int r = getnameinfo ((struct sockaddr *) &sockaddr, len,
> -		       orig_host, sizeof (orig_host),
> -		       orig_port, sizeof (orig_port),
> -		       NI_NUMERICHOST | NI_NUMERICSERV);
> +  if (sockaddr.ss_family == AF_UNIX)
> +    {
> +      struct sockaddr_un su;
> +      socklen_t len;

Missing newline here.

According to getsockname(2), 'len' should contain the initial size of
the 'struct sockaddr' you're passing in the second argument.  Therefore,
you should declare 'len' as 'sizeof (su)'.

Also, there's another 'len' declare in the scope above.  In order to
avoid -Wshadow warnings, I'd recommend you to choose another variable
name here.

Last, but not least, this code also needs to be guarded with #ifdef's.

> +      if (getsockname (listen_desc, (struct sockaddr *) &su, &len) != 0)
> +	perror (_("Could not obtain remote address"));
>  
> -  if (r != 0)
> -    fprintf (stderr, _("Could not obtain remote address: %s\n"),
> -	     gai_strerror (r));
> +      fprintf (stderr, _("Remote debugging on local socket bound to %s\n"),
> +	       su.sun_path);
> +    }
>    else
> -    fprintf (stderr, _("Remote debugging from host %s, port %s\n"),
> -	     orig_host, orig_port);
> +    {
> +      /* Convert IP address to string.  */
> +      char orig_host[GDB_NI_MAX_ADDR], orig_port[GDB_NI_MAX_PORT];
> +
> +      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);
> +    }
>  
>    enable_async_notification (remote_desc);
>  
> @@ -250,6 +266,9 @@ remote_prepare (const char *name)
>    struct addrinfo hint;
>    struct addrinfo *ainfo;
>  
> +  struct sockaddr *addr;
> +  socklen_t addrlen;
> +
>    memset (&hint, 0, sizeof (hint));
>    /* Assume no prefix will be passed, therefore we should use
>       AF_UNSPEC.  */
> @@ -258,7 +277,7 @@ remote_prepare (const char *name)
>    hint.ai_protocol = IPPROTO_TCP;
>  
>    parsed_connection_spec parsed
> -    = parse_connection_spec_without_prefix (name, &hint);
> +    = parse_connection_spec (name, &hint);

There's a problem here: gdbserver doesn't work with UDP connections.
Actually, this is the only reason why there's a "_without_prefix"
variant of "parse_connection_spec": because, in this particular case, we
*don't* want to consider prefixes.  Because (so far) gdbserver deals
only with TCP sockets, and because IPv4 and IPv6 addresses are easily
distinguishable, we don't use any kind of prefix.

However, with your patch, we will probably need a prefix indeed (to
differentiate between serial tty devices and UNIX sockets, at least).
My suggestion is that you make a "parse_connection_spec_maybe_unix" or
some such, which takes into account *just* the UNIX prefix (and no
other).  After you extract the "unix:" prefix, you can pass the rest of
the string to "parse_connection_spec_without_prefix" (which would be
made "static" since it won't be used externally anymore).

>  
>    if (parsed.port_str.empty ())
>      {
> @@ -276,47 +295,86 @@ remote_prepare (const char *name)
>      }
>  #endif
>  
> -  int r = getaddrinfo (parsed.host_str.c_str (), parsed.port_str.c_str (),
> -		       &hint, &ainfo);
> +  struct sockaddr_un unix_addr;
>  
> -  if (r != 0)
> -    error (_("%s: cannot resolve name: %s"), name, gai_strerror (r));
> +#ifndef UNIX_PATH_MAX
> +#define UNIX_PATH_MAX sizeof(((struct sockaddr_un *) NULL)->sun_path)
                               ^

Missing whitespace between function and parens.

> +#endif

Even though I don't quite like using "PATH_MAX"-like because of
reproducibility issues, I don't really know what else could be done here
to prevent that.

>  
> -  scoped_free_addrinfo freeaddrinfo (ainfo);
> +  if (hint.ai_family == AF_LOCAL)
> +    {
> +      const char *sock_name = parsed.port_str.c_str ();
> +      if (parsed.port_str.length() > UNIX_PATH_MAX - 1)
                                   ^
Missing whitespace between function and parens.

> +	{
> +	  error
> +	    (_("%s is too long.  Socket names may be no longer than %s bytes."),

s/bytes/characters/ (increases readability, IMHO)?

> +	     sock_name, pulongest (UNIX_PATH_MAX - 1));
> +	  return;
> +	}
> +      listen_desc = socket (AF_UNIX,  SOCK_STREAM,  0);
> +      if (listen_desc < 0)
> +	perror_with_name ("Can't open socket");
>  
> -  struct addrinfo *iter;
> +      memset (&unix_addr, 0, sizeof (unix_addr));
> +      unix_addr.sun_family = AF_UNIX;
> +      strncpy (unix_addr.sun_path, sock_name, UNIX_PATH_MAX - 1);
>  
> -  for (iter = ainfo; iter != NULL; iter = iter->ai_next)
> -    {
> -      listen_desc = gdb_socket_cloexec (iter->ai_family, iter->ai_socktype,
> -					iter->ai_protocol);
> +      struct stat statbuf;
> +      int stat_result = stat (sock_name, &statbuf);

Missing newline here.  No need for the "stat_result" variable; you can
just check its return value in the "if" below.

> +      if (stat_result == 0
> +	  && (S_IFSOCK & statbuf.st_mode))

Don't think you need to break the line.

You should use S_ISSOCK (statbuf.st_mode).

> +	unlink (sock_name);
>  
> -      if (listen_desc >= 0)
> -	break;
> +      addr = (struct sockaddr *) &unix_addr;
> +      addrlen = sizeof (unix_addr);
>      }
> +  else
> +    {
> +      struct addrinfo *iter;
>  
> -  if (iter == NULL)
> -    perror_with_name ("Can't open socket");
> +      int r = getaddrinfo (parsed.host_str.c_str (),
> +			   parsed.port_str.c_str (),
> +			   &hint, &ainfo);
>  
> -  /* Allow rapid reuse of this port. */
> -  tmp = 1;
> -  setsockopt (listen_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp,
> -	      sizeof (tmp));
> +      if (r != 0)
> +	error (_("%s: cannot resolve name: %s"), name, gai_strerror (r));
>  
> -  switch (iter->ai_family)
> -    {
> -    case AF_INET:
> -      ((struct sockaddr_in *) iter->ai_addr)->sin_addr.s_addr = INADDR_ANY;
> -      break;
> -    case AF_INET6:
> -      ((struct sockaddr_in6 *) iter->ai_addr)->sin6_addr = in6addr_any;
> -      break;
> -    default:
> -      internal_error (__FILE__, __LINE__,
> -		      _("Invalid 'ai_family' %d\n"), iter->ai_family);
> +      scoped_free_addrinfo freeaddrinfo (ainfo);
> +
> +      for (iter = ainfo; iter != NULL; iter = iter->ai_next)
> +	{
> +	  listen_desc = gdb_socket_cloexec (iter->ai_family, iter->ai_socktype,
> +					    iter->ai_protocol);
> +
> +	  if (listen_desc >= 0)
> +	    break;
> +	}
> +
> +      if (iter == NULL)
> +	perror_with_name ("Can't open socket");
> +
> +      /* Allow rapid reuse of this port. */
> +      tmp = 1;
> +      setsockopt (listen_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp,
> +		  sizeof (tmp));
> +
> +      switch (iter->ai_family)
> +	{
> +	case AF_INET:
> +	  ((struct sockaddr_in *) iter->ai_addr)->sin_addr.s_addr = INADDR_ANY;
> +	  break;
> +	case AF_INET6:
> +	  ((struct sockaddr_in6 *) iter->ai_addr)->sin6_addr = in6addr_any;
> +	  break;
> +	default:
> +	  internal_error (__FILE__, __LINE__,
> +			  _("Invalid 'ai_family' %d\n"), iter->ai_family);
> +	}
> +      addr = iter->ai_addr;
> +      addrlen = iter->ai_addrlen;
>      }
>  
> -  if (bind (listen_desc, iter->ai_addr, iter->ai_addrlen) != 0)
> +  if (bind (listen_desc, addr, addrlen) != 0)
>      perror_with_name ("Can't bind address");
>  
>    if (listen (listen_desc, 1) != 0)
> @@ -354,11 +412,11 @@ remote_open (const char *name)
>      }
>  #ifndef USE_WIN32API
>    else if (port_str == NULL)
> -    {
> +     {
>        struct stat statbuf;
>  
>        if (stat (name, &statbuf) == 0
> -	  && (S_ISCHR (statbuf.st_mode) || S_ISFIFO (statbuf.st_mode)))
> +         && (S_ISCHR (statbuf.st_mode) || S_ISFIFO (statbuf.st_mode)))

These two changes are unrelated and should go in a separate patch (they
can be pushed as obvious, BTW).

>  	remote_desc = open (name, O_RDWR);
>        else
>  	{
> -- 
> 2.11.0

Thanks,
  

On Thu, Oct 18, 2018 at 04:18:48PM -0400, Sergio Durigan Junior wrote:

     > +  bool is_unix = hint->ai_family == AF_UNIX;
     
     No need for a newline between the declarations of is_ipv6 and is_unix.
     
     Here, and everywhere else, AF_UNIX may be undefined if building GDB on a
     non-UNIX environment.  I'm afraid you may have to guard this code with
     "HAVE_SYS_UN_H".
     

This is true.  But a quick experiment showed me that there are quite a
few other places in gdb which has this problem.

I can prepare a separate patch to fix those.

J'
  

On Friday, October 19 2018, John Darrington wrote:

> On Thu, Oct 18, 2018 at 04:18:48PM -0400, Sergio Durigan Junior wrote:
>
>      > +  bool is_unix = hint->ai_family == AF_UNIX;
>      
>      No need for a newline between the declarations of is_ipv6 and is_unix.
>      
>      Here, and everywhere else, AF_UNIX may be undefined if building GDB on a
>      non-UNIX environment.  I'm afraid you may have to guard this code with
>      "HAVE_SYS_UN_H".
>      
>
> This is true.  But a quick experiment showed me that there are quite a
> few other places in gdb which has this problem.

Which places?  There are some files that are not compiled on certain
systems, so it's fine to have system-dependent code without the guards.
I don't use proprietary OSes, so the way I test here is to compile GDB
using a mingw compiler.  If it passes, then I assume things are OK.
Your patch, for example, broke the compilation (because of
AF_UNIX/AF_LOCAL).

Cheers,
  

On 2018-10-19 3:43 p.m., Sergio Durigan Junior wrote:
> On Friday, October 19 2018, John Darrington wrote:
> 
>> On Thu, Oct 18, 2018 at 04:18:48PM -0400, Sergio Durigan Junior wrote:
>>
>>      > +  bool is_unix = hint->ai_family == AF_UNIX;
>>      
>>      No need for a newline between the declarations of is_ipv6 and is_unix.
>>      
>>      Here, and everywhere else, AF_UNIX may be undefined if building GDB on a
>>      non-UNIX environment.  I'm afraid you may have to guard this code with
>>      "HAVE_SYS_UN_H".
>>      
>>
>> This is true.  But a quick experiment showed me that there are quite a
>> few other places in gdb which has this problem.
> 
> Which places?  There are some files that are not compiled on certain
> systems, so it's fine to have system-dependent code without the guards.
> I don't use proprietary OSes, so the way I test here is to compile GDB
> using a mingw compiler.  If it passes, then I assume things are OK.
> Your patch, for example, broke the compilation (because of
> AF_UNIX/AF_LOCAL).
> 
> Cheers,
> 

I just tried compiling with mingw and stumbled on this:

  CXX    common/netstuff.o
/home/simark/src/binutils-gdb/gdb/common/netstuff.c: In function ‘parsed_connection_spec parse_connection_spec(const char*, addrinfo*)’:
/home/simark/src/binutils-gdb/gdb/common/netstuff.c:148:18: error: ‘AF_LOCAL’ was not declared in this scope
       { "unix:", AF_LOCAL,  SOCK_STREAM },
                  ^~~~~~~~

What is the status on this?

Simon
  

Mea Culpa.

I missed that one.  I'll try to get a mingw environment set up and 
check in a fix.

J'


On Sun, Oct 28, 2018 at 12:20:28PM -0400, Simon Marchi wrote:
     On 2018-10-19 3:43 p.m., Sergio Durigan Junior wrote:
     > On Friday, October 19 2018, John Darrington wrote:
     > 
     >> On Thu, Oct 18, 2018 at 04:18:48PM -0400, Sergio Durigan Junior wrote:
     >>
     >>      > +  bool is_unix = hint->ai_family == AF_UNIX;
     >>      
     >>      No need for a newline between the declarations of is_ipv6 and is_unix.
     >>      
     >>      Here, and everywhere else, AF_UNIX may be undefined if building GDB on a
     >>      non-UNIX environment.  I'm afraid you may have to guard this code with
     >>      "HAVE_SYS_UN_H".
     >>      
     >>
     >> This is true.  But a quick experiment showed me that there are quite a
     >> few other places in gdb which has this problem.
     > 
     > Which places?  There are some files that are not compiled on certain
     > systems, so it's fine to have system-dependent code without the guards.
     > I don't use proprietary OSes, so the way I test here is to compile GDB
     > using a mingw compiler.  If it passes, then I assume things are OK.
     > Your patch, for example, broke the compilation (because of
     > AF_UNIX/AF_LOCAL).
     > 
     > Cheers,
     > 
     
     I just tried compiling with mingw and stumbled on this:
     
       CXX    common/netstuff.o
     /home/simark/src/binutils-gdb/gdb/common/netstuff.c: In function ???parsed_connection_spec parse_connection_spec(const char*, addrinfo*)???:
     /home/simark/src/binutils-gdb/gdb/common/netstuff.c:148:18: error: ???AF_LOCAL??? was not declared in this scope
            { "unix:", AF_LOCAL,  SOCK_STREAM },
                       ^~~~~~~~
     
     What is the status on this?
     
     Simon
  

On 2018-10-28 14:10, John Darrington wrote:
> Mea Culpa.
> 
> I missed that one.  I'll try to get a mingw environment set up and
> check in a fix.

Ok.  If you are on Linux, it shouldn't be too hard.  For example, I use 
these packages (on two different machines):

https://aur.archlinux.org/packages/mingw-w64-gcc/
https://packages.ubuntu.com/en/bionic/gcc-mingw-w64

and configure with --host=x86_64-w64-mingw32.  I think that's all.

Simon
  

On Sun, Oct 28, 2018 at 02:51:50PM -0400, Simon Marchi wrote:
     On 2018-10-28 14:10, John Darrington wrote:
     > Mea Culpa.
     > 
     > I missed that one.  I'll try to get a mingw environment set up and
     > check in a fix.
     
     Ok.  If you are on Linux, it shouldn't be too hard.  For example, I use
     these packages (on two different machines):
     
     https://aur.archlinux.org/packages/mingw-w64-gcc/
     https://packages.ubuntu.com/en/bionic/gcc-mingw-w64
     
     and configure with --host=x86_64-w64-mingw32.  I think that's all.
     
For some reason, when I try to crossbuild in this way, there are many
failures (unrelated to this issue).

However I've checked in a fix for this issue, and tested it by building
natively with a hacked set of standard include headers.

J'
  

Hi John,

> However I've checked in a fix for this issue, and tested it by building
> natively with a hacked set of standard include headers.

you always need to post patches here, if only for reference.

Besides, we're currently very inconsistent here (haven't checked which
part of that is due to your code): most places use AF_UNIX, only two use
AF_LOCAL instead (common/netstuff.c, gdbserver/remote-utils.c), and your
configure check only checks for AF_LOCAL.  I believe we should
canonicalize for one of the two and allow for systems that define only
one or the other.

	Rainer
  

Hi John,

>> However I've checked in a fix for this issue, and tested it by building
>> natively with a hacked set of standard include headers.
>
> you always need to post patches here, if only for reference.

I also noticed that your ChangeLog entry had a couple of formatting
issues.  I've fixed them with my last commit.

	Rainer
  

On 2018-10-29 5:11 a.m., Rainer Orth wrote:
> Hi John,

> 

>> However I've checked in a fix for this issue, and tested it by building

>> natively with a hacked set of standard include headers.

> 

> you always need to post patches here, if only for reference.


Not only should you post here the patches you push as obvious, but I don't
think that this:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=98a17ece013cb94cd602496b9efb92b8816b3953

falls under the obvious rule:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=blob;f=gdb/MAINTAINERS;h=d5154d394206861ef5471b4984a4762589c34fc4;hb=HEAD#l82

I can't judge whether the patch is right or not with a quick glance, but it
certainly is complex enough to warrant a discussion (as Rainer's reply below
shows).

Additionally, it seems like the initial 4-patch series was pushed without
explicit approval from a maintainer (at least I can't find any).  Next time,
please wait to have an approval before pushing.  If you are not sure whether
a reply constitutes an a approval, it's better to ask the maintainer to
clarify.

> Besides, we're currently very inconsistent here (haven't checked which

> part of that is due to your code): most places use AF_UNIX, only two use

> AF_LOCAL instead (common/netstuff.c, gdbserver/remote-utils.c), and your

> configure check only checks for AF_LOCAL.  I believe we should

> canonicalize for one of the two and allow for systems that define only

> one or the other.


mingw defines AF_UNIX, so I would tend to go towards that route.  Any of you
knows what happens at runtime when you try to bind a AF_UNIX socket on mingw/Windows?

Simon
  

On Mon, Oct 29, 2018 at 03:51:55PM +0000, Simon Marchi wrote:
     On 2018-10-29 5:11 a.m., Rainer Orth wrote:
     > Hi John,
     > 
     >> However I've checked in a fix for this issue, and tested it by building
     >> natively with a hacked set of standard include headers.
     > 
     > you always need to post patches here, if only for reference.
     
Doesn't that make the gdb-cvs list completely redundant?

     Not only should you post here the patches you push as obvious, but I don't
     think that this:
     
     https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=98a17ece013cb94cd602496b9efb92b8816b3953
     
     falls under the obvious rule:

But a number of people had complained that their build was broken, and
this was a fix for that.   I judged that in consideration of those
people fixding their problem was more important than strict observance 
of protocol.
     
     I can't judge whether the patch is right or not with a quick glance, but it
     certainly is complex enough to warrant a discussion (as Rainer's reply below
     shows).

     
     Additionally, it seems like the initial 4-patch series was pushed without
     explicit approval from a maintainer (at least I can't find any).  Next time,
     please wait to have an approval before pushing.  If you are not sure whether
     a reply constitutes an a approval, it's better to ask the maintainer to
     clarify.

All of those patches were certainly discussed.   In the past, when I've
followed up a person who has commented on a patch, but been vague about
approval, I have had either a piqued response; or no response at all.

If you think it necesary however I can revert anything you think hasn't
had enough discussion.

     
     > Besides, we're currently very inconsistent here (haven't checked which
     > part of that is due to your code): most places use AF_UNIX, only two use
     > AF_LOCAL instead (common/netstuff.c, gdbserver/remote-utils.c), and your
     > configure check only checks for AF_LOCAL.  I believe we should
     > canonicalize for one of the two and allow for systems that define only
     > one or the other.
     
     mingw defines AF_UNIX, so I would tend to go towards that route.  Any of you
     knows what happens at runtime when you try to bind a AF_UNIX socket on mingw/Windows?
     
AS I understand it, it depends upon which version of windows.
Apparently recent versions have local ssockets, but older ones do not.

See https://blogs.msdn.microsoft.com/commandline/2017/12/19/af_unix-comes-to-windows/

J'
  

On Monday, October 29 2018, John Darrington wrote:

> On Mon, Oct 29, 2018 at 03:51:55PM +0000, Simon Marchi wrote:
>      On 2018-10-29 5:11 a.m., Rainer Orth wrote:
>      > Hi John,
>      > 
>      >> However I've checked in a fix for this issue, and tested it by building
>      >> natively with a hacked set of standard include headers.
>      > 
>      > you always need to post patches here, if only for reference.
>      
> Doesn't that make the gdb-cvs list completely redundant?
>
>      Not only should you post here the patches you push as obvious, but I don't
>      think that this:
>      
>      https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=98a17ece013cb94cd602496b9efb92b8816b3953
>      
>      falls under the obvious rule:
>
> But a number of people had complained that their build was broken, and
> this was a fix for that.   I judged that in consideration of those
> people fixding their problem was more important than strict observance 
> of protocol.
>      
>      I can't judge whether the patch is right or not with a quick glance, but it
>      certainly is complex enough to warrant a discussion (as Rainer's reply below
>      shows).
>
>      
>      Additionally, it seems like the initial 4-patch series was pushed without
>      explicit approval from a maintainer (at least I can't find any).  Next time,
>      please wait to have an approval before pushing.  If you are not sure whether
>      a reply constitutes an a approval, it's better to ask the maintainer to
>      clarify.
>
> All of those patches were certainly discussed.   In the past, when I've
> followed up a person who has commented on a patch, but been vague about
> approval, I have had either a piqued response; or no response at all.

We were certainly discussing the patches, but they were not approved by
anyone.  It's also worth mentioning that I raised various points that
were not addressed (even though we discussed them).  It is still a
requirement that the patches need to be approved by at least one
maintainer before it is pushed to the repository.

> If you think it necesary however I can revert anything you think hasn't
> had enough discussion.

Yes, please.  The patch series is not ready to be pushed yet; there are
a bunch of points I raised that were not addressed (and are now causing
the failures).  I am not a global maintainer, but it is my opinion that
the patch series should be reverted for now.  We can continue discussing
and fixing things with it here in the list.

Thanks,
  

On Mon, Oct 29, 2018 at 12:42:43PM -0400, Sergio Durigan Junior wrote:

     > If you think it necesary however I can revert anything you think hasn't
     > had enough discussion.
     
     Yes, please.  The patch series is not ready to be pushed yet; there are
     a bunch of points I raised that were not addressed (and are now causing
     the failures).  I am not a global maintainer, but it is my opinion that
     the patch series should be reverted for now.  We can continue discussing
     and fixing things with it here in the list.
     
Done.

Start discussing.

J'
  

On 2018-10-29 12:42 p.m., Sergio Durigan Junior wrote:
> On Monday, October 29 2018, John Darrington wrote:

> 

>> On Mon, Oct 29, 2018 at 03:51:55PM +0000, Simon Marchi wrote:

>>      On 2018-10-29 5:11 a.m., Rainer Orth wrote:

>>      > Hi John,

>>      > 

>>      >> However I've checked in a fix for this issue, and tested it by building

>>      >> natively with a hacked set of standard include headers.

>>      > 

>>      > you always need to post patches here, if only for reference.

>>      

>> Doesn't that make the gdb-cvs list completely redundant?


Perhaps, I don't know.  Personally, I am not subscribed to gdb-cvs.  And generally,
when posting on gdb-patches, you'll mention that the patch was pushed as "obvious",
which the commit message won't necessarily contain.

It's also sometimes necessary to discuss a patch that was initially committed as
obvious.  The post on gdb-patches would be the place for that.

>>      Not only should you post here the patches you push as obvious, but I don't

>>      think that this:

>>      

>>      https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;a=commit;h=98a17ece013cb94cd602496b9efb92b8816b3953

>>      

>>      falls under the obvious rule:

>>

>> But a number of people had complained that their build was broken, and

>> this was a fix for that.   I judged that in consideration of those

>> people fixding their problem was more important than strict observance 

>> of protocol.


Let me reassure you, I completely understand that the intention was good.  I don't
want you to feel bad in any way for trying to fix things up!  Next time, post the
patch to the mailing list for review.  If the patch title contains "Fix build for...",
it is likely that it will be reviewed quite quickly.

>>      

>>      I can't judge whether the patch is right or not with a quick glance, but it

>>      certainly is complex enough to warrant a discussion (as Rainer's reply below

>>      shows).

>>

>>      

>>      Additionally, it seems like the initial 4-patch series was pushed without

>>      explicit approval from a maintainer (at least I can't find any).  Next time,

>>      please wait to have an approval before pushing.  If you are not sure whether

>>      a reply constitutes an a approval, it's better to ask the maintainer to

>>      clarify.

>>

>> All of those patches were certainly discussed.   In the past, when I've

>> followed up a person who has commented on a patch, but been vague about

>> approval, I have had either a piqued response; or no response at all.

> 

> We were certainly discussing the patches, but they were not approved by

> anyone.  It's also worth mentioning that I raised various points that

> were not addressed (even though we discussed them).  It is still a

> requirement that the patches need to be approved by at least one

> maintainer before it is pushed to the repository.


Yep.

>> If you think it necesary however I can revert anything you think hasn't

>> had enough discussion.

> 

> Yes, please.  The patch series is not ready to be pushed yet; there are

> a bunch of points I raised that were not addressed (and are now causing

> the failures).  I am not a global maintainer, but it is my opinion that

> the patch series should be reverted for now.  We can continue discussing

> and fixing things with it here in the list.


I have reverted the initial series as well as the fixup.  Sergio has mentioned on IRC
that it causes some test failures on the buildbot that cause the test time very long.
So this needs more testing before we push it again.

Simon
  

On 2018-10-29 1:34 p.m., John Darrington wrote:
> On Mon, Oct 29, 2018 at 12:42:43PM -0400, Sergio Durigan Junior wrote:

> 

>      > If you think it necesary however I can revert anything you think hasn't

>      > had enough discussion.

>      

>      Yes, please.  The patch series is not ready to be pushed yet; there are

>      a bunch of points I raised that were not addressed (and are now causing

>      the failures).  I am not a global maintainer, but it is my opinion that

>      the patch series should be reverted for now.  We can continue discussing

>      and fixing things with it here in the list.

>      

> Done.

> 

> Start discussing.


Well, there are these two loose ends.

https://sourceware.org/ml/gdb-patches/2018-10/msg00438.html
https://sourceware.org/ml/gdb-patches/2018-10/msg00465.html

Also, please check why this series made the buildbot go crazy in
the native-extended-gdbserver configuration:

  https://gdb-build.sergiodj.net/builders/Fedora-x86_64-native-extended-gdbserver-m64/builds/11314

It was suggested on IRC that the scoped_free_addrinfo in remote-utils.c is
at a wrong place, causing something to be used after free.

Simon