Re: [patch 0/6] sys_indirect RFC - sys_indirect introduction

From: Davide Libenzi
Date: Sat Jun 30 2007 - 15:23:37 EST


On Fri, 29 Jun 2007, Ulrich Drepper wrote:

> On 6/29/07, Davide Libenzi <davidel@xxxxxxxxxxxxxxx> wrote:
> > [include/linux/indirect.h]
> > #define SYSIND_CTX_OPENFLAGS 0
> > struct sysind_ctx_OPENFLAGS {
> > __u32 ctx;
> > __u32 flags;
>
> I agree that this interface is more than any other in danger of
> needing an interface change. But I think your solution is a bit too
> expensive and complex. You need two reads from userlevel.

But, a __get_user(), once you scrap off all the gcc wrapping, is bacially
a move. That could even be removed, but really I don't see the reason
since it allows for a cleaner strcture definition in userland.




> The standard way to handle this is a versioned struct. I.e., define a
> struct for the current needs, define an initial version. To use the
> syscall pass the version number and the struct pointer to the syscall.
> If the kernel doesn't know the version number it fails. Otherwise it
> might have to read old versions of the struct which is trivial to do.
> E.g.:
>
> #define SYSIND_VERSION 1
> #define SYSIND_CTX_OPENFLAGS 0
> #define SYSIND_CTX_SIGMASK 1
> struct sysind_ctx {
> int ctx;
> union {
> int flags;
> kernel_sigset_t sset;
> };
> };

But this does not allow more than one context to be set at a time, like
the current implementation does. Ie, the current implementation allow you
to:

#define SYSIND_CTX_OPENFLAGS 0
struct sysind_ctx_OPENFLAGS {
__u32 ctx;
__u32 flags;
};

#define SYSIND_CTX_SIGSET 1
struct sysind_ctx_SIGSET {
__u32 ctx;
__compat_sigset set;
};

struct sysind_ctx_OPENFLAGS octx;
struct sysind_ctx_SIGSET sctx;
struct indirect_ctx *ctxs[2];
unsigned long params[6];

octx.ctx = SYSIND_CTX_OPENFLAGS;
octx.flags = O_CLOEXEC;
sctx.ctx = SYSIND_CTX_SIGSET;
sctx.set = SIG_XYZ | SIG_ABC;
ctxs[0] = (struct indirect_ctx *) &octx;
ctxs[1] = (struct indirect_ctx *) &sctx;
params[0] = blah;
params[1] = blew;
...
res = indirect(__NR_xxxx, ctxs, 2, params);


So you basically keep the context strcture separated, and allow to pass
down more then one context to be set.
Another solution would be to have:

struct sysind_ctx_OPENFLAGS {
__u32 flags;
};
struct sysind_ctx_SIGSET {
__compat_sigset set;
};
struct monster_struct {
__u32 size;
__u64 flags;
struct sysind_ctx_OPENFLAGS octx;
struct sysind_ctx_SIGSET sctx;
...
};

Where size gives you the size of the monster structure, and every bit on
flags tells you which strcture inside monster is valid.
But I'd clearly prefer the former.



- Davide


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/