shmem uses get_empty_filp() and then init_file(). Their is no good reason
not to just use alloc_file() like everything else.
Signed-off-by: Eric Paris <[email protected]>
---
mm/shmem.c | 20 ++++++++++----------
1 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/mm/shmem.c b/mm/shmem.c
index 356dd99..831f8bb 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2640,32 +2640,32 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
if (!dentry)
goto put_memory;
- error = -ENFILE;
- file = get_empty_filp();
- if (!file)
- goto put_dentry;
-
error = -ENOSPC;
inode = shmem_get_inode(root->d_sb, S_IFREG | S_IRWXUGO, 0, flags);
if (!inode)
- goto close_file;
+ goto put_dentry;
d_instantiate(dentry, inode);
inode->i_size = size;
inode->i_nlink = 0; /* It is unlinked */
- init_file(file, shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
- &shmem_file_operations);
+
+ error = -ENFILE;
+ file = alloc_file(shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
+ &shmem_file_operations);
+ if (!file)
+ goto put_dentry;
#ifndef CONFIG_MMU
error = ramfs_nommu_expand_for_mapping(inode, size);
if (error)
goto close_file;
#endif
- ima_counts_get(file);
return file;
+#ifndef CONFIG_MMU
close_file:
- put_filp(file);
+ fput(file);
+#endif
put_dentry:
dput(dentry);
put_memory:
The pipe code duplicates the functionality of alloc-file and init-file. Use
the generic vfs functions instead of duplicating code.
Signed-off-by: Eric Paris <[email protected]>
---
fs/pipe.c | 21 +++++++++------------
1 files changed, 9 insertions(+), 12 deletions(-)
diff --git a/fs/pipe.c b/fs/pipe.c
index ae17d02..5d6c969 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -1028,20 +1028,17 @@ void free_write_pipe(struct file *f)
struct file *create_read_pipe(struct file *wrf, int flags)
{
- struct file *f = get_empty_filp();
- if (!f)
- return ERR_PTR(-ENFILE);
-
- /* Grab pipe from the writer */
- f->f_path = wrf->f_path;
- path_get(&wrf->f_path);
- f->f_mapping = wrf->f_path.dentry->d_inode->i_mapping;
+ struct file *f;
+ struct dentry *dentry = wrf->f_path.dentry;
+ struct vfsmount *mnt = wrf->f_path.mnt;
- f->f_pos = 0;
+ dentry = dget(dentry);
+ f = alloc_file(mnt, dentry, FMODE_READ, &read_pipefifo_fops);
+ if (!f) {
+ dput(dentry);
+ return ERR_PTR(-ENFILE);
+ }
f->f_flags = O_RDONLY | (flags & O_NONBLOCK);
- f->f_op = &read_pipefifo_fops;
- f->f_mode = FMODE_READ;
- f->f_version = 0;
return f;
}
inotify basically duplicates everything from alloc-file and init-file. Use
the generic vfs functions instead.
Signed-off-by: Eric Paris <[email protected]>
---
fs/notify/inotify/inotify_user.c | 23 +++++++++--------------
1 files changed, 9 insertions(+), 14 deletions(-)
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index c40894a..3e03803 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -725,6 +725,7 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
struct fsnotify_group *group;
struct user_struct *user;
struct file *filp;
+ struct dentry *dentry;
int fd, ret;
/* Check the IN_* constants for consistency. */
@@ -738,12 +739,6 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
if (fd < 0)
return fd;
- filp = get_empty_filp();
- if (!filp) {
- ret = -ENFILE;
- goto out_put_fd;
- }
-
user = get_current_user();
if (unlikely(atomic_read(&user->inotify_devs) >=
inotify_max_user_instances)) {
@@ -758,11 +753,12 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
goto out_free_uid;
}
- filp->f_op = &inotify_fops;
- filp->f_path.mnt = mntget(inotify_mnt);
- filp->f_path.dentry = dget(inotify_mnt->mnt_root);
- filp->f_mapping = filp->f_path.dentry->d_inode->i_mapping;
- filp->f_mode = FMODE_READ;
+ dentry = dget(inotify_mnt->mnt_root);
+ filp = alloc_file(inotify_mnt, dentry, FMODE_READ, &inotify_fops);
+ if (!filp) {
+ ret = -ENFILE;
+ goto out_dput;
+ }
filp->f_flags = O_RDONLY | (flags & O_NONBLOCK);
filp->private_data = group;
@@ -771,11 +767,10 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
fd_install(fd, filp);
return fd;
-
+out_dput:
+ dput(dentry);
out_free_uid:
free_uid(user);
- put_filp(filp);
-out_put_fd:
put_unused_fd(fd);
return ret;
}
Currently the networking code does interesting things allocating its struct
file and file descriptors. This patch attempts to unify all of that and
simplify the error paths. It is also a part of my patch series trying to get
rid of init-file and get-empty_filp and friends.
Signed-off-by: Eric Paris <[email protected]>
---
net/socket.c | 122 +++++++++++++++++++++-------------------------------------
1 files changed, 45 insertions(+), 77 deletions(-)
diff --git a/net/socket.c b/net/socket.c
index 402abb3..41ac0b1 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -355,32 +355,24 @@ static const struct dentry_operations sockfs_dentry_operations = {
* but we take care of internal coherence yet.
*/
-static int sock_alloc_fd(struct file **filep, int flags)
+static int sock_alloc_fd(struct file **filep, struct socket *sock, int flags)
{
- int fd;
-
- fd = get_unused_fd_flags(flags);
- if (likely(fd >= 0)) {
- struct file *file = get_empty_filp();
-
- *filep = file;
- if (unlikely(!file)) {
- put_unused_fd(fd);
- return -ENFILE;
- }
- } else
- *filep = NULL;
- return fd;
-}
-
-static int sock_attach_fd(struct socket *sock, struct file *file, int flags)
-{
- struct dentry *dentry;
+ int fd, rc;
+ struct file *file;
+ struct dentry *dentry = NULL;
struct qstr name = { .name = "" };
+ fd = get_unused_fd_flags(flags & O_CLOEXEC);
+ if (unlikely(fd < 0)) {
+ rc = fd;
+ goto out_err;
+ }
+
dentry = d_alloc(sock_mnt->mnt_sb->s_root, &name);
- if (unlikely(!dentry))
- return -ENOMEM;
+ if (unlikely(!dentry)) {
+ rc = -ENOMEM;
+ goto out_err;
+ }
dentry->d_op = &sockfs_dentry_operations;
/*
@@ -391,32 +383,37 @@ static int sock_attach_fd(struct socket *sock, struct file *file, int flags)
dentry->d_flags &= ~DCACHE_UNHASHED;
d_instantiate(dentry, SOCK_INODE(sock));
+ file = alloc_file(sock_mnt, dentry, FMODE_READ | FMODE_WRITE,
+ &socket_file_ops);
+ if (unlikely(!file)) {
+ rc = -ENFILE;
+ goto out_err;
+ }
+
sock->file = file;
- init_file(file, sock_mnt, dentry, FMODE_READ | FMODE_WRITE,
- &socket_file_ops);
SOCK_INODE(sock)->i_fop = &socket_file_ops;
file->f_flags = O_RDWR | (flags & O_NONBLOCK);
- file->f_pos = 0;
file->private_data = sock;
- return 0;
+ return fd;
+out_err:
+ if (fd >= 0)
+ put_unused_fd(fd);
+ if (dentry)
+ dput(dentry);
+ *filep = NULL;
+ return rc;
}
int sock_map_fd(struct socket *sock, int flags)
{
struct file *newfile;
- int fd = sock_alloc_fd(&newfile, flags);
-
- if (likely(fd >= 0)) {
- int err = sock_attach_fd(sock, newfile, flags);
+ int fd;
- if (unlikely(err < 0)) {
- put_filp(newfile);
- put_unused_fd(fd);
- return err;
- }
+ fd = sock_alloc_fd(&newfile, sock, flags);
+ if (likely(fd >= 0))
fd_install(fd, newfile);
- }
+
return fd;
}
@@ -1384,35 +1381,22 @@ SYSCALL_DEFINE4(socketpair, int, family, int, type, int, protocol,
err = sock_create(family, type, protocol, &sock2);
if (err < 0)
- goto out_release_1;
+ goto out_release_sock_1;
err = sock1->ops->socketpair(sock1, sock2);
if (err < 0)
- goto out_release_both;
+ goto out_release_sock_2;
- fd1 = sock_alloc_fd(&newfile1, flags & O_CLOEXEC);
+ fd1 = sock_alloc_fd(&newfile1, sock1, flags & (O_CLOEXEC | O_NONBLOCK));
if (unlikely(fd1 < 0)) {
err = fd1;
- goto out_release_both;
+ goto out_release_sock_2;
}
- fd2 = sock_alloc_fd(&newfile2, flags & O_CLOEXEC);
+ fd2 = sock_alloc_fd(&newfile2, sock2, flags & (O_CLOEXEC | O_NONBLOCK));
if (unlikely(fd2 < 0)) {
err = fd2;
- put_filp(newfile1);
- put_unused_fd(fd1);
- goto out_release_both;
- }
-
- err = sock_attach_fd(sock1, newfile1, flags & O_NONBLOCK);
- if (unlikely(err < 0)) {
- goto out_fd2;
- }
-
- err = sock_attach_fd(sock2, newfile2, flags & O_NONBLOCK);
- if (unlikely(err < 0)) {
- fput(newfile1);
- goto out_fd1;
+ goto out_release_fd_1;
}
audit_fd_pair(fd1, fd2);
@@ -1432,22 +1416,15 @@ SYSCALL_DEFINE4(socketpair, int, family, int, type, int, protocol,
sys_close(fd1);
return err;
-out_release_both:
+out_release_fd_1:
+ fput(newfile1);
+ put_unused_fd(fd1);
+out_release_sock_2:
sock_release(sock2);
-out_release_1:
+out_release_sock_1:
sock_release(sock1);
out:
return err;
-
-out_fd2:
- put_filp(newfile1);
- sock_release(sock1);
-out_fd1:
- put_filp(newfile2);
- sock_release(sock2);
- put_unused_fd(fd1);
- put_unused_fd(fd2);
- goto out;
}
/*
@@ -1551,17 +1528,13 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
*/
__module_get(newsock->ops->owner);
- newfd = sock_alloc_fd(&newfile, flags & O_CLOEXEC);
+ newfd = sock_alloc_fd(&newfile, newsock, flags & (O_CLOEXEC | O_NONBLOCK));
if (unlikely(newfd < 0)) {
err = newfd;
sock_release(newsock);
goto out_put;
}
- err = sock_attach_fd(newsock, newfile, flags & O_NONBLOCK);
- if (err < 0)
- goto out_fd_simple;
-
err = security_socket_accept(sock, newsock);
if (err)
goto out_fd;
@@ -1591,11 +1564,6 @@ out_put:
fput_light(sock->file, fput_needed);
out:
return err;
-out_fd_simple:
- sock_release(newsock);
- put_filp(newfile);
- put_unused_fd(newfd);
- goto out_put;
out_fd:
fput(newfile);
put_unused_fd(newfd);
init-file is no longer used by anything except alloc_file. Make it static and
remove from headers.
Signed-off-by: Eric Paris <[email protected]>
---
fs/file_table.c | 73 ++++++++++++++++++++++----------------------------
include/linux/file.h | 3 --
2 files changed, 32 insertions(+), 44 deletions(-)
diff --git a/fs/file_table.c b/fs/file_table.c
index 4bef4c0..0f9d2f2 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -150,53 +150,16 @@ fail:
EXPORT_SYMBOL(get_empty_filp);
/**
- * alloc_file - allocate and initialize a 'struct file'
- * @mnt: the vfsmount on which the file will reside
- * @dentry: the dentry representing the new file
- * @mode: the mode with which the new file will be opened
- * @fop: the 'struct file_operations' for the new file
- *
- * Use this instead of get_empty_filp() to get a new
- * 'struct file'. Do so because of the same initialization
- * pitfalls reasons listed for init_file(). This is a
- * preferred interface to using init_file().
- *
- * If all the callers of init_file() are eliminated, its
- * code should be moved into this function.
- */
-struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
- fmode_t mode, const struct file_operations *fop)
-{
- struct file *file;
-
- file = get_empty_filp();
- if (!file)
- return NULL;
-
- init_file(file, mnt, dentry, mode, fop);
- return file;
-}
-EXPORT_SYMBOL(alloc_file);
-
-/**
* init_file - initialize a 'struct file'
* @file: the already allocated 'struct file' to initialized
* @mnt: the vfsmount on which the file resides
* @dentry: the dentry representing this file
* @mode: the mode the file is opened with
* @fop: the 'struct file_operations' for this file
- *
- * Use this instead of setting the members directly. Doing so
- * avoids making mistakes like forgetting the mntget() or
- * forgetting to take a write on the mnt.
- *
- * Note: This is a crappy interface. It is here to make
- * merging with the existing users of get_empty_filp()
- * who have complex failure logic easier. All users
- * of this should be moving to alloc_file().
*/
-int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
- fmode_t mode, const struct file_operations *fop)
+static int init_file(struct file *file, struct vfsmount *mnt,
+ struct dentry *dentry, fmode_t mode,
+ const struct file_operations *fop)
{
int error = 0;
file->f_path.dentry = dentry;
@@ -218,7 +181,35 @@ int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
}
return error;
}
-EXPORT_SYMBOL(init_file);
+
+/**
+ * alloc_file - allocate and initialize a 'struct file'
+ * @mnt: the vfsmount on which the file will reside
+ * @dentry: the dentry representing the new file
+ * @mode: the mode with which the new file will be opened
+ * @fop: the 'struct file_operations' for the new file
+ *
+ * Use this instead of get_empty_filp() to get a new
+ * 'struct file'. Do so because of the same initialization
+ * pitfalls reasons listed for init_file(). This is a
+ * preferred interface to using init_file().
+ *
+ * If all the callers of init_file() are eliminated, its
+ * code should be moved into this function.
+ */
+struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
+ fmode_t mode, const struct file_operations *fop)
+{
+ struct file *file;
+
+ file = get_empty_filp();
+ if (!file)
+ return NULL;
+
+ init_file(file, mnt, dentry, mode, fop);
+ return file;
+}
+EXPORT_SYMBOL(alloc_file);
void fput(struct file *file)
{
diff --git a/include/linux/file.h b/include/linux/file.h
index 335a0a5..6a8d361 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -18,9 +18,6 @@ extern void drop_file_write_access(struct file *file);
struct file_operations;
struct vfsmount;
struct dentry;
-extern int init_file(struct file *, struct vfsmount *mnt,
- struct dentry *dentry, fmode_t mode,
- const struct file_operations *fop);
extern struct file *alloc_file(struct vfsmount *, struct dentry *dentry,
fmode_t mode, const struct file_operations *fop);
All users outside of fs/ of get_empty_filp() have been removed. This patch
moves the definition from the include/ directory to internal.h so no new
users crop up and removes the EXPORT_SYMBOL. I'd love to see open intents
stop using it too, but that's a problem for another day and a smarter
developer!
Signed-off-by: Eric Paris <[email protected]>
---
fs/file_table.c | 4 ++--
fs/internal.h | 1 +
fs/namei.c | 2 ++
fs/open.c | 2 ++
include/linux/fs.h | 1 -
5 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/fs/file_table.c b/fs/file_table.c
index 0f9d2f2..629a167 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -24,6 +24,8 @@
#include <asm/atomic.h>
+#include "internal.h"
+
/* sysctl tunables... */
struct files_stat_struct files_stat = {
.max_files = NR_FILE
@@ -147,8 +149,6 @@ fail:
return NULL;
}
-EXPORT_SYMBOL(get_empty_filp);
-
/**
* init_file - initialize a 'struct file'
* @file: the already allocated 'struct file' to initialized
diff --git a/fs/internal.h b/fs/internal.h
index 515175b..f67cd14 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -79,6 +79,7 @@ extern void chroot_fs_refs(struct path *, struct path *);
* file_table.c
*/
extern void mark_files_ro(struct super_block *);
+extern struct file *get_empty_filp(void);
/*
* super.c
diff --git a/fs/namei.c b/fs/namei.c
index 87f97ba..d7ecd2f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -35,6 +35,8 @@
#include <linux/fs_struct.h>
#include <asm/uaccess.h>
+#include "internal.h"
+
#define ACC_MODE(x) ("\000\004\002\006"[(x)&O_ACCMODE])
/* [Feb-1997 T. Schoebel-Theuer]
diff --git a/fs/open.c b/fs/open.c
index fa3bf4c..ebb74d4 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -31,6 +31,8 @@
#include <linux/falloc.h>
#include <linux/fs_struct.h>
+#include "internal.h"
+
int vfs_statfs(struct dentry *dentry, struct kstatfs *buf)
{
int retval = -ENODEV;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5de1bab..b8ed6bf 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2192,7 +2192,6 @@ static inline void insert_inode_hash(struct inode *inode) {
__insert_inode_hash(inode, inode->i_ino);
}
-extern struct file * get_empty_filp(void);
extern void file_move(struct file *f, struct list_head *list);
extern void file_kill(struct file *f);
#ifdef CONFIG_BLOCK
From: Eric Paris <[email protected]>
Date: Thu, 03 Dec 2009 14:59:17 -0500
> Currently the networking code does interesting things allocating its struct
> file and file descriptors. This patch attempts to unify all of that and
> simplify the error paths. It is also a part of my patch series trying to get
> rid of init-file and get-empty_filp and friends.
>
> Signed-off-by: Eric Paris <[email protected]>
I'm fine with this:
Acked-by: David S. Miller <[email protected]>
Quoting Eric Paris ([email protected]):
> init-file is no longer used by anything except alloc_file. Make it static and
> remove from headers.
Should these go through a deprecation period? (Same for the next patch)
> Signed-off-by: Eric Paris <[email protected]>
> ---
>
> fs/file_table.c | 73 ++++++++++++++++++++++----------------------------
> include/linux/file.h | 3 --
> 2 files changed, 32 insertions(+), 44 deletions(-)
>
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 4bef4c0..0f9d2f2 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -150,53 +150,16 @@ fail:
> EXPORT_SYMBOL(get_empty_filp);
>
> /**
> - * alloc_file - allocate and initialize a 'struct file'
> - * @mnt: the vfsmount on which the file will reside
> - * @dentry: the dentry representing the new file
> - * @mode: the mode with which the new file will be opened
> - * @fop: the 'struct file_operations' for the new file
> - *
> - * Use this instead of get_empty_filp() to get a new
> - * 'struct file'. Do so because of the same initialization
> - * pitfalls reasons listed for init_file(). This is a
> - * preferred interface to using init_file().
> - *
> - * If all the callers of init_file() are eliminated, its
> - * code should be moved into this function.
> - */
> -struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
> - fmode_t mode, const struct file_operations *fop)
> -{
> - struct file *file;
> -
> - file = get_empty_filp();
> - if (!file)
> - return NULL;
> -
> - init_file(file, mnt, dentry, mode, fop);
> - return file;
> -}
> -EXPORT_SYMBOL(alloc_file);
> -
> -/**
> * init_file - initialize a 'struct file'
> * @file: the already allocated 'struct file' to initialized
> * @mnt: the vfsmount on which the file resides
> * @dentry: the dentry representing this file
> * @mode: the mode the file is opened with
> * @fop: the 'struct file_operations' for this file
> - *
> - * Use this instead of setting the members directly. Doing so
> - * avoids making mistakes like forgetting the mntget() or
> - * forgetting to take a write on the mnt.
> - *
> - * Note: This is a crappy interface. It is here to make
> - * merging with the existing users of get_empty_filp()
> - * who have complex failure logic easier. All users
> - * of this should be moving to alloc_file().
> */
> -int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
> - fmode_t mode, const struct file_operations *fop)
> +static int init_file(struct file *file, struct vfsmount *mnt,
> + struct dentry *dentry, fmode_t mode,
> + const struct file_operations *fop)
> {
> int error = 0;
> file->f_path.dentry = dentry;
> @@ -218,7 +181,35 @@ int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
> }
> return error;
> }
> -EXPORT_SYMBOL(init_file);
> +
> +/**
> + * alloc_file - allocate and initialize a 'struct file'
> + * @mnt: the vfsmount on which the file will reside
> + * @dentry: the dentry representing the new file
> + * @mode: the mode with which the new file will be opened
> + * @fop: the 'struct file_operations' for the new file
> + *
> + * Use this instead of get_empty_filp() to get a new
> + * 'struct file'. Do so because of the same initialization
> + * pitfalls reasons listed for init_file(). This is a
> + * preferred interface to using init_file().
> + *
> + * If all the callers of init_file() are eliminated, its
> + * code should be moved into this function.
> + */
> +struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
> + fmode_t mode, const struct file_operations *fop)
> +{
> + struct file *file;
> +
> + file = get_empty_filp();
> + if (!file)
> + return NULL;
> +
> + init_file(file, mnt, dentry, mode, fop);
> + return file;
> +}
> +EXPORT_SYMBOL(alloc_file);
>
> void fput(struct file *file)
> {
> diff --git a/include/linux/file.h b/include/linux/file.h
> index 335a0a5..6a8d361 100644
> --- a/include/linux/file.h
> +++ b/include/linux/file.h
> @@ -18,9 +18,6 @@ extern void drop_file_write_access(struct file *file);
> struct file_operations;
> struct vfsmount;
> struct dentry;
> -extern int init_file(struct file *, struct vfsmount *mnt,
> - struct dentry *dentry, fmode_t mode,
> - const struct file_operations *fop);
> extern struct file *alloc_file(struct vfsmount *, struct dentry *dentry,
> fmode_t mode, const struct file_operations *fop);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2009-12-03 at 14:00 -0800, David Miller wrote:
> From: Eric Paris <[email protected]>
> Date: Thu, 03 Dec 2009 14:59:17 -0500
>
> > Currently the networking code does interesting things allocating its struct
> > file and file descriptors. This patch attempts to unify all of that and
> > simplify the error paths. It is also a part of my patch series trying to get
> > rid of init-file and get-empty_filp and friends.
> >
> > Signed-off-by: Eric Paris <[email protected]>
>
> I'm fine with this:
>
> Acked-by: David S. Miller <[email protected]>
It's actually busted, I forgot to actually pass back the new file in
sock_alloc_fd(). But I've got a fixed version and will resend the
series once I see other comments....
inc diff below in case anyone is trying to test this series.
diff --git a/net/socket.c b/net/socket.c
index 41ac0b1..6620421 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -390,6 +390,7 @@ static int sock_alloc_fd(struct file **filep, struct
socket *sock, int flags)
goto out_err;
}
+ *filep = file;
sock->file = file;
SOCK_INODE(sock)->i_fop = &socket_file_ops;
file->f_flags = O_RDWR | (flags & O_NONBLOCK);
On Thu, Dec 03, 2009 at 04:44:02PM -0600, Serge E. Hallyn wrote:
> Quoting Eric Paris ([email protected]):
> > init-file is no longer used by anything except alloc_file. Make it static and
> > remove from headers.
These look like pretty good changes to me, FWIW. I had found myself
wondering about this recently so it's good to see it improved.
> Should these go through a deprecation period? (Same for the next patch)
Maybe waiting a release or two couldn't hurt. Unless Eric you
have some other changes in mind after these?
> > Signed-off-by: Eric Paris <[email protected]>
> > ---
> >
> > fs/file_table.c | 73 ++++++++++++++++++++++----------------------------
> > include/linux/file.h | 3 --
> > 2 files changed, 32 insertions(+), 44 deletions(-)
> >
> > diff --git a/fs/file_table.c b/fs/file_table.c
> > index 4bef4c0..0f9d2f2 100644
> > --- a/fs/file_table.c
> > +++ b/fs/file_table.c
> > @@ -150,53 +150,16 @@ fail:
> > EXPORT_SYMBOL(get_empty_filp);
> >
> > /**
> > - * alloc_file - allocate and initialize a 'struct file'
> > - * @mnt: the vfsmount on which the file will reside
> > - * @dentry: the dentry representing the new file
> > - * @mode: the mode with which the new file will be opened
> > - * @fop: the 'struct file_operations' for the new file
> > - *
> > - * Use this instead of get_empty_filp() to get a new
> > - * 'struct file'. Do so because of the same initialization
> > - * pitfalls reasons listed for init_file(). This is a
> > - * preferred interface to using init_file().
> > - *
> > - * If all the callers of init_file() are eliminated, its
> > - * code should be moved into this function.
> > - */
> > -struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
> > - fmode_t mode, const struct file_operations *fop)
> > -{
> > - struct file *file;
> > -
> > - file = get_empty_filp();
> > - if (!file)
> > - return NULL;
> > -
> > - init_file(file, mnt, dentry, mode, fop);
> > - return file;
> > -}
> > -EXPORT_SYMBOL(alloc_file);
> > -
> > -/**
> > * init_file - initialize a 'struct file'
> > * @file: the already allocated 'struct file' to initialized
> > * @mnt: the vfsmount on which the file resides
> > * @dentry: the dentry representing this file
> > * @mode: the mode the file is opened with
> > * @fop: the 'struct file_operations' for this file
> > - *
> > - * Use this instead of setting the members directly. Doing so
> > - * avoids making mistakes like forgetting the mntget() or
> > - * forgetting to take a write on the mnt.
> > - *
> > - * Note: This is a crappy interface. It is here to make
> > - * merging with the existing users of get_empty_filp()
> > - * who have complex failure logic easier. All users
> > - * of this should be moving to alloc_file().
> > */
> > -int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
> > - fmode_t mode, const struct file_operations *fop)
> > +static int init_file(struct file *file, struct vfsmount *mnt,
> > + struct dentry *dentry, fmode_t mode,
> > + const struct file_operations *fop)
> > {
> > int error = 0;
> > file->f_path.dentry = dentry;
> > @@ -218,7 +181,35 @@ int init_file(struct file *file, struct vfsmount *mnt, struct dentry *dentry,
> > }
> > return error;
> > }
> > -EXPORT_SYMBOL(init_file);
> > +
> > +/**
> > + * alloc_file - allocate and initialize a 'struct file'
> > + * @mnt: the vfsmount on which the file will reside
> > + * @dentry: the dentry representing the new file
> > + * @mode: the mode with which the new file will be opened
> > + * @fop: the 'struct file_operations' for the new file
> > + *
> > + * Use this instead of get_empty_filp() to get a new
> > + * 'struct file'. Do so because of the same initialization
> > + * pitfalls reasons listed for init_file(). This is a
> > + * preferred interface to using init_file().
> > + *
> > + * If all the callers of init_file() are eliminated, its
> > + * code should be moved into this function.
> > + */
> > +struct file *alloc_file(struct vfsmount *mnt, struct dentry *dentry,
> > + fmode_t mode, const struct file_operations *fop)
> > +{
> > + struct file *file;
> > +
> > + file = get_empty_filp();
> > + if (!file)
> > + return NULL;
> > +
> > + init_file(file, mnt, dentry, mode, fop);
> > + return file;
> > +}
> > +EXPORT_SYMBOL(alloc_file);
> >
> > void fput(struct file *file)
> > {
> > diff --git a/include/linux/file.h b/include/linux/file.h
> > index 335a0a5..6a8d361 100644
> > --- a/include/linux/file.h
> > +++ b/include/linux/file.h
> > @@ -18,9 +18,6 @@ extern void drop_file_write_access(struct file *file);
> > struct file_operations;
> > struct vfsmount;
> > struct dentry;
> > -extern int init_file(struct file *, struct vfsmount *mnt,
> > - struct dentry *dentry, fmode_t mode,
> > - const struct file_operations *fop);
> > extern struct file *alloc_file(struct vfsmount *, struct dentry *dentry,
> > fmode_t mode, const struct file_operations *fop);
> >
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to [email protected]
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 03 Dec 2009, Eric Paris wrote:
> shmem uses get_empty_filp() and then init_file(). Their is no good reason
> not to just use alloc_file() like everything else.
There's a more in this patch, though, and none of that is explained...
>
> Signed-off-by: Eric Paris <[email protected]>
> ---
>
> mm/shmem.c | 20 ++++++++++----------
> 1 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 356dd99..831f8bb 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2640,32 +2640,32 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
> if (!dentry)
> goto put_memory;
>
> - error = -ENFILE;
> - file = get_empty_filp();
> - if (!file)
> - goto put_dentry;
> -
> error = -ENOSPC;
> inode = shmem_get_inode(root->d_sb, S_IFREG | S_IRWXUGO, 0, flags);
> if (!inode)
> - goto close_file;
> + goto put_dentry;
>
> d_instantiate(dentry, inode);
> inode->i_size = size;
> inode->i_nlink = 0; /* It is unlinked */
> - init_file(file, shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> - &shmem_file_operations);
> +
> + error = -ENFILE;
> + file = alloc_file(shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> + &shmem_file_operations);
> + if (!file)
> + goto put_dentry;
>
> #ifndef CONFIG_MMU
> error = ramfs_nommu_expand_for_mapping(inode, size);
> if (error)
> goto close_file;
> #endif
> - ima_counts_get(file);
Where's this gone?
> return file;
>
> +#ifndef CONFIG_MMU
> close_file:
I suggest moving this piece of cleanup into the ifdef above, instead
of adding more ifdefs.
> - put_filp(file);
> + fput(file);
OK, put_filp() seems to have been wrong here, but please document it
in the changelog.
> +#endif
> put_dentry:
> dput(dentry);
> put_memory:
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Thu, 03 Dec 2009, Eric Paris wrote:
> The pipe code duplicates the functionality of alloc-file and init-file. Use
> the generic vfs functions instead of duplicating code.
>
> Signed-off-by: Eric Paris <[email protected]>
Acked-by: Miklos Szeredi <[email protected]>
As a side note: I wonder why we aren't passing a "struct path" to
alloc_file() and why are the refcount rules wrt. dentries/vfsmounts so
weird?
> ---
>
> fs/pipe.c | 21 +++++++++------------
> 1 files changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/fs/pipe.c b/fs/pipe.c
> index ae17d02..5d6c969 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -1028,20 +1028,17 @@ void free_write_pipe(struct file *f)
>
> struct file *create_read_pipe(struct file *wrf, int flags)
> {
> - struct file *f = get_empty_filp();
> - if (!f)
> - return ERR_PTR(-ENFILE);
> -
> - /* Grab pipe from the writer */
> - f->f_path = wrf->f_path;
> - path_get(&wrf->f_path);
> - f->f_mapping = wrf->f_path.dentry->d_inode->i_mapping;
> + struct file *f;
> + struct dentry *dentry = wrf->f_path.dentry;
> + struct vfsmount *mnt = wrf->f_path.mnt;
>
> - f->f_pos = 0;
> + dentry = dget(dentry);
> + f = alloc_file(mnt, dentry, FMODE_READ, &read_pipefifo_fops);
> + if (!f) {
> + dput(dentry);
> + return ERR_PTR(-ENFILE);
> + }
> f->f_flags = O_RDONLY | (flags & O_NONBLOCK);
> - f->f_op = &read_pipefifo_fops;
> - f->f_mode = FMODE_READ;
> - f->f_version = 0;
>
> return f;
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Thu, 03 Dec 2009, Eric Paris wrote:
> inotify basically duplicates everything from alloc-file and init-file. Use
> the generic vfs functions instead.
>
> Signed-off-by: Eric Paris <[email protected]>
Acked-by: Miklos Szeredi <[email protected]>
> ---
>
> fs/notify/inotify/inotify_user.c | 23 +++++++++--------------
> 1 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index c40894a..3e03803 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -725,6 +725,7 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
> struct fsnotify_group *group;
> struct user_struct *user;
> struct file *filp;
> + struct dentry *dentry;
> int fd, ret;
>
> /* Check the IN_* constants for consistency. */
> @@ -738,12 +739,6 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
> if (fd < 0)
> return fd;
>
> - filp = get_empty_filp();
> - if (!filp) {
> - ret = -ENFILE;
> - goto out_put_fd;
> - }
> -
> user = get_current_user();
> if (unlikely(atomic_read(&user->inotify_devs) >=
> inotify_max_user_instances)) {
> @@ -758,11 +753,12 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
> goto out_free_uid;
> }
>
> - filp->f_op = &inotify_fops;
> - filp->f_path.mnt = mntget(inotify_mnt);
> - filp->f_path.dentry = dget(inotify_mnt->mnt_root);
> - filp->f_mapping = filp->f_path.dentry->d_inode->i_mapping;
> - filp->f_mode = FMODE_READ;
> + dentry = dget(inotify_mnt->mnt_root);
> + filp = alloc_file(inotify_mnt, dentry, FMODE_READ, &inotify_fops);
> + if (!filp) {
> + ret = -ENFILE;
> + goto out_dput;
> + }
> filp->f_flags = O_RDONLY | (flags & O_NONBLOCK);
> filp->private_data = group;
>
> @@ -771,11 +767,10 @@ SYSCALL_DEFINE1(inotify_init1, int, flags)
> fd_install(fd, filp);
>
> return fd;
> -
> +out_dput:
> + dput(dentry);
> out_free_uid:
> free_uid(user);
> - put_filp(filp);
> -out_put_fd:
> put_unused_fd(fd);
> return ret;
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On Thu, 03 Dec 2009, Eric Paris wrote:
> @@ -391,32 +383,37 @@ static int sock_attach_fd(struct socket *sock, struct file *file, int flags)
> dentry->d_flags &= ~DCACHE_UNHASHED;
> d_instantiate(dentry, SOCK_INODE(sock));
>
> + file = alloc_file(sock_mnt, dentry, FMODE_READ | FMODE_WRITE,
> + &socket_file_ops);
> + if (unlikely(!file)) {
> + rc = -ENFILE;
> + goto out_err;
> + }
> +
> sock->file = file;
> - init_file(file, sock_mnt, dentry, FMODE_READ | FMODE_WRITE,
> - &socket_file_ops);
> SOCK_INODE(sock)->i_fop = &socket_file_ops;
> file->f_flags = O_RDWR | (flags & O_NONBLOCK);
> - file->f_pos = 0;
> file->private_data = sock;
>
> - return 0;
> + return fd;
> +out_err:
> + if (fd >= 0)
> + put_unused_fd(fd);
> + if (dentry)
> + dput(dentry);
Could you please use separate labels intead of conditionals here?
Thanks,
Miklos
On Thu, 03 Dec 2009, Eric Paris wrote:
> All users outside of fs/ of get_empty_filp() have been removed. This patch
> moves the definition from the include/ directory to internal.h so no new
> users crop up and removes the EXPORT_SYMBOL. I'd love to see open intents
> stop using it too, but that's a problem for another day and a smarter
> developer!
ACK for 5/6 and 6/6.
Thanks,
Miklos
On Fri, 2009-12-04 at 06:58 +0100, Miklos Szeredi wrote:
> On Thu, 03 Dec 2009, Eric Paris wrote:
> > shmem uses get_empty_filp() and then init_file(). Their is no good reason
> > not to just use alloc_file() like everything else.
>
> There's a more in this patch, though, and none of that is explained...
>
> >
> > Signed-off-by: Eric Paris <[email protected]>
> > ---
> >
> > mm/shmem.c | 20 ++++++++++----------
> > 1 files changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 356dd99..831f8bb 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -2640,32 +2640,32 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
> > if (!dentry)
> > goto put_memory;
> >
> > - error = -ENFILE;
> > - file = get_empty_filp();
> > - if (!file)
> > - goto put_dentry;
> > -
> > error = -ENOSPC;
> > inode = shmem_get_inode(root->d_sb, S_IFREG | S_IRWXUGO, 0, flags);
> > if (!inode)
> > - goto close_file;
> > + goto put_dentry;
> >
> > d_instantiate(dentry, inode);
> > inode->i_size = size;
> > inode->i_nlink = 0; /* It is unlinked */
> > - init_file(file, shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> > - &shmem_file_operations);
> > +
> > + error = -ENFILE;
> > + file = alloc_file(shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> > + &shmem_file_operations);
> > + if (!file)
> > + goto put_dentry;
> >
> > #ifndef CONFIG_MMU
> > error = ramfs_nommu_expand_for_mapping(inode, size);
> > if (error)
> > goto close_file;
> > #endif
> > - ima_counts_get(file);
>
> Where's this gone?
That's the impetuous for the whole patch series. I originally did the
ima rework first an the vfs changes second, but decided to push the vfs
stuff first and I guess I forgot to back out this bit of ima change I
had done. This will be dropped for actual submission.
> > return file;
> >
> > +#ifndef CONFIG_MMU
> > close_file:
>
> I suggest moving this piece of cleanup into the ifdef above, instead
> of adding more ifdefs.
Ok.
> > - put_filp(file);
> > + fput(file);
>
> OK, put_filp() seems to have been wrong here, but please document it
> in the changelog.
It was only wrong in the ifndef !CONFIG_MMU case. The other error path
which ended up here used it correctly. In any case, I'll mention it in
the changelog.
On Thu, Dec 03 2009, Eric Paris wrote:
> The pipe code duplicates the functionality of alloc-file and init-file. Use
> the generic vfs functions instead of duplicating code.
>
> Signed-off-by: Eric Paris <[email protected]>
Acked-by: Jens Axboe <[email protected]>
--
Jens Axboe
Quoting Eric Paris ([email protected]):
> shmem uses get_empty_filp() and then init_file(). Their is no good reason
> not to just use alloc_file() like everything else.
>
> Signed-off-by: Eric Paris <[email protected]>
So,
Acked-by: Serge Hallyn <[email protected]>
to the first 3 patches. I'll review #4 when you resend. In principle, ack
also to 5 and 6, but for the sake of out-of-tree filesystems I think deprecating
for a version or two would be worthwhile. Of course, if your ima patches also
go through, then the out-of-three filesystems will spit out ima warnings anyway,
but they can consider that further pursuation to switch :)
Thanks, Eric.
-serge
> ---
>
> mm/shmem.c | 20 ++++++++++----------
> 1 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 356dd99..831f8bb 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2640,32 +2640,32 @@ struct file *shmem_file_setup(const char *name, loff_t size, unsigned long flags
> if (!dentry)
> goto put_memory;
>
> - error = -ENFILE;
> - file = get_empty_filp();
> - if (!file)
> - goto put_dentry;
> -
> error = -ENOSPC;
> inode = shmem_get_inode(root->d_sb, S_IFREG | S_IRWXUGO, 0, flags);
> if (!inode)
> - goto close_file;
> + goto put_dentry;
>
> d_instantiate(dentry, inode);
> inode->i_size = size;
> inode->i_nlink = 0; /* It is unlinked */
> - init_file(file, shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> - &shmem_file_operations);
> +
> + error = -ENFILE;
> + file = alloc_file(shm_mnt, dentry, FMODE_WRITE | FMODE_READ,
> + &shmem_file_operations);
> + if (!file)
> + goto put_dentry;
>
> #ifndef CONFIG_MMU
> error = ramfs_nommu_expand_for_mapping(inode, size);
> if (error)
> goto close_file;
> #endif
> - ima_counts_get(file);
> return file;
>
> +#ifndef CONFIG_MMU
> close_file:
> - put_filp(file);
> + fput(file);
> +#endif
> put_dentry:
> dput(dentry);
> put_memory:
On Fri, 2009-12-04 at 07:08 +0100, Miklos Szeredi wrote:
> On Thu, 03 Dec 2009, Eric Paris wrote:
> > The pipe code duplicates the functionality of alloc-file and init-file. Use
> > the generic vfs functions instead of duplicating code.
> >
> > Signed-off-by: Eric Paris <[email protected]>
>
> Acked-by: Miklos Szeredi <[email protected]>
>
> As a side note: I wonder why we aren't passing a "struct path" to
> alloc_file() and why are the refcount rules wrt. dentries/vfsmounts so
> weird?
It's probably because of the slightly weird refcnt rules that it asks
for the dentry and vfsmount separately rather than as a struct path.
The rules make perfect sense if you consider
d_alloc() <-- reference on dentry
d_instantiate()
alloc_file() <-- reference on vfsmount
so here file->f_path() is all good.
Which a number of callers user. They make less sense when you consider
something that is not allocating the dentry right there (like this path)
dget(dentry); <-- reference here
alloc_file() <-- reference on vfsmount;
so here file->f_path is all good.
It would be a reasonable interface if it took a struct path and then
took a reference on the struct path. The second case would look more
clean, but the first case would turn into
d_alloc()
d_instantiate()
alloc_file()
d_put() /* matches d_alloc() */
and
alloc_file()
Is this better? I'll gladly do it if other think so it makes more
sense....
-Eric