Re: [PATCH 8/9 v2] Implement vFile:setfs in gdbserver

[Gary Benson <gbenson@redhat.com>]

Pedro Alves wrote:
> On 04/30/2015 01:05 PM, Gary Benson wrote:
> > This commit implements the "vFile:setfs" packet in gdbserver.
> > 
> > gdb/gdbserver/ChangeLog:
> > 
> > 	* target.h (struct target_ops) <multifs_open>: New field.
> > 	<multifs_unlink>: Likewise.
> > 	<multifs_readlink>: Likewise.
> > 	* linux-low.c (nat/linux-namespaces.h): New include.
> > 	(linux_target_ops): Initialize the_target->multifs_open,
> > 	the_target->multifs_unlink and the_target->multifs_readlink.
> > 	* hostio.c (hostio_fs_pid): New static variable.
> > 	(handle_setfs): New function.
> > 	(handle_open): Use the_target->multifs_open as appropriate.
> > 	(handle_unlink): Use the_target->multifs_unlink as appropriate.
> > 	(handle_readlink): Use the_target->multifs_readlink as
> > 	appropriate.
> > 	(handle_vFile): Handle vFile:setfs packets.
> > ---
> >  gdb/gdbserver/ChangeLog   |   16 +++++++++++
> >  gdb/gdbserver/hostio.c    |   62 ++++++++++++++++++++++++++++++++++++++++++--
> >  gdb/gdbserver/linux-low.c |    4 +++
> >  gdb/gdbserver/target.h    |   21 +++++++++++++++
> >  4 files changed, 100 insertions(+), 3 deletions(-)
> > 
> > diff --git a/gdb/gdbserver/hostio.c b/gdb/gdbserver/hostio.c
> > index 9e858d9..10cff5a 100644
> > --- a/gdb/gdbserver/hostio.c
> > +++ b/gdb/gdbserver/hostio.c
> > @@ -243,6 +243,47 @@ hostio_reply_with_data (char *own_buf, char *buffer, int len,
> >    return input_index;
> >  }
> >  
> > +/* Process ID of inferior whose filesystem hostio functions
> > +   that take FILENAME arguments will use.  Zero means to use
> > +   our own filesystem.  */
> > +
> > +static int hostio_fs_pid = 0;
> 
> 
> This should be cleared if GDB reconnects.

Ok, I added a hostio_handle_new_gdb_connection function to do this
that's called just after target_handle_new_gdb_connection in server.c.

> > +/* Handle a "vFile:setfs:" packet.  */
> > +
> > +static void
> > +handle_setfs (char *own_buf)
> > +{
> > +  char *p;
> > +  int pid;
> > +
> > +  /* If the target doesn't have any of the in-filesystem-of methods
> > +     then there's no point in GDB sending "vFile:setfs:" packets.  We
> > +     reply with an empty packet (i.e. we pretend we don't understand
> > +     "vFile:setfs:") and that should stop GDB sending any more.  */
> > +  if (the_target->multifs_open == NULL
> > +      && the_target->multifs_unlink == NULL
> > +      && the_target->multifs_readlink == NULL)
> > +    {
> > +      own_buf[0] = '\0';
> > +      return;
> > +    }
> 
> For the setns/ENOSYS case, how about somehow probing whether setns
> actually works here (with a new method, or probing one of the
> existing ones with getpid()) and return empty packet, instead of
> ending up with open failing with ENOSYS later on?

Probing and then telling GDB we don't understand "vFile:setfs:" isn't
that good of an idea.  If we do that with an inferior in another mount
namespace then the end result is that gdbserver will access the wrong
filesystem.  You might get "file not found", or you might get an open
filehandle on another file (i.e. the file with the same name but in
gdbserver's filesystem).  If the inferior's filesystem is different
from gdbserver and gdbserver cannot access that filesystem then the
only correct response is to fail.

I've updated linux_mntns_access_fs to translate ENOSYS from setns
into ENOTSUP, so neither native nor gdbserver will ever get ENOSYS
from this code. From [1] ENOTSUP seems to be the perfect code for
linux_mntns_{open_cloexec,unlink,readlink} to be returning:

  A function returns this error when certain parameter values are
  valid, but the functionality they request is not available. This can
  mean that the function does not implement a particular command or
  option value or flag bit at all. ...  If the entire function is not
  available at all in the implementation, it returns ENOSYS instead.

Thanks for hassling me about this, it took me a while to understand
what you were saying.

Cheers,
Gary

--
[1] http://lkml.iu.edu/hypermail/linux/kernel/0208.0/0121.html