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

[Davide Libenzi <davidel@xmailserver.org>]

On Fri, 29 Jun 2007, Ulrich Drepper wrote:

> On 6/29/07, Davide Libenzi <davidel@xmailserver.org> 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