This series of three proposed patches changes fs/ioctl.c and Unionfs as
follows. This series is against v2.6.24-rc1-192-gef49c32.
Patch 1: just applies coding standards to fs/ioctl.c (while I'm at it, I
figured it's worth cleaning VFS files one at a time).
Patch 2: does two things:
(a) Renames the old vfs_ioctl to do_ioctl, because the comment above the old
vfs_ioctl clearly indicates that it is an internal function not to be
exported to modules; therefore it should have a more traditional do_XXX
"internal function" name. The new do_ioctl is exported in fs.h but not
to modules.
(b) Renames the old (static) do_ioctl to vfs_ioctl because the names vfs_XXX
should preferably be reserved to callable VFS functions which modules
may call, as other vfs_XXX functions already do. Export the new
vfs_ioctl to modules so others can use it (including Unionfs and
eCryptfs).
Patch 3: demonstrates how Unionfs can use the new vfs_ioctl. I successfully
tested unionfs with this new exported vfs_ioctl. (eCryptfs could do the
same.)
I'd like to propose that the first two patches be merged in -mm and even
mainline, pending review.
Erez Zadok (3):
VFS: apply coding standards to fs/ioctl.c
VFS: swap do_ioctl and vfs_ioctl names
Unionfs: use vfs_ioctl
fs/compat_ioctl.c | 2
fs/ioctl.c | 176 ++++++++++++++++++++++++------------------------
fs/unionfs/commonfops.c | 22 +-----
include/linux/fs.h | 3
Cheers,
Erez.
Rename old vfs_ioctl to do_ioctl, because the comment above it clearly
indicates that it is an internal function not to be exported to modules;
therefore it should have a more traditional do_XXX name. The new do_ioctl
is exported in fs.h but not to modules.
Rename the old do_ioctl to vfs_ioctl because the names vfs_XXX should
preferably be reserved to callable VFS functions which modules may call,
as many other vfs_XXX functions already do. Export the new vfs_ioctl to
modules so others can use it (including Unionfs and eCryptfs).
Signed-off-by: Erez Zadok <[email protected]>
---
fs/compat_ioctl.c | 2 +-
fs/ioctl.c | 18 ++++++++++--------
include/linux/fs.h | 3 ++-
3 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index a4284cc..a1604ce 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -2972,7 +2972,7 @@ asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd,
}
do_ioctl:
- error = vfs_ioctl(filp, fd, cmd, arg);
+ error = do_ioctl(filp, fd, cmd, arg);
out_fput:
fput_light(filp, fput_needed);
out:
diff --git a/fs/ioctl.c b/fs/ioctl.c
index 652cacf..00abbbf 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -16,8 +16,9 @@
#include <asm/ioctls.h>
-static long do_ioctl(struct file *filp, unsigned int cmd,
- unsigned long arg)
+/* vfs_ioctl can be called by other file systems or modules */
+long vfs_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
{
int error = -ENOTTY;
@@ -39,6 +40,7 @@ static long do_ioctl(struct file *filp, unsigned int cmd,
out:
return error;
}
+EXPORT_SYMBOL(vfs_ioctl);
static int file_ioctl(struct file *filp, unsigned int cmd,
unsigned long arg)
@@ -72,18 +74,18 @@ static int file_ioctl(struct file *filp, unsigned int cmd,
return put_user(i_size_read(inode) - filp->f_pos, p);
}
- return do_ioctl(filp, cmd, arg);
+ return vfs_ioctl(filp, cmd, arg);
}
/*
* When you add any new common ioctls to the switches above and below
* please update compat_sys_ioctl() too.
*
- * vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
+ * do_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
* It's just a simple helper for sys_ioctl and compat_sys_ioctl.
*/
-int vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
- unsigned long arg)
+int do_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
+ unsigned long arg)
{
unsigned int flag;
int on, error = 0;
@@ -152,7 +154,7 @@ int vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
error = file_ioctl(filp, cmd, arg);
else
- error = do_ioctl(filp, cmd, arg);
+ error = vfs_ioctl(filp, cmd, arg);
break;
}
return error;
@@ -172,7 +174,7 @@ asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
if (error)
goto out_fput;
- error = vfs_ioctl(filp, fd, cmd, arg);
+ error = do_ioctl(filp, fd, cmd, arg);
out_fput:
fput_light(filp, fput_needed);
out:
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b3ec4a4..c0c5d36 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1924,7 +1924,8 @@ extern int vfs_stat_fd(int dfd, char __user *, struct kstat *);
extern int vfs_lstat_fd(int dfd, char __user *, struct kstat *);
extern int vfs_fstat(unsigned int, struct kstat *);
-extern int vfs_ioctl(struct file *, unsigned int, unsigned int, unsigned long);
+extern long vfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg);
+extern int do_ioctl(struct file *, unsigned int, unsigned int, unsigned long);
extern void get_filesystem(struct file_system_type *fs);
extern void put_filesystem(struct file_system_type *fs);
--
1.5.2.2
Signed-off-by: Erez Zadok <[email protected]>
---
fs/ioctl.c | 164 +++++++++++++++++++++++++++++++-----------------------------
1 files changed, 84 insertions(+), 80 deletions(-)
diff --git a/fs/ioctl.c b/fs/ioctl.c
index c2a773e..652cacf 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -12,8 +12,8 @@
#include <linux/fs.h>
#include <linux/security.h>
#include <linux/module.h>
+#include <linux/uaccess.h>
-#include <asm/uaccess.h>
#include <asm/ioctls.h>
static long do_ioctl(struct file *filp, unsigned int cmd,
@@ -45,31 +45,31 @@ static int file_ioctl(struct file *filp, unsigned int cmd,
{
int error;
int block;
- struct inode * inode = filp->f_path.dentry->d_inode;
+ struct inode *inode = filp->f_path.dentry->d_inode;
int __user *p = (int __user *)arg;
switch (cmd) {
- case FIBMAP:
- {
- struct address_space *mapping = filp->f_mapping;
- int res;
- /* do we support this mess? */
- if (!mapping->a_ops->bmap)
- return -EINVAL;
- if (!capable(CAP_SYS_RAWIO))
- return -EPERM;
- if ((error = get_user(block, p)) != 0)
- return error;
-
- lock_kernel();
- res = mapping->a_ops->bmap(mapping, block);
- unlock_kernel();
- return put_user(res, p);
- }
- case FIGETBSZ:
- return put_user(inode->i_sb->s_blocksize, p);
- case FIONREAD:
- return put_user(i_size_read(inode) - filp->f_pos, p);
+ case FIBMAP:
+ {
+ struct address_space *mapping = filp->f_mapping;
+ int res;
+ /* do we support this mess? */
+ if (!mapping->a_ops->bmap)
+ return -EINVAL;
+ if (!capable(CAP_SYS_RAWIO))
+ return -EPERM;
+ error = get_user(block, p);
+ if (error)
+ return error;
+ lock_kernel();
+ res = mapping->a_ops->bmap(mapping, block);
+ unlock_kernel();
+ return put_user(res, p);
+ }
+ case FIGETBSZ:
+ return put_user(inode->i_sb->s_blocksize, p);
+ case FIONREAD:
+ return put_user(i_size_read(inode) - filp->f_pos, p);
}
return do_ioctl(filp, cmd, arg);
@@ -82,81 +82,85 @@ static int file_ioctl(struct file *filp, unsigned int cmd,
* vfs_ioctl() is not for drivers and not intended to be EXPORT_SYMBOL()'d.
* It's just a simple helper for sys_ioctl and compat_sys_ioctl.
*/
-int vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd, unsigned long arg)
+int vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
+ unsigned long arg)
{
unsigned int flag;
int on, error = 0;
switch (cmd) {
- case FIOCLEX:
- set_close_on_exec(fd, 1);
- break;
+ case FIOCLEX:
+ set_close_on_exec(fd, 1);
+ break;
- case FIONCLEX:
- set_close_on_exec(fd, 0);
- break;
+ case FIONCLEX:
+ set_close_on_exec(fd, 0);
+ break;
- case FIONBIO:
- if ((error = get_user(on, (int __user *)arg)) != 0)
- break;
- flag = O_NONBLOCK;
+ case FIONBIO:
+ error = get_user(on, (int __user *)arg);
+ if (error)
+ break;
+ flag = O_NONBLOCK;
#ifdef __sparc__
- /* SunOS compatibility item. */
- if(O_NONBLOCK != O_NDELAY)
- flag |= O_NDELAY;
+ /* SunOS compatibility item. */
+ if (O_NONBLOCK != O_NDELAY)
+ flag |= O_NDELAY;
#endif
- if (on)
- filp->f_flags |= flag;
- else
- filp->f_flags &= ~flag;
+ if (on)
+ filp->f_flags |= flag;
+ else
+ filp->f_flags &= ~flag;
+ break;
+
+ case FIOASYNC:
+ error = get_user(on, (int __user *)arg);
+ if (error)
break;
-
- case FIOASYNC:
- if ((error = get_user(on, (int __user *)arg)) != 0)
- break;
- flag = on ? FASYNC : 0;
-
- /* Did FASYNC state change ? */
- if ((flag ^ filp->f_flags) & FASYNC) {
- if (filp->f_op && filp->f_op->fasync) {
- lock_kernel();
- error = filp->f_op->fasync(fd, filp, on);
- unlock_kernel();
- }
- else error = -ENOTTY;
- }
- if (error != 0)
- break;
-
- if (on)
- filp->f_flags |= FASYNC;
- else
- filp->f_flags &= ~FASYNC;
- break;
-
- case FIOQSIZE:
- if (S_ISDIR(filp->f_path.dentry->d_inode->i_mode) ||
- S_ISREG(filp->f_path.dentry->d_inode->i_mode) ||
- S_ISLNK(filp->f_path.dentry->d_inode->i_mode)) {
- loff_t res = inode_get_bytes(filp->f_path.dentry->d_inode);
- error = copy_to_user((loff_t __user *)arg, &res, sizeof(res)) ? -EFAULT : 0;
- }
- else
+ flag = on ? FASYNC : 0;
+
+ /* Did FASYNC state change ? */
+ if ((flag ^ filp->f_flags) & FASYNC) {
+ if (filp->f_op && filp->f_op->fasync) {
+ lock_kernel();
+ error = filp->f_op->fasync(fd, filp, on);
+ unlock_kernel();
+ } else
error = -ENOTTY;
+ }
+ if (error != 0)
break;
- default:
- if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
- error = file_ioctl(filp, cmd, arg);
- else
- error = do_ioctl(filp, cmd, arg);
- break;
+
+ if (on)
+ filp->f_flags |= FASYNC;
+ else
+ filp->f_flags &= ~FASYNC;
+ break;
+
+ case FIOQSIZE:
+ if (S_ISDIR(filp->f_path.dentry->d_inode->i_mode) ||
+ S_ISREG(filp->f_path.dentry->d_inode->i_mode) ||
+ S_ISLNK(filp->f_path.dentry->d_inode->i_mode)) {
+ loff_t res =
+ inode_get_bytes(filp->f_path.dentry->d_inode);
+ error = copy_to_user((loff_t __user *)arg, &res,
+ sizeof(res)) ? -EFAULT : 0;
+ } else
+ error = -ENOTTY;
+ break;
+ default:
+ if (S_ISREG(filp->f_path.dentry->d_inode->i_mode))
+ error = file_ioctl(filp, cmd, arg);
+ else
+ error = do_ioctl(filp, cmd, arg);
+ break;
}
return error;
}
asmlinkage long sys_ioctl(unsigned int fd, unsigned int cmd, unsigned long arg)
{
- struct file * filp;
+ struct file *filp;
int error = -EBADF;
int fput_needed;
--
1.5.2.2
Signed-off-by: Erez Zadok <[email protected]>
---
fs/unionfs/commonfops.c | 32 ++++++--------------------------
1 files changed, 6 insertions(+), 26 deletions(-)
diff --git a/fs/unionfs/commonfops.c b/fs/unionfs/commonfops.c
index 50e5775..c99b519 100644
--- a/fs/unionfs/commonfops.c
+++ b/fs/unionfs/commonfops.c
@@ -661,31 +661,6 @@ out:
return err;
}
-/* pass the ioctl to the lower fs */
-static long do_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
-{
- struct file *lower_file;
- int err;
-
- lower_file = unionfs_lower_file(file);
-
- err = -ENOTTY;
- if (!lower_file || !lower_file->f_op)
- goto out;
- if (lower_file->f_op->unlocked_ioctl) {
- err = lower_file->f_op->unlocked_ioctl(lower_file, cmd, arg);
- } else if (lower_file->f_op->ioctl) {
- lock_kernel();
- err = lower_file->f_op->ioctl(
- lower_file->f_path.dentry->d_inode,
- lower_file, cmd, arg);
- unlock_kernel();
- }
-
-out:
- return err;
-}
-
/*
* return to user-space the branch indices containing the file in question
*
@@ -752,6 +727,7 @@ out:
long unionfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
{
long err;
+ struct file *lower_file;
unionfs_read_lock(file->f_path.dentry->d_sb);
@@ -775,7 +751,11 @@ long unionfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
default:
/* pass the ioctl down */
- err = do_ioctl(file, cmd, arg);
+ lower_file = unionfs_lower_file(file);
+ if (lower_file)
+ err = vfs_ioctl(lower_file, cmd, arg);
+ else
+ err = -ENOTTY;
break;
}
--
1.5.2.2
Nice, I always hated these double-indented switch statements.
> + case FIBMAP:
> + {
> + struct address_space *mapping = filp->f_mapping;
> + int res;
> + /* do we support this mess? */
> + if (!mapping->a_ops->bmap)
> + return -EINVAL;
> + if (!capable(CAP_SYS_RAWIO))
> + return -EPERM;
> + error = get_user(block, p);
> + if (error)
> + return error;
> + lock_kernel();
> + res = mapping->a_ops->bmap(mapping, block);
> + unlock_kernel();
> + return put_user(res, p);
While you're at it, it's probably worth splitting this out into
a small helper function.
> + case FIONBIO:
> + error = get_user(on, (int __user *)arg);
> + if (error)
> + break;
> + flag = O_NONBLOCK;
> #ifdef __sparc__
> + /* SunOS compatibility item. */
> + if (O_NONBLOCK != O_NDELAY)
> + flag |= O_NDELAY;
> #endif
> + if (on)
> + filp->f_flags |= flag;
> + else
> + filp->f_flags &= ~flag;
> + break;
Same here.
> + case FIOASYNC:
> + error = get_user(on, (int __user *)arg);
> + if (error)
> break;
> + flag = on ? FASYNC : 0;
> +
> + /* Did FASYNC state change ? */
> + if ((flag ^ filp->f_flags) & FASYNC) {
> + if (filp->f_op && filp->f_op->fasync) {
> + lock_kernel();
> + error = filp->f_op->fasync(fd, filp, on);
> + unlock_kernel();
> + } else
> error = -ENOTTY;
> + }
> + if (error != 0)
> break;
> +
> + if (on)
> + filp->f_flags |= FASYNC;
> + else
> + filp->f_flags &= ~FASYNC;
> + break;
And here.
On Sat, Oct 27, 2007 at 07:10:44PM -0400, Erez Zadok wrote:
> Rename old vfs_ioctl to do_ioctl, because the comment above it clearly
> indicates that it is an internal function not to be exported to modules;
> therefore it should have a more traditional do_XXX name. The new do_ioctl
> is exported in fs.h but not to modules.
>
> Rename the old do_ioctl to vfs_ioctl because the names vfs_XXX should
> preferably be reserved to callable VFS functions which modules may call,
> as many other vfs_XXX functions already do.
Yes, good idea for consistency.
> Export the new vfs_ioctl to
> modules so others can use it (including Unionfs and eCryptfs).
> +EXPORT_SYMBOL(vfs_ioctl);
This should be an _GPL export. All new exports for above VFS users
like NFSD or stackable filesystem are _GPL because they're not for
general consumptions but these rather specific tied to the kernel
uses.
And while you're at it I'd suggest adding a docbook comment describing
the new export vfs_ioctl because we'd eventually like to have the exported
API documented fully.
In message <[email protected]>, Christoph Hellwig writes:
> Nice, I always hated these double-indented switch statements.
>
> > + case FIBMAP:
> > + {
> > + struct address_space *mapping = filp->f_mapping;
> > + int res;
> > + /* do we support this mess? */
> > + if (!mapping->a_ops->bmap)
> > + return -EINVAL;
> > + if (!capable(CAP_SYS_RAWIO))
> > + return -EPERM;
> > + error = get_user(block, p);
> > + if (error)
> > + return error;
> > + lock_kernel();
> > + res = mapping->a_ops->bmap(mapping, block);
> > + unlock_kernel();
> > + return put_user(res, p);
>
> While you're at it, it's probably worth splitting this out into
> a small helper function.
Sure. I assume you mean an internal function to encapsulate the entire case
statement's code, one for each of the FIO* cases.
Erez.
On 10/28/07, Christoph Hellwig <[email protected]> wrote:
> While you're at it, it's probably worth splitting this out into
> a small helper function.
Why? Is the same pattern called from more than one place?
Regards,
Daniel
On Sun, Oct 28, 2007 at 02:05:16PM -0400, Erez Zadok wrote:
>
> Sure. I assume you mean an internal function to encapsulate the entire case
> statement's code, one for each of the FIO* cases.
Yes.
On Sun, Oct 28, 2007 at 07:57:47PM -0700, Daniel Phillips wrote:
> On 10/28/07, Christoph Hellwig <[email protected]> wrote:
> > While you're at it, it's probably worth splitting this out into
> > a small helper function.
>
> Why? Is the same pattern called from more than one place?
Becauase it's a lot more readable. ioctl subcommands are invidividual
functionality, and separating them out into small self-contained functions
makes the code a lot more readable.