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

Pedro Alves
2015-04-17T17:09:37Z

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