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

[Pedro Alves <palves@redhat.com>]

On 08/31/2018 05:40 PM, John Darrington wrote:
> On Fri, Aug 31, 2018 at 05:00:55PM +0100, Pedro Alves wrote:
>      
>      What server are you using this against?  It'd be great to add
>      support for gdbserver as well.  Were you planning on doing it?
> 
> I'm using upad [1], but a version which has not yet been released (the
> released one doesn't have the necessary features).
> I wasn't planning to update gdbserver but I could do that when ...
> 
>      If we had that, then I think we could add unix domain socket testing
>      to gdb/testsuite/gdb.server/server-connect.exp (assuming it'd be easy
>      enough to create a socket from tcl).
> 
> Yes.  One of the big advantages of using local sockets in testing as
> opposed to tcp sockets is that parallel tests become a lot easier.  

That's not what I suggested.  The server-connect.exp test does this:

 # Test multiple types of connection (IPv4, IPv6, TCP, UDP) and make
 # sure both gdbserver and GDB work.

so what I meant was that we could add unix socket testing to that file
in order to smoke test unix socket work and continue working, regardless
of how the rest of the testsuite is being run.

> You don't have to worry about port number conflicts or wait times.
> 
> However to do it right is not altogether straightforward.  You'd need
> gdbserver to have some kind of daemon mode, for example
> 
> tempdir=$(mkdir -d)
> gdbserver --socket=$tempdir/mysock --start
> gdb --iex="target remote $tempdir/mysock" ...
> gdbserver --socket=$tempdir/mysock --stop
> rm -rf $tempdir
> 
> This is because there is a race condition in a server between the 
> bind and listen syscalls.  GDB must not attempt to connect until
> listen has returned successfully.

Can you clarify how unix sockets are different from tcp sockets here?

> 
> 
> [1] http://www.nongnu.org/micropad
>      
>      
>      gdbserver/tracepoint.c does:
>      
>      #ifndef UNIX_PATH_MAX
>      #define UNIX_PATH_MAX sizeof(((struct sockaddr_un *) NULL)->sun_path)
>      #endif
> 
> I'm not sure if that's entirely safe.  I believe some systems define
> sun_path as a pointer into a static buffer in the kernel.  

How can userspace have a pointer into kernel memory?

> But if some higher authority can say otherwise I'll defer to them.

What do you mean by "higher authority"?

If you google around a bit, you find references to kernels other than
Linux having a limit different than 108.  E.g., from:

 https://unix.stackexchange.com/questions/367008/why-is-socket-path-length-limited-to-a-hundred-chars

    OpenBSD: 104 characters
    FreeBSD: 104 characters
    Mac OS X 10.9: 104 characters

So hardcoding to 108 seems worse to me.

Regardless, seems odd to have different parts of gdb use different
fallbacks.  Ideally, we'd move the fallback definition to a single
place used by both users. 

> 
>      > diff --git a/gdb/serial.c b/gdb/serial.c
>      > index fb2b212918..13b1af3873 100644
>      > --- a/gdb/serial.c
>      > +++ b/gdb/serial.c
>      > @@ -213,7 +213,15 @@ serial_open (const char *name)
>      >    else if (strchr (name, ':'))
>      >      ops = serial_interface_lookup ("tcp");
>      >    else
>      > -    ops = serial_interface_lookup ("hardwire");
>      > +    {
>      > +      /* Check to see if name is a socket.  If it is, then treat is
>      > +         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))
>      > +	ops = serial_interface_lookup ("socket");
>      > +      else
>      > +	ops = serial_interface_lookup ("hardware");
>      
>      
>      Nit: maybe it's just me, but I find "socket" ambiguous -- is it
>      a unix domain socket, a tcp socket, a udp socket, other?
>      I'd have picked "unix" or "uds" instead, and likewise have
>      named the file ser-unix.c and functions unix_foo instead
>      of serial.  We already have ser-unix.c, but since this is
>      only for unix really, then we could add the new code in
>      that file instead?
> 
> Let's face it, the names of these files are totally anachronistic.
> ser-tcp.c has nothing to do with serial ports and serial.c does only
> tangentially.

All these files provide different implementations of serial transports
(as opposed to parallel), abstracted behind "struct serial", and to
be used with the "remote SERIAL protocol".  It's not that tangential.

We have three host-specific files named such that their name indicates
the host which they are for:

 "ser-unix.c", "ser-go32.c" and "ser-mingw.c".

Then we have host-independent files that are named wrt to the
transport they implement internally:

 "ser-event.c", "ser-pipe.c", "ser-tcp.c".

ser-event.c is a bit of an outlier, if you'd like to
pick on some case.

> It could use a big rename action ...

Sure, it could be better.  

But, is "socket" your ideal choice?

Thanks,
Pedro Alves