This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 7/7] Implement vFile:setfs in gdbserver
- From: Pedro Alves <palves at redhat dot com>
- To: Gary Benson <gbenson at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Fri, 17 Apr 2015 18:09:31 +0100
- Subject: Re: [PATCH 7/7] Implement vFile:setfs in gdbserver
- Authentication-results: sourceware.org; auth=none
- References: <1429186791-6867-1-git-send-email-gbenson at redhat dot com> <1429186791-6867-8-git-send-email-gbenson at redhat dot com> <553126FA dot 8030402 at redhat dot com> <20150417160524 dot GC14618 at blade dot nx> <20150417162944 dot GA16766 at blade dot nx>
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