Re: [PATCH 7/7] Implement vFile:setfs in gdbserver

[Pedro Alves <palves at redhat dot com>]

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