Re: [PATCH 1/3] VFS: apply coding standards to fs/ioctl.c
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. - 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
Re: [PATCH 1/3] VFS: apply coding standards to fs/ioctl.c
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. - 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
Re: [PATCH 1/3] VFS: apply coding standards to fs/ioctl.c
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 - 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
Re: [PATCH 1/3] VFS: apply coding standards to fs/ioctl.c
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.
-
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
Re: [PATCH 1/3] VFS: apply coding standards to fs/ioctl.c
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.
-
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
[PATCH 1/3] VFS: apply coding standards to fs/ioctl.c
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
#include
#include
+#include
-#include
#include
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) {
-
