Home » Mailing list archives » linux-kernel » 2007 » October » 30

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

Previous thread: [patch 13/28] Add cmpxchg_local, cmpxchg64 and cmpxchg64_local to ia64 by Mathieu Desnoyers on Tuesday, October 30, 2007 - 3:16 pm. (1 message)

Next thread: [PATCH v2] Make the dev_*() family of macros in device.h complete by Emil Medve on Tuesday, October 30, 2007 - 3:37 pm. (1 message)
[view original message]
From: Erez Zadok <ezk@...>
To: <hch@...>, <viro@...>, <akpm@...>, <randy.dunlap@...>
Cc: <linux-kernel@...>, <linux-fsdevel@...>
Subject: [PATCH v3] 0/4 fs/ioctl.c coding style, function renaming/factoring
Date: Tuesday, October 30, 2007 - 3:39 pm

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.

-
[view original message]
From: Andrew Morton <akpm@...>
To: Erez Zadok <ezk@...>
Cc: <hch@...>, <viro@...>, <randy.dunlap@...>, <linux-kernel@...>, <linux-fsdevel@...>
Subject: Re: [PATCH v3] 0/4 fs/ioctl.c coding style, function renaming/factoring
Date: Tuesday, October 30, 2007 - 7:04 pm

On Tue, 30 Oct 2007 15:39:55 -0400


The problem is of course that you need these in your tree for ongoing

development, but 2.6.25 is months away.  They look OK to me so I suggest

that you go ahead and commit them to your git tree and I'll drop them

again.  Please resend them for merging in the 2.6.25-rc1 merge window.


unionfs has been hanging around for a long time now and we should work

towards getting it into 2.6.25 or dropped from -mm (sorry).  Right now

would be a great time to get this process underway.  


I have only a partial memory of what the sticking points were, and I have

basically zero knowledge of what was done to address them.  So could you

please, over the next few weeks:


- Send out a description of what the issues were, and how they were addressed


- If issues remain, describe their impact and possible workarounds, all

  that stuff.


- If it mostly-survives all that design-level review and consideration

  then let's go for it: get all the patches cleaned up and consolidated and

  get them emailed out for review no later than 2.6.24-rc5.


Thanks.

-
[view original message]
From: Erez Zadok <ezk@...>
To: Andrew Morton <akpm@...>
Cc: Erez Zadok <ezk@...>, <hch@...>, <viro@...>, <randy.dunlap@...>, <linux-kernel@...>, <linux-fsdevel@...>
Subject: Re: [PATCH v3] 0/4 fs/ioctl.c coding style, function renaming/factoring
Date: Wednesday, October 31, 2007 - 1:37 pm

[Empty message]
[view original message]
From: Erez Zadok <ezk@...>
To: <hch@...>, <viro@...>, <akpm@...>, <randy.dunlap@...>
Cc: <linux-kernel@...>, <linux-fsdevel@...>, Erez Zadok <ezk@...>
Subject: [PATCH 3/4] VFS: factor out three helpers for FIBMAP/FIONBIO/FIOASYNC file ioctls
Date: Tuesday, October 30, 2007 - 3:39 pm

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>

---

 fs/ioctl.c |  129 +++++++++++++++++++++++++++++++++++-------------------------

 1 files changed, 75 insertions(+), 54 deletions(-)


diff --git a/fs/ioctl.c b/fs/ioctl.c

index 1ab7b7d..cd8c1a3 100644

--- a/fs/ioctl.c

+++ b/fs/ioctl.c

@@ -53,32 +53,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:

@@ -88,6 +90,57 @@ static int file_ioctl(struct file *filp, unsigned int cmd,

 	return vfs_ioctl(filp, cmd, arg);

 }


+static int ioctl_fionbio(struct file *filp, int __user *argp)

+{

+	unsigned int flag;

+	int on, error;

+

+	error = get_user(on, argp);

+	if (error)

+		return error;

+	flag = O_NONBLOCK;

+#ifdef __sparc__

+	/* SunOS compatibility item. */

+	if (O...
[ message continues ]
[view original message]
From: Erez Zadok <ezk@...>
To: <hch@...>, <viro@...>, <akpm@...>, <randy.dunlap@...>
Cc: <linux-kernel@...>, <linux-fsdevel@...>, Erez Zadok <ezk@...>
Subject: [PATCH 2/4] VFS: swap do_ioctl and vfs_ioctl names
Date: Tuesday, October 30, 2007 - 3:39 pm

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 @@ s...
[ message continues ]
[view original message]
From: Erez Zadok <ezk@...>
To: <hch@...>, <viro@...>, <akpm@...>, <randy.dunlap@...>
Cc: <linux-kernel@...>, <linux-fsdevel@...>, Erez Zadok <ezk@...>
Subject: [PATCH 1/4] VFS: apply coding standards to fs/ioctl.c
Date: Tuesday, October 30, 2007 - 3:39 pm

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-&...
[ message continues ]
[view original message]
From: Erez Zadok <ezk@...>
To: <hch@...>, <viro@...>, <akpm@...>, <randy.dunlap@...>
Cc: <linux-kernel@...>, <linux-fsdevel@...>, Erez Zadok <ezk@...>
Subject: [PATCH 4/4] Unionfs: use vfs_ioctl
Date: Tuesday, October 30, 2007 - 3:39 pm

Signed-off-by: Erez Zadok <ezk@cs.sunysb.edu>

---

 fs/unionfs/commonfops.c |   36 ++++++------------------------------

 1 files changed, 6 insertions(+), 30 deletions(-)


diff --git a/fs/unionfs/commonfops.c b/fs/unionfs/commonfops.c

index 7654bcb..c99b519 100644

--- a/fs/unionfs/commonfops.c

+++ b/fs/unionfs/commonfops.c

@@ -661,35 +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 = security_file_ioctl(lower_file, cmd, arg);

-	if (err)

-		goto out;

-

-	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

  *

@@ -756,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);


@@ -779,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


-
Previous thread: [patch 13/28] Add cmpxchg_local, cmpxchg64 and cmpxchg64_local to ia64 by Mathieu Desnoyers on Tuesday, October 30, 2007 - 3:16 pm. (1 message)

Next thread: [PATCH v2] Make the dev_*() family of macros in device.h complete by Emil Medve on Tuesday, October 30, 2007 - 3:37 pm. (1 message)