[PATCH] 0/4 fs/ioctl.c coding style, rename vfs_ioctl/do_ioctl, refactoring (take 2)

[PATCH] 0/4 fs/ioctl.c coding style, rename vfs_ioctl/do_ioctl, refactoring (take 2)

[Erez Zadok]

This series of 4 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 it
    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 (GPL) modules so others can use it (including Unionfs and
    eCryptfs).

Patch 3: factors out the switch statements' cases for
FIBMAP/FIONBIO/FIOASYNC, into three small static helper functions.

Patch 4: 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 3 patches be merged in -mm and even
mainline, pending review.

Erez Zadok (4):
      VFS: apply coding standards to fs/ioctl.c
      VFS: swap do_ioctl and vfs_ioctl names
      VFS: factor out three helpers for FIBMAP/FIONBIO/FIOASYNC file ioctls
      Unionfs: use vfs_ioctl

 fs/compat_ioctl.c       |    2 
 fs/ioctl.c              |  224 ++++++++++++++++++++++++++++--------------------
 fs/unionfs/commonfops.c |   32 +-----
 include/linux/fs.h      |    3 
 4 files changed, 140 insertions(+), 121 deletions(-)

Cheers,
Erez.

[PATCH 1/4] VFS: apply coding standards to fs/ioctl.c

[Erez Zadok]

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
 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

[PATCH 2/4] VFS: swap do_ioctl and vfs_ioctl names

[Erez Zadok]

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 GPL
modules so others can use it (including Unionfs and eCryptfs).  Add DocBook
for new vfs_ioctl.

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
 fs/compat_ioctl.c  |    2 +-
 fs/ioctl.c         |   30 ++++++++++++++++++++++--------
 include/linux/fs.h |    3 ++-
 3 files changed, 25 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..34e3f58 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -16,8 +16,21 @@
 
 #include <asm/ioctls.h>
 
-static long do_ioctl(struct file *filp, unsigned int cmd,
-		unsigned long arg)
+/**
+ * vfs_ioctl - call filesystem specific ioctl methods
+ *
+ * @filp: [in]     open file to invoke ioctl method on
+ * @cmd:  [in]     ioctl command to execute
+ * @arg:  [in/out] command-specific argument for ioctl
+ *
+ * Invokes filesystem specific ->unlock_ioctl, if one exists; otherwise
+ * invokes * filesystem specific ->ioctl method.  If neither method exists,
+ * returns -ENOTTY.
+ *
+ * Returns 0 on success, -errno on error.
+ */
+long vfs_ioctl(struct file *filp, unsigned int cmd,
+	       unsigned long arg)
 {
 	int error = -ENOTTY;
 
@@ -39,6 +52,7 @@ static long do_ioctl(struct file *filp, unsigned int cmd,
  out:
 	return error;
 }
+EXPORT_SYMBOL_GPL(vfs_ioctl);
 
 static int file_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
@@ -72,18 +86,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 +166,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 +186,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

Re: [PATCH 2/4] VFS: swap do_ioctl and vfs_ioctl names

[Randy Dunlap]

On Sun, 28 Oct 2007 20:40:56 -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.  Export the new vfs_ioctl to GPL
> modules so others can use it (including Unionfs and eCryptfs).  Add DocBook
> for new vfs_ioctl.
> 
> Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
> ---
>  fs/compat_ioctl.c  |    2 +-
>  fs/ioctl.c         |   30 ++++++++++++++++++++++--------
>  include/linux/fs.h |    3 ++-
>  3 files changed, 25 insertions(+), 10 deletions(-)
> 

> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index 652cacf..34e3f58 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -16,8 +16,21 @@
>  
>  #include <asm/ioctls.h>
>  
> -static long do_ioctl(struct file *filp, unsigned int cmd,
> -		unsigned long arg)
> +/**
> + * vfs_ioctl - call filesystem specific ioctl methods
> + *

No "blank" line allowed in kernel-doc between function name and its
parameters.

> + * @filp: [in]     open file to invoke ioctl method on
> + * @cmd:  [in]     ioctl command to execute
> + * @arg:  [in/out] command-specific argument for ioctl
> + *
> + * Invokes filesystem specific ->unlock_ioctl, if one exists; otherwise
> + * invokes * filesystem specific ->ioctl method.  If neither method exists,
> + * returns -ENOTTY.
> + *
> + * Returns 0 on success, -errno on error.
> + */
> +long vfs_ioctl(struct file *filp, unsigned int cmd,
> +	       unsigned long arg)
>  {
>  	int error = -ENOTTY;
>  

> 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);

Use/keep parameter names, please.  That is preferred.

>  extern void get_filesystem(struct file_system_type *fs);
>  extern void put_filesystem(struct file_system_type *fs);
> -- 
> 1.5.2.2
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 


---
~Randy

Re: [PATCH 2/4] VFS: swap do_ioctl and vfs_ioctl names

[Christoph Hellwig]

On Sun, Oct 28, 2007 at 08:40:56PM -0400, Erez Zadok wrote:
> +/**
> + * vfs_ioctl - call filesystem specific ioctl methods
> + *
> + * @filp: [in]     open file to invoke ioctl method on
> + * @cmd:  [in]     ioctl command to execute
> + * @arg:  [in/out] command-specific argument for ioctl

I've never seen these in/out annotations and doubt they're valid in
kerneldoc.  Randy?

> + * Invokes filesystem specific ->unlock_ioctl, if one exists; otherwise

unlocked_ioctl

Re: [PATCH 2/4] VFS: swap do_ioctl and vfs_ioctl names

[Randy Dunlap]

On Tue, 30 Oct 2007 09:56:53 +0000 Christoph Hellwig wrote:

> On Sun, Oct 28, 2007 at 08:40:56PM -0400, Erez Zadok wrote:
> > +/**
> > + * vfs_ioctl - call filesystem specific ioctl methods
> > + *
> > + * @filp: [in]     open file to invoke ioctl method on
> > + * @cmd:  [in]     ioctl command to execute
> > + * @arg:  [in/out] command-specific argument for ioctl
> 
> I've never seen these in/out annotations and doubt they're valid in
> kerneldoc.  Randy?

They are just treated as part of the parameter explanation text.
I don't see any problem with them.

> > + * Invokes filesystem specific ->unlock_ioctl, if one exists; otherwise
> 
> unlocked_ioctl


---
~Randy

Re: [PATCH 2/4] VFS: swap do_ioctl and vfs_ioctl names

[Christoph Hellwig]

On Tue, Oct 30, 2007 at 08:22:40AM -0700, Randy Dunlap wrote:
> They are just treated as part of the parameter explanation text.
> I don't see any problem with them.

Well, it's completely inconsistant with any other kerneldoc..

Re: [PATCH 2/4] VFS: swap do_ioctl and vfs_ioctl names

[Randy Dunlap]

On Tue, 30 Oct 2007 17:14:36 +0000 Christoph Hellwig wrote:

> On Tue, Oct 30, 2007 at 08:22:40AM -0700, Randy Dunlap wrote:
> > They are just treated as part of the parameter explanation text.
> > I don't see any problem with them.
> 
> Well, it's completely inconsistant with any other kerneldoc..

True.  and it has an advantage.

---
~Randy

Re: [PATCH 2/4] VFS: swap do_ioctl and vfs_ioctl names

[Erez Zadok]

In message <20071030171436.GA15922@infradead.org>, Christoph Hellwig writes:
> On Tue, Oct 30, 2007 at 08:22:40AM -0700, Randy Dunlap wrote:
> > They are just treated as part of the parameter explanation text.
> > I don't see any problem with them.
> 
> Well, it's completely inconsistant with any other kerneldoc..

If it doesn't hurt, and kerneldoc will present it as such, then I'd leave
the [in/out] in place.  Ioctls are the few places where you can have
variables used as both input and output; so _somehow_ we need to capture
that behavior.

Erez.

[PATCH 3/4] VFS: factor out three helpers for FIBMAP/FIONBIO/FIOASYNC file ioctls

[Erez Zadok]

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
 fs/ioctl.c |  128 ++++++++++++++++++++++++++++++++++-------------------------
 1 files changed, 74 insertions(+), 54 deletions(-)

diff --git a/fs/ioctl.c b/fs/ioctl.c
index 34e3f58..8dd2ef1 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -54,32 +54,34 @@ long vfs_ioctl(struct file *filp, unsigned int cmd,
 }
 EXPORT_SYMBOL_GPL(vfs_ioctl);
 
+static int __ioctl_fibmap(struct file *filp, int __user *p)
+{
+	struct address_space *mapping = filp->f_mapping;
+	int res, block;
+
+	/* do we support this mess? */
+	if (!mapping->a_ops->bmap)
+		return -EINVAL;
+	if (!capable(CAP_SYS_RAWIO))
+		return -EPERM;
+	res = get_user(block, p);
+	if (res)
+		return res;
+	lock_kernel();
+	res = mapping->a_ops->bmap(mapping, block);
+	unlock_kernel();
+	return put_user(res, p);
+}
+
 static int file_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
-	int error;
-	int block;
 	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;
-		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);
-	}
+		return __ioctl_fibmap(filp, p);
 	case FIGETBSZ:
 		return put_user(inode->i_sb->s_blocksize, p);
 	case FIONREAD:
@@ -89,6 +91,57 @@ static int file_ioctl(struct file *filp, unsigned int cmd,
 	return vfs_ioctl(filp, cmd, arg);
 }
 
+static int __ioctl_fionbio(struct file *filp, unsigned long arg)
+{
+	unsigned int flag;
+	int on, error;
+
+	error = get_user(on, (int __user *)arg);
+	if (error)
+		return error;
+	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;
+	return error;
+}
+
+static int __ioctl_fioasync(unsigned int fd, struct file *filp,
+			    unsigned long arg)
+{
+	unsigned int flag;
+	int on, error;
+
+	error = get_user(on, (int __user *)arg);
+	if (error)
+		return error;
+	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)
+		return error;
+
+	if (on)
+		filp->f_flags |= FASYNC;
+	else
+		filp->f_flags &= ~FASYNC;
+	return error;
+}
+
 /*
  * When you add any new common ioctls to the switches above and below
  * please update compat_sys_ioctl() too.
@@ -99,8 +152,7 @@ static int file_ioctl(struct file *filp, unsigned int cmd,
 int do_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
 	     unsigned long arg)
 {
-	unsigned int flag;
-	int on, error = 0;
+	int error = 0;
 
 	switch (cmd) {
 	case FIOCLEX:
@@ -112,43 +164,11 @@ int do_ioctl(struct file *filp, unsigned int fd, unsigned int cmd,
 		break;
 
 	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;
+		error = __ioctl_fionbio(filp, arg);
 		break;
 
 	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;
+		error = __ioctl_fioasync(fd, filp, arg);
 		break;
 
 	case FIOQSIZE:
-- 
1.5.2.2

Re: [PATCH 3/4] VFS: factor out three helpers for FIBMAP/FIONBIO/FIOASYNC file ioctls

[Christoph Hellwig]

> +static int __ioctl_fibmap(struct file *filp, int __user *p)

I'd say kill the __ prefix for all the functions you're adding.

> +static int __ioctl_fionbio(struct file *filp, unsigned long arg)

> +static int __ioctl_fioasync(unsigned int fd, struct file *filp,
> +			    unsigned long arg)

For these two I'd add a

	void __user *argp = (void __user *)arg;

in the caller and then just pass argp down.  That way we have the cast in
one place.

Re: [PATCH 3/4] VFS: factor out three helpers for FIBMAP/FIONBIO/FIOASYNC file ioctls

[Erez Zadok]

BTW, what's the origin of this oddity in fs/ioctl.c:

#ifdef __sparc__
	/* SunOS compatibility item. */
	if (O_NONBLOCK != O_NDELAY)
		flag |= O_NDELAY;
#endif

It seems rather odd to have architecture-specific code in the VFS, no?

Erez.

Re: [PATCH 3/4] VFS: factor out three helpers for FIBMAP/FIONBIO/FIOASYNC file ioctls

[Christoph Hellwig]

On Tue, Oct 30, 2007 at 01:49:48PM -0400, Erez Zadok wrote:
> BTW, what's the origin of this oddity in fs/ioctl.c:
> 
> #ifdef __sparc__
> 	/* SunOS compatibility item. */
> 	if (O_NONBLOCK != O_NDELAY)
> 		flag |= O_NDELAY;
> #endif
> 
> It seems rather odd to have architecture-specific code in the VFS, no?

When Dave did the sparc port he followed sunos ABIs for lots of things.
When these ABIs are hidden behind ioctl arguments they can't easily
be handled in arch code and we need warts like that.  It's definitively
not recommended for new ports..

[PATCH 4/4] Unionfs: use vfs_ioctl

[Erez Zadok]

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
 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

[PATCH v3] 0/4 fs/ioctl.c coding style, function renaming/factoring

[Erez Zadok]

This series of 4 proposed patches (take 3) changes fs/ioctl.c and Unionfs as
follows.  This series is against v2.6.24-rc1-423-g97855b4.

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 it
    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 (GPL) modules so others can use it (including Unionfs and
    eCryptfs).

Patch 3: factors out the switch statements' cases for
FIBMAP/FIONBIO/FIOASYNC, into three small static helper functions.

Patch 4: 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 3 patches be merged in -mm and even
mainline, pending review.

Erez Zadok (4):
      VFS: apply coding standards to fs/ioctl.c
      VFS: swap do_ioctl and vfs_ioctl names
      VFS: factor out three helpers for FIBMAP/FIONBIO/FIOASYNC file ioctls
      Unionfs: use vfs_ioctl

 fs/compat_ioctl.c       |    2 
 fs/ioctl.c              |  224 ++++++++++++++++++++++++++++--------------------
 fs/unionfs/commonfops.c |   36 +------
 include/linux/fs.h      |    4 
 4 files changed, 141 insertions(+), 125 deletions(-)

Cheers,
Erez.

[PATCH 2/4] VFS: swap do_ioctl and vfs_ioctl names

[Erez Zadok]

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 GPL
modules so others can use it (including Unionfs and eCryptfs).  Add DocBook
for new vfs_ioctl.

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>
---
 fs/compat_ioctl.c  |    2 +-
 fs/ioctl.c         |   29 +++++++++++++++++++++--------
 include/linux/fs.h |    4 +++-
 3 files changed, 25 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..1ab7b7d 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -16,8 +16,20 @@
 
 #include <asm/ioctls.h>
 
-static long do_ioctl(struct file *filp, unsigned int cmd,
-		unsigned long arg)
+/**
+ * vfs_ioctl - call filesystem specific ioctl methods
+ * @filp: [in]     open file to invoke ioctl method on
+ * @cmd:  [in]     ioctl command to execute
+ * @arg:  [in/out] command-specific argument for ioctl
+ *
+ * Invokes filesystem specific ->unlocked_ioctl, if one exists; otherwise
+ * invokes * filesystem specific ->ioctl method.  If neither method exists,
+ * returns -ENOTTY.
+ *
+ * Returns 0 on success, -errno on error.
+ */
+long vfs_ioctl(struct file *filp, unsigned int cmd,
+	       unsigned long arg)
 {
 	int error = -ENOTTY;
 
@@ -39,6 +51,7 @@ static long do_ioctl(struct file *filp, unsigned int cmd,
  out:
 	return error;
 }
+EXPORT_SYMBOL_GPL(vfs_ioctl);
 
 static int file_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
@@ -72,18 +85,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 +165,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 +185,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..d513f16 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1924,7 +1924,9 @@ 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 *filp, unsigned int fd, unsigned int cmd,
+		    unsigned long arg);
 
 extern void get_filesystem(struct file_system_type *fs);
 extern void put_filesystem(struct file_system_type *fs);
-- 
1.5.2.2