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

[Pedro Alves <palves@redhat.com>]

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