Re: [PATCH] Allow remote debugging over a Unix local domain socket.

[Pedro Alves <palves@redhat.com>]

On 09/05/2018 04:26 PM, John Darrington wrote:
> Extend the "target remote" and "target extended-remote" commands
> such that if the filename provided is a Unix local domain (AF_UNIX)
> socket, then it'll be treated as such, instead of trying to open
> it as if it were a character device.
> 
> gdb/ChangeLog:
> 	* NEWS: Mention changed commands.
> 	* ser-local.c: New file.
> 	* configure.ac (SER_HARDWIRE): Add ser-local.o.
> 	* Makefile.in: Add new file.
> 	* serial.c (serial_open): Check if filename is a socket
> 	  and lookup the appropriate interface accordingly.

Add:
	* configure: Regenerate.

> 
> gdb/doc/ChangeLog:
> 	* gdb.texinfo (Remote Connection Commands):  Describe

Spurious double space.

> 	  the changes to target remote and target extended-remote
> 	  relating to Unix domain sockets.
> ---
>  gdb/Makefile.in     |   1 +
>  gdb/NEWS            |   5 +++
>  gdb/configure       |   1 +
>  gdb/configure.ac    |   1 +
>  gdb/doc/gdb.texinfo |  22 +++++++++-
>  gdb/ser-local.c     | 116 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  gdb/serial.c        |  12 +++++-
>  7 files changed, 156 insertions(+), 2 deletions(-)
>  create mode 100644 gdb/ser-local.c
> 
> diff --git a/gdb/Makefile.in b/gdb/Makefile.in
> index 118c3c8062..39bbe9be88 100644
> --- a/gdb/Makefile.in
> +++ b/gdb/Makefile.in
> @@ -2323,6 +2323,7 @@ ALLDEPFILES = \
>  	ser-go32.c \
>  	ser-mingw.c \
>  	ser-pipe.c \
> +	ser-local.c \
>  	ser-tcp.c \
>  	sh-nbsd-nat.c \
>  	sh-nbsd-tdep.c \
> diff --git a/gdb/NEWS b/gdb/NEWS
> index a7a3674375..e5926897a0 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -36,6 +36,11 @@ maint show dwarf unwinders
>  
>  * Changed commands
>  
> +target remote FILENAME
> +target extended-remote FILENAME
> +  If FILENAME is a Unix domain socket, GDB will attempt to connect
> +  to this socket instead of opening FILENAME as a character device.
> +
>  thread apply [all | COUNT | -COUNT] [FLAG]... COMMAND
>    The 'thread apply' command accepts new FLAG arguments.
>    FLAG arguments allow to control what output to produce and how to handle
> diff --git a/gdb/configure b/gdb/configure
> index 9cd0036848..95f246d013 100755
> --- a/gdb/configure
> +++ b/gdb/configure
> @@ -15586,6 +15586,7 @@ case ${host} in
>    *go32* ) SER_HARDWIRE=ser-go32.o ;;
>    *djgpp* ) SER_HARDWIRE=ser-go32.o ;;
>    *mingw32*) SER_HARDWIRE="ser-base.o ser-tcp.o ser-mingw.o" ;;
> +  *) SER_HARDWIRE="$SER_HARDWIRE ser-local.o" ;;
>  esac
>  
>  
> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 13bc5f9a8f..9a919eaa1e 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -1875,6 +1875,7 @@ case ${host} in
>    *go32* ) SER_HARDWIRE=ser-go32.o ;;
>    *djgpp* ) SER_HARDWIRE=ser-go32.o ;;
>    *mingw32*) SER_HARDWIRE="ser-base.o ser-tcp.o ser-mingw.o" ;;
> +  *) SER_HARDWIRE="$SER_HARDWIRE ser-local.o" ;;
>  esac
>  AC_SUBST(SER_HARDWIRE)
>  
> diff --git a/gdb/doc/gdb.texinfo b/gdb/doc/gdb.texinfo
> index 5068c0ac81..dae62c1787 100644
> --- a/gdb/doc/gdb.texinfo
> +++ b/gdb/doc/gdb.texinfo
> @@ -20703,7 +20703,8 @@ programs.
>  
>  @subsection Remote Connection Commands
>  @cindex remote connection commands
> -@value{GDBN} can communicate with the target over a serial line, or
> +@value{GDBN} can communicate with the target over a serial line, a
> +local Unix domain socket, or
>  over an @acronym{IP} network using @acronym{TCP} or @acronym{UDP}.  In
>  each case, @value{GDBN} uses the same protocol for debugging your
>  program; only the medium carrying the debugging packets varies.  The
> @@ -20728,6 +20729,25 @@ If you're using a serial line, you may want to give @value{GDBN} the
>  (@pxref{Remote Configuration, set serial baud}) before the
>  @code{target} command.
>  
> +
> +@item target remote @var{local-socket}
> +@itemx target extended-remote @var{local-socket}
> +@cindex local socket, @code{target remote}
> +@cindex Unix domain socket
> +Use @var{local-socket} to communicate with the target.  For example,
> +to use a local Unix domain socket bound to the file system entry @file{/tmp/gdb-socket0}:
> +
> +@smallexample
> +target remote /tmp/gdb-socket0
> +@end smallexample
> +
> +Note that this command has the same form as the command to connect
> +to a serial line.  @value{GDBN} will automatically determine which
> +kind of file you have specified and will make the appropriate kind
> +of connection.
> +This feature is not available if the host system does not support
> +Unix domain sockets.
> +
>  @item target remote @code{@var{host}:@var{port}}
>  @itemx target remote @code{@var{[host]}:@var{port}}
>  @itemx target remote @code{tcp:@var{host}:@var{port}}
> diff --git a/gdb/ser-local.c b/gdb/ser-local.c
> new file mode 100644
> index 0000000000..a3b8248ae5
> --- /dev/null
> +++ b/gdb/ser-local.c
> @@ -0,0 +1,116 @@
> +/* Serial interface for local domain connections on Un*x like systems.
> +
> +   Copyright (C) 1992-2018 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "defs.h"
> +#include "serial.h"
> +#include "ser-base.h"
> +
> +#include <sys/socket.h>
> +#include <sys/un.h>
> +
> +#ifndef UNIX_PATH_MAX
> +#define UNIX_PATH_MAX sizeof(((struct sockaddr_un *) NULL)->sun_path)
> +#endif
> +
> +/* Open a AF_UNIX socket.  */

"Open an", I suppose.  Add empty line between comment and function.

> +int
> +local_open (struct serial *scb, const char *name)

I think this can be static.

> +{
> +  struct sockaddr_un addr;
> +
> +  if (strlen(name) > UNIX_PATH_MAX - 1)

Space before open parens.

> +    {
> +      warning (_("The socket name is too long.  It may be no longer than %zu bytes."),

Too long line (over 80 cols).

> +               UNIX_PATH_MAX - 1L);

That %z potentially causes a warning on systems that _don't_ define
UNIX_PATH_MAX in terms of sizeof (and thus size_t).  Use %s with pulongest
instead.

> +      return -1;
> +    }
> +
> +  memset (&addr, 0, sizeof addr);
> +  addr.sun_family = AF_UNIX;
> +  strncpy (addr.sun_path, name, UNIX_PATH_MAX - 1);
> +
> +  int sock = socket (AF_UNIX, SOCK_STREAM, 0);
> +
> +  if (connect (sock, (struct sockaddr *) &addr,
> +	       sizeof (struct sockaddr_un)) < 0)
> +    {
> +      close (sock);
> +      scb->fd = -1;
> +      return -1;
> +    }
> +
> +  scb->fd = sock;
> +
> +  return 0;
> +}
> +
> +void
> +local_close (struct serial *scb)

Likewise, I think this can be static.


> +{
> +  if (scb->fd == -1)
> +    return;
> +
> +  close (scb->fd);
> +  scb->fd = -1;
> +}
> +
> +static int
> +local_read_prim (struct serial *scb, size_t count)
> +{
> +  return recv (scb->fd, scb->buf, count, 0);
> +}
> +
> +static int
> +local_write_prim (struct serial *scb, const void *buf, size_t count)
> +{
> +  return send (scb->fd, buf, count, 0);
> +}
> +
> +/* The local socket ops.  */
> +
> +static const struct serial_ops local_ops =
> +{
> +  "local",
> +  local_open,
> +  local_close,
> +  NULL,
> +  ser_base_readchar,
> +  ser_base_write,
> +  ser_base_flush_output,
> +  ser_base_flush_input,
> +  ser_base_send_break,
> +  ser_base_raw,
> +  ser_base_get_tty_state,
> +  ser_base_copy_tty_state,
> +  ser_base_set_tty_state,
> +  ser_base_print_tty_state,
> +  ser_base_setbaudrate,
> +  ser_base_setstopbits,
> +  ser_base_setparity,
> +  ser_base_drain_output,
> +  ser_base_async,
> +  local_read_prim,
> +  local_write_prim
> +};
> +
> +void
> +_initialize_ser_socket (void)
> +{
> +  serial_add_interface (&local_ops);
> +}
> diff --git a/gdb/serial.c b/gdb/serial.c
> index fb2b212918..c04e011012 100644
> --- a/gdb/serial.c
> +++ b/gdb/serial.c
> @@ -213,7 +213,17 @@ serial_open (const char *name)
>    else if (strchr (name, ':'))
>      ops = serial_interface_lookup ("tcp");
>    else
> -    ops = serial_interface_lookup ("hardwire");
> +    {
> +#ifndef USE_WIN32API
> +      /* Check to see if name is a socket.  If it is, then treat it
> +         as such.  Otherwise assume that it's a character device.  */
> +      struct stat sb;
> +      if (0 == stat (name, &sb) && ((sb.st_mode & S_IFMT) == S_IFSOCK))

We don't use that constant-on-left style in GDB.  GCC warns about
mistaken use of "=" anyway.  Redundant set of parens on the right
term (which we avoid).  Write:

      if (stat (name, &sb) == 0 && (sb.st_mode & S_IFMT) == S_IFSOCK)


> +	ops = serial_interface_lookup ("local");
> +      else
> +#endif
> +	ops = serial_interface_lookup ("hardwire");
> +    }

Otherwise, and other than the naming thing discussed
on the other thread, this LGTM.

Really sorry for the delay, and thanks much for the patch.
Lack of UDS support has come up once in a while on IRC
over the years.  The workaround has been to use netcat
or something like it, if you needed it.

Thanks,
Pedro Alves