[7/7] Implement vFile:setfs in gdbserver

  gdb/gdbserver/ChangeLog:

	* target.h (struct target_ops) <call_with_fs_of>: New field.
	* linux-low.c (nat/linux-namespaces.h): New include.
	(linux_low_call_with_fs_of): New function.
	* linux-low.c (linux_target_ops): Initialize call_with_fs_of.
	* hostio.c (hostio_fs_pid): New static variable.
	(handle_setfs): New function.
	(enter_filesystem): Likewise.
	(open_closure): New structure.
	(handle_open_1): New function.
	(handle_open): Update to use enter_filesystem.
	(unlink_closure): New structure.
	(handle_unlink_1): New function.
	(handle_unlink): Update to use enter_filesystem.
	(readlink_closure): New structure.
	(handle_readlink_1): New function.
	(handle_readlink): Update to use enter_filesystem.
	(handle_vFile): Handle vFile:setfs packets.
---
 gdb/gdbserver/ChangeLog   |   20 ++++++
 gdb/gdbserver/hostio.c    |  152 ++++++++++++++++++++++++++++++++++++++++----
 gdb/gdbserver/linux-low.c |   10 +++
 gdb/gdbserver/target.h    |    6 ++
 4 files changed, 174 insertions(+), 14 deletions(-)
  

On 04/16/2015 01:19 PM, Gary Benson wrote:
> +/* Handle a "vFile:setfs:" packet.  */
> +
> +static void
> +handle_setfs (char *own_buf)
> +{
> +  char *p;
> +  int pid;
> +
> +  p = own_buf + strlen ("vFile:setfs:");
> +
> +  if (require_int (&p, &pid)
> +      || pid < 0
> +      || require_end (p))
> +    {
> +      hostio_packet_error (own_buf);
> +      return;
> +    }
> +
> +  hostio_fs_pid = pid;
> +
> +  hostio_reply (own_buf, 0);
> +}

This should probably return empty if
the_target->call_with_fs_of is NULL, to disable the packet
in GDB?

>  int
>  handle_vFile (char *own_buf, int packet_len, int *new_packet_len)
>  {
> -  if (startswith (own_buf, "vFile:open:"))
> +  if (the_target->call_with_fs_of != NULL
> +      && startswith (own_buf, "vFile:setfs:"))
> +    handle_setfs (own_buf);
> +  else if (startswith (own_buf, "vFile:open:"))

Ah, I see now that it's handled here.  I'd prefer moving that check
inside the handle_setfs function instead so that the "setfs"
specifics handling is all localized.

> +struct open_closure
> +{
> +  char filename[HOSTIO_PATH_MAX];
> +  int flags;
> +  int mode;

mode_t mode.

(please check for the the same issue on the native side.)

> +  int return_value;
> +};

> +/* Implementation of target_ops->call_with_fs_of.  */
> +
> +static int
> +linux_low_call_with_fs_of (int pid, void (*func) (void *), void *arg)
> +{
> +  return linux_ns_enter (pid, LINUX_NS_MNT, func, arg);
> +}
> +

So back on linux_ns_enter's naming ---

I find the abstraction here a bit odd.  It seems to be me that
a function that does:

 #1 - select filesystem/namespace
 #2 - call foo
 #3 - restore filesystem/namespace

should be a function in common code, and that the only
target-specific part is selecting a filesystem/namespace.
That is, simplifying, something like:

/* Cause the filesystem to appear as it does to process PID and
   call FUNC with argument ARG, restoring the filesystem to its
   original state afterwards.  Return nonzero if FUNC was called,
   zero otherwise (and set ERRNO). */

int
call_with_fs_of (int pid, void (*func) (void *), void *arg)
{
  int ret;

  target->select_fs (pid);
  ret = func ();
  target->select_fs (0);
  return ret;
}

Was there a reason this wasn't done that way?

(maybe make target->select_fs() return the previous
namespace, instead of passing 0 on the second
call.)

Thanks,
Pedro Alves
  

Pedro Alves wrote:
> On 04/16/2015 01:19 PM, Gary Benson wrote:
> > +/* Handle a "vFile:setfs:" packet.  */
> > +
> > +static void
> > +handle_setfs (char *own_buf)
> > +{
[snip]
> > +}
> 
> This should probably return empty if the_target->call_with_fs_of is
> NULL, to disable the packet in GDB?
> 
> >  int
> >  handle_vFile (char *own_buf, int packet_len, int *new_packet_len)
> >  {
> > -  if (startswith (own_buf, "vFile:open:"))
> > +  if (the_target->call_with_fs_of != NULL
> > +      && startswith (own_buf, "vFile:setfs:"))
> > +    handle_setfs (own_buf);
> > +  else if (startswith (own_buf, "vFile:open:"))
> 
> Ah, I see now that it's handled here.  I'd prefer moving that
> check inside the handle_setfs function instead so that the "setfs"
> specifics handling is all localized.

Ok.

> > +struct open_closure
> > +{
> > +  char filename[HOSTIO_PATH_MAX];
> > +  int flags;
> > +  int mode;
> 
> mode_t mode.
> 
> (please check for the the same issue on the native side.)

It's there in both places.  Is mode_t ok to use in gdbserver?
I don't see it anywhere...

> > +  int return_value;
> > +};
> 
> > +/* Implementation of target_ops->call_with_fs_of.  */
> > +
> > +static int
> > +linux_low_call_with_fs_of (int pid, void (*func) (void *), void *arg)
> > +{
> > +  return linux_ns_enter (pid, LINUX_NS_MNT, func, arg);
> > +}
> > +
> 
> So back on linux_ns_enter's naming ---
> 
> I find the abstraction here a bit odd.  It seems to be me that
> a function that does:
> 
>  #1 - select filesystem/namespace
>  #2 - call foo
>  #3 - restore filesystem/namespace
> 
> should be a function in common code, and that the only
> target-specific part is selecting a filesystem/namespace.
> That is, simplifying, something like:
> 
> /* Cause the filesystem to appear as it does to process PID and
>    call FUNC with argument ARG, restoring the filesystem to its
>    original state afterwards.  Return nonzero if FUNC was called,
>    zero otherwise (and set ERRNO). */
> 
> int
> call_with_fs_of (int pid, void (*func) (void *), void *arg)
> {
>   int ret;
> 
>   target->select_fs (pid);
>   ret = func ();
>   target->select_fs (0);
>   return ret;
> }
> 
> Was there a reason this wasn't done that way?
> 
> (maybe make target->select_fs() return the previous
> namespace, instead of passing 0 on the second
> call.)

I'll have a go at doing it that way.

Thanks,
Gary
  

Gary Benson wrote:
> Pedro Alves wrote:
> > So back on linux_ns_enter's naming ---
> > 
> > I find the abstraction here a bit odd.  It seems to be me that
> > a function that does:
> > 
> >  #1 - select filesystem/namespace
> >  #2 - call foo
> >  #3 - restore filesystem/namespace
> > 
> > should be a function in common code, and that the only
> > target-specific part is selecting a filesystem/namespace.
> > That is, simplifying, something like:
> > 
> > /* Cause the filesystem to appear as it does to process PID and
> >    call FUNC with argument ARG, restoring the filesystem to its
> >    original state afterwards.  Return nonzero if FUNC was called,
> >    zero otherwise (and set ERRNO). */
> > 
> > int
> > call_with_fs_of (int pid, void (*func) (void *), void *arg)
> > {
> >   int ret;
> > 
> >   target->select_fs (pid);
> >   ret = func ();
> >   target->select_fs (0);
> >   return ret;
> > }
> > 
> > Was there a reason this wasn't done that way?
> > 
> > (maybe make target->select_fs() return the previous
> > namespace, instead of passing 0 on the second
> > call.)
> 
> I'll have a go at doing it that way.

It is kind of uglier doing it that way.  It would need to look
something more like this:

  struct cleanup **old_chain;

  if (target->select_fs (pid, &old_chain) != 0)
     return failure_return_value;
  ret = func ();
  do_cleanups (old_chain);
  return ret;

linux_ns_enter has to open file descriptors for its own namespace
and the one it wants to enter.  Its own namespace file descriptor
is what it needs to return, and it has to be held open: you can't
flip namespace and then open your own descriptor to flip back.
So the thing target->select_fs would have to return would be an
open file descriptor (to then pass to target->restore_fs, which
would take a file descriptor argument rather than an PID) but
that would be putting a Linuxism into the target vector.

Cheers,
Gary
  

On 04/17/2015 05:29 PM, Gary Benson wrote:
> Gary Benson wrote:
>> Pedro Alves wrote:
>>> So back on linux_ns_enter's naming ---
>>>
>>> I find the abstraction here a bit odd.  It seems to be me that
>>> a function that does:
>>>
>>>  #1 - select filesystem/namespace
>>>  #2 - call foo
>>>  #3 - restore filesystem/namespace
>>>
>>> should be a function in common code, and that the only
>>> target-specific part is selecting a filesystem/namespace.
>>> That is, simplifying, something like:
>>>
>>> /* Cause the filesystem to appear as it does to process PID and
>>>    call FUNC with argument ARG, restoring the filesystem to its
>>>    original state afterwards.  Return nonzero if FUNC was called,
>>>    zero otherwise (and set ERRNO). */
>>>
>>> int
>>> call_with_fs_of (int pid, void (*func) (void *), void *arg)
>>> {
>>>   int ret;
>>>
>>>   target->select_fs (pid);
>>>   ret = func ();
>>>   target->select_fs (0);
>>>   return ret;
>>> }
>>>
>>> Was there a reason this wasn't done that way?
>>>
>>> (maybe make target->select_fs() return the previous
>>> namespace, instead of passing 0 on the second
>>> call.)
>>
>> I'll have a go at doing it that way.
> 
> It is kind of uglier doing it that way.  It would need to look
> something more like this:
> 
>   struct cleanup **old_chain;
> 
>   if (target->select_fs (pid, &old_chain) != 0)
>      return failure_return_value;
>   ret = func ();
>   do_cleanups (old_chain);
>   return ret;
> 
> linux_ns_enter has to open file descriptors for its own namespace
> and the one it wants to enter.  Its own namespace file descriptor
> is what it needs to return, and it has to be held open: you can't
> flip namespace and then open your own descriptor to flip back.
> So the thing target->select_fs would have to return would be an
> open file descriptor (to then pass to target->restore_fs, which
> would take a file descriptor argument rather than an PID) but
> that would be putting a Linuxism into the target vector.

Seems to me we can fix that by returning an opaque closure
instead then.  I don't think that's ugly at all.  E.g.,:

int
call_with_fs_of (int pid, void (*func) (void *), void *arg)
{
  int ret = failure_return_value;

   restore_token = target->select_fs (pid);
   if (restore_token == NULL)
     return failure_return_value;
   TRY
     {
       ret = func ();
     }
   CATCH (ex, RETURN_MASK_ALL)
     {
       target->restore_fs (restore_token);
       throw_exception (ex);
     }
   END_CATCH
   return ret;
}

The reason I think this target interface is better is that
it also allows open coding the select_fs/restore_fs calls.
Which in turn would allow getting rid of all those closures in
the other patch.  For example, like this:

/* On exception, restore the filesystem, and rethrow.  */
#define CATCH_RESTORE_FS(restore_token) \
   CATCH (ex, RETURN_MASK_ALL) \
     { \
       linux_restore_namespace (restore_token); \
       throw_exception (ex); \
     } \
   END_CATCH

static int
linux_nat_fileio_unlink (struct target_ops *ops,
		         const char *filename, int *target_errno)
{
  void *restore_token;
  int ret = -1;

  restore_token = linux_nat_enter_fs ();

  TRY
    {
      ret = super_fileio_unlink (ops, filename, target_errno);
    }
  CATCH_RESTORE_FS(restore_token)

  return ret;
}

static char *
linux_nat_fileio_readlink (struct target_ops *ops,
			   const char *filename, int *target_errno)
{
  void *restore_token;
  char *ret = NULL;

  restore_token = linux_nat_enter_fs ();

  TRY
    {
      ret = super_fileio_readlink (ops, filename, target_errno);
    }
  CATCH_RESTORE_FS(restore_token)

  return ret;
}


etc., which I find _much_ more readable than:


+/* Arguments and return value of to_fileio_unlink.  */
+
+struct unlink_closure
+{
+  struct target_ops *ops;
+  const char *filename;
+  int *target_errno;
+  int return_value;
+};
+
+/* Helper for linux_nat_fileio_unlink.  */
+
+static void
+linux_nat_fileio_unlink_1 (void *arg)
+{
+  struct unlink_closure *clo = (struct unlink_closure *) arg;
+
+  clo->return_value = super_fileio_unlink (clo->ops, clo->filename,
+					   clo->target_errno);
+}
+
+/* Implementation of to_fileio_unlink.  */
+
+static int
+linux_nat_fileio_unlink (struct target_ops *ops,
+			   const char *filename, int *target_errno)
+{
+  struct unlink_closure clo =
+    {
+      /* Arguments.  */
+      ops, filename, target_errno,
+
+      /* Failure return value.  */
+      -1
+    };
+
+  linux_nat_enter_fs (linux_nat_fileio_unlink_1, &clo, target_errno);
+
+  return clo.return_value;
+}
+
+/* Arguments and return value of to_fileio_readlink.  */
+
+struct readlink_closure
+{
+  struct target_ops *ops;
+  const char *filename;
+  int *target_errno;
+  char *return_value;
+};
+
+/* Helper for linux_nat_fileio_readlink.  */
+
+static void
+linux_nat_fileio_readlink_1 (void *arg)
+{
+  struct readlink_closure *clo = (struct readlink_closure *) arg;
+
+  clo->return_value = super_fileio_readlink (clo->ops, clo->filename,
+					     clo->target_errno);
+}
+
+/* Implementation of to_fileio_readlink.  */
+
+static char *
+linux_nat_fileio_readlink (struct target_ops *ops,
+			   const char *filename, int *target_errno)
+{
+  struct readlink_closure clo =
+    {
+      /* Arguments.  */
+      ops, filename, target_errno,
+
+      /* Failure return value.  */
+      NULL
+    };
+
+  linux_nat_enter_fs (linux_nat_fileio_readlink_1, &clo, target_errno);
+
+  return clo.return_value;
+}

Thanks,
Pedro Alves