Minor cleanup of open() code. Put the original open flags, mode, etc. into
an 'opendata' structure that can be passed as an intent to lookup.
diff -u --recursive --new-file linux-2.5.69/fs/namei.c linux-2.5.69-01-open1/fs/namei.c
--- linux-2.5.69/fs/namei.c 2003-05-05 07:49:54.000000000 +0200
+++ linux-2.5.69-01-open1/fs/namei.c 2003-05-22 15:30:50.000000000 +0200
@@ -18,6 +18,7 @@
#include <linux/slab.h>
#include <linux/fs.h>
#include <linux/namei.h>
+#include <linux/open.h>
#include <linux/quotaops.h>
#include <linux/pagemap.h>
#include <linux/dnotify.h>
@@ -1204,19 +1205,18 @@
* for symlinks (where the permissions are checked later).
* SMP-safe
*/
-int open_namei(const char * pathname, int flag, int mode, struct nameidata *nd)
+int open_namei(const char * pathname, struct opendata *opendata, struct nameidata *nd)
{
- int acc_mode, error = 0;
+ int flag = opendata->flag;
+ int error = 0;
struct dentry *dentry;
struct dentry *dir;
int count = 0;
- acc_mode = ACC_MODE(flag);
-
/* Allow the LSM permission hook to distinguish append
access from general write access. */
if (flag & O_APPEND)
- acc_mode |= MAY_APPEND;
+ opendata->acc_mode |= MAY_APPEND;
/*
* The simplest case - just a plain lookup.
@@ -1258,6 +1258,7 @@
/* Negative dentry, just create the file */
if (!dentry->d_inode) {
+ int mode = opendata->mode;
if (!IS_POSIXACL(dir->d_inode))
mode &= ~current->fs->umask;
error = vfs_create(dir->d_inode, dentry, mode);
@@ -1267,7 +1268,7 @@
if (error)
goto exit;
/* Don't check for write permission, don't truncate */
- acc_mode = 0;
+ opendata->acc_mode = 0;
flag &= ~O_TRUNC;
goto ok;
}
@@ -1299,7 +1300,7 @@
if (dentry->d_inode && S_ISDIR(dentry->d_inode->i_mode))
goto exit;
ok:
- error = may_open(nd, acc_mode, flag);
+ error = may_open(nd, opendata->acc_mode, flag);
if (error)
goto exit;
return 0;
diff -u --recursive --new-file linux-2.5.69/fs/open.c linux-2.5.69-01-open1/fs/open.c
--- linux-2.5.69/fs/open.c 2003-05-21 02:23:23.000000000 +0200
+++ linux-2.5.69-01-open1/fs/open.c 2003-05-22 14:25:57.000000000 +0200
@@ -15,6 +15,7 @@
#include <linux/slab.h>
#include <linux/tty.h>
#include <linux/namei.h>
+#include <linux/open.h>
#include <linux/backing-dev.h>
#include <linux/security.h>
#include <linux/mount.h>
@@ -602,6 +603,8 @@
return error;
}
+#define ACC_MODE(x) ("\000\004\002\006"[(x)&O_ACCMODE])
+
/*
* Note that while the flag value (low two bits) for sys_open means:
* 00 - read-only
@@ -620,14 +623,19 @@
{
int namei_flags, error;
struct nameidata nd;
+ struct opendata opendata = {
+ .flag = flags,
+ .mode = mode,
+ };
namei_flags = flags;
if ((namei_flags+1) & O_ACCMODE)
namei_flags++;
if (namei_flags & O_TRUNC)
namei_flags |= 2;
+ opendata.acc_mode = ACC_MODE(namei_flags);
- error = open_namei(filename, namei_flags, mode, &nd);
+ error = open_namei(filename, &opendata, &nd);
if (!error)
return dentry_open(nd.dentry, nd.mnt, flags);
diff -u --recursive --new-file linux-2.5.69/include/linux/fs.h linux-2.5.69-01-open1/include/linux/fs.h
--- linux-2.5.69/include/linux/fs.h 2003-05-17 23:09:32.000000000 +0200
+++ linux-2.5.69-01-open1/include/linux/fs.h 2003-05-22 14:25:57.000000000 +0200
@@ -23,6 +23,7 @@
struct iovec;
struct nameidata;
+struct opendata;
struct pipe_inode_info;
struct poll_table_struct;
struct statfs;
@@ -1135,7 +1136,7 @@
}
extern int do_pipe(int *);
-extern int open_namei(const char *, int, int, struct nameidata *);
+extern int open_namei(const char *, struct opendata *, struct nameidata *);
extern int may_open(struct nameidata *, int, int);
extern int kernel_read(struct file *, unsigned long, char *, unsigned long);
diff -u --recursive --new-file linux-2.5.69/include/linux/open.h linux-2.5.69-01-open1/include/linux/open.h
--- linux-2.5.69/include/linux/open.h 1970-01-01 01:00:00.000000000 +0100
+++ linux-2.5.69-01-open1/include/linux/open.h 2003-05-22 14:25:57.000000000 +0200
@@ -0,0 +1,17 @@
+#ifndef _LINUX_OPEN_H
+#define _LINUX_OPEN_H
+
+struct opendata {
+ int flag;
+ int mode;
+ int acc_mode;
+
+#if 0
+ /* Private data to be added to the filp->private_data field */
+ void *private;
+ /* Callback for destroying private data in case of an error */
+ void (*destroy)(struct opendata *, void *);
+#endif
+};
+
+#endif
On Fri, 23 May 2003, Trond Myklebust wrote:
>
> Minor cleanup of open() code. Put the original open flags, mode, etc. into
> an 'opendata' structure that can be passed as an intent to lookup.
I don't mind the concepts, but I _really_ dislike the implementation.
For one thing, if you're creating a structure to pass in the flags for
open, then you should take the time to make the code _more_ readable
rather than less. In particular, the notion of having a structure like
this:
struct opendata {
int flag;
int mode;
int acc_mode;
};
where each of "flag" and "acc_mode" are magic bitfields just fills me with
horror.
So why not make those internal modes that we translate the "flags" into be
a real bitmap? That should make the code a lot more readable.
Also, I don't really understand why you want to have "opendata" and
"intent" as different structures. That's _especially_ true now that the
only intent is the "open" intent, but even if there were other intents,
I'd rather have something like this
struct lookup_info {
enum type; /* open, validate, whatever.. */
union {
struct open_intent open;
..
} data;
}
and gace tge flags (create/exclusive etc) inside that lookup_intent
instead of having multiple different pointers and transferring data from
one to the other at different phases of the "open".
Also, in patch 3/4, you do
xxx_create(struct inode *dir, struct dentry *dentry, int mode, struct vfsintent *intent)
and that "mode" this I again find offensive: why is it not in the intent?
It automatically _would_ be, if you only had one structure and one
pointer, but you lost it when you did the "opendata->intent"
transformation.
So please don't have this artifical (and clearly broken) differentiation
between "intent" and "opendata". They should be one and the same thing:
"lookup_info". Because that is what they _are_. They are not intents. They
are literally extra information for the lookup.
Linus
On Fri, May 23, 2003 at 09:23:33AM -0700, Linus Torvalds wrote:
>
> On Fri, 23 May 2003, Trond Myklebust wrote:
> >
> > Minor cleanup of open() code. Put the original open flags, mode, etc. into
> > an 'opendata' structure that can be passed as an intent to lookup.
>
> I don't mind the concepts, but I _really_ dislike the implementation.
>
> For one thing, if you're creating a structure to pass in the flags for
> open, then you should take the time to make the code _more_ readable
> rather than less. In particular, the notion of having a structure like
> this:
>
> struct opendata {
> int flag;
> int mode;
> int acc_mode;
> };
>
> where each of "flag" and "acc_mode" are magic bitfields just fills me with
> horror.
>
> So why not make those internal modes that we translate the "flags" into be
> a real bitmap? That should make the code a lot more readable.
>
> Also, I don't really understand why you want to have "opendata" and
> "intent" as different structures. That's _especially_ true now that the
> only intent is the "open" intent, but even if there were other intents,
> I'd rather have something like this
>
> struct lookup_info {
> enum type; /* open, validate, whatever.. */
> union {
> struct open_intent open;
> ..
> } data;
> }
>
> and gace tge flags (create/exclusive etc) inside that lookup_intent
> instead of having multiple different pointers and transferring data from
> one to the other at different phases of the "open".
Linus, that was one of the reasons why struct nameidata had been introduced
in the first place. _And_ discussed with Peter, BTW, so I've no idea
where the hell does lookup_info come from.
Peter, Trond: please fold that stuff into struct nameidata (note that
flags are already there) and pass the pointer to it into methods.
That would have an extra benefit (also discussed before) of allowing to
bring credentials into the game - we could store them in the same place.
If we are up to changing method prototypes - let's do it properly.
On May 23, 2003 18:59 +0100, [email protected] wrote:
> On Fri, May 23, 2003 at 09:23:33AM -0700, Linus Torvalds wrote:
> >
> > On Fri, 23 May 2003, Trond Myklebust wrote:
> > > Minor cleanup of open() code. Put the original open flags, mode, etc. into
> > > an 'opendata' structure that can be passed as an intent to lookup.
> >
> > I don't mind the concepts, but I _really_ dislike the implementation.
> >
> > For one thing, if you're creating a structure to pass in the flags for
> > open, then you should take the time to make the code _more_ readable
> > rather than less. In particular, the notion of having a structure like
> > this:
> >
> > struct opendata {
> > int flag;
> > int mode;
> > int acc_mode;
> > };
> >
> > where each of "flag" and "acc_mode" are magic bitfields just fills me with
> > horror.
> >
> > So why not make those internal modes that we translate the "flags" into be
> > a real bitmap? That should make the code a lot more readable.
> >
> > Also, I don't really understand why you want to have "opendata" and
> > "intent" as different structures. That's _especially_ true now that the
> > only intent is the "open" intent, but even if there were other intents,
> > I'd rather have something like this
> >
> > struct lookup_info {
> > enum type; /* open, validate, whatever.. */
> > union {
> > struct open_intent open;
> > ..
> > } data;
> > }
> >
> > and gace tge flags (create/exclusive etc) inside that lookup_intent
> > instead of having multiple different pointers and transferring data from
> > one to the other at different phases of the "open".
>
>
> Linus, that was one of the reasons why struct nameidata had been introduced
> in the first place. _And_ discussed with Peter, BTW, so I've no idea
> where the hell does lookup_info come from.
Yes, that is where Lustre puts the intent data already in 2.5. The code
that Trond submitted is AFAICS totally different than what we have been
using for Lustre, but I don't know if Trond and Peter have been discussing
this in private or not. I've CC'd Peter on this email, although I imagine
he is already getting it by way of fsdevel - he's just in a different
timezone right now.
> Peter, Trond: please fold that stuff into struct nameidata (note that
> flags are already there) and pass the pointer to it into methods.
> That would have an extra benefit (also discussed before) of allowing to
> bring credentials into the game - we could store them in the same place.
>
> If we are up to changing method prototypes - let's do it properly.
Cheers, Andreas
--
Andreas Dilger
http://sourceforge.net/projects/ext2resize/
http://www-mddsp.enel.ucalgary.ca/People/adilger/