[PATCH] 0/3 fs/ioctl.c coding style, rename vfs_ioctl/do_ioctl

[PATCH] 0/3 fs/ioctl.c coding style, rename vfs_ioctl/do_ioctl

[Erez Zadok]

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.

[PATCH 1/3] 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

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

[Christoph Hellwig]

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.

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

[Erez Zadok]

In message <20071028141222.GA29323@infradead.org>, 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.

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

[Christoph Hellwig]

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.

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

[Daniel Phillips]

On 10/28/07, Christoph Hellwig <hch@infradead.org> 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

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

[Christoph Hellwig]

On Sun, Oct 28, 2007 at 07:57:47PM -0700, Daniel Phillips wrote:
> On 10/28/07, Christoph Hellwig <hch@infradead.org> 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.

[PATCH 2/3] 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
modules so others can use it (including Unionfs and eCryptfs).

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

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

[Christoph Hellwig]

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.

[PATCH 3/3] 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