[PATCH] ioctl conversion

[PATCH] ioctl conversion

[Stoyan Gaydarov]

This changes the ioctl usage of kernel/power/user.c to use the unlocked_ioctl

Signed-off-by: Stoyan Gaydarov <stoyboyker@gmail.com>

diff -uprN linux-2.6.26-rc9/kernel/power/user.c devel/kernel/power/user.c
--- linux-2.6.26-rc9/kernel/power/user.c        2008-07-05
17:53:22.000000000 -0500
+++ devel/kernel/power/user.c   2008-07-10 19:26:42.000000000 -0500
@@ -18,6 +18,7 @@
 #include <linux/mm.h>
 #include <linux/swap.h>
 #include <linux/swapops.h>
+#include <linux/smp_lock.h>
 #include <linux/pm.h>
 #include <linux/fs.h>
 #include <linux/console.h>
@@ -164,20 +165,28 @@ static ssize_t snapshot_write(struct fil
        return res;
 }

-static int snapshot_ioctl(struct inode *inode, struct file *filp,
-                          unsigned int cmd, unsigned long arg)
+static long snapshot_ioctl(struct file *filp, unsigned int cmd,
+                          unsigned long arg)
 {
        int error = 0;
        struct snapshot_data *data;
        loff_t size;
        sector_t offset;

-       if (_IOC_TYPE(cmd) != SNAPSHOT_IOC_MAGIC)
+       lock_kernel();
+
+       if (_IOC_TYPE(cmd) != SNAPSHOT_IOC_MAGIC) {
+               unlock_kernel();
                return -ENOTTY;
-       if (_IOC_NR(cmd) > SNAPSHOT_IOC_MAXNR)
+       }
+       if (_IOC_NR(cmd) > SNAPSHOT_IOC_MAXNR) {
+               unlock_kernel();
                return -ENOTTY;
-       if (!capable(CAP_SYS_ADMIN))
+       }
+       if (!capable(CAP_SYS_ADMIN)) {
+               unlock_kernel();
                return -EPERM;
+       }

        data = filp->private_data;

@@ -390,6 +399,7 @@ static int snapshot_ioctl(struct inode *

        }

+       unlock_kernel();
        return error;
 }

@@ -399,7 +409,7 @@ static const struct file_operations snap
        .read = snapshot_read,
        .write = snapshot_write,
        .llseek = no_llseek,
-       .ioctl = snapshot_ioctl,
+       .unlocked_ioctl = snapshot_ioctl,
 };

 static struct miscdevice snapshot_device = {

Re: [PATCH] ioctl conversion

[Arnd Bergmann]

On Friday 11 July 2008, Stoyan Gaydarov wrote:
> -       if (_IOC_TYPE(cmd) != SNAPSHOT_IOC_MAGIC)
> +       lock_kernel();
> +
> +       if (_IOC_TYPE(cmd) != SNAPSHOT_IOC_MAGIC) {
> +               unlock_kernel();
>                 return -ENOTTY;
> -       if (_IOC_NR(cmd) > SNAPSHOT_IOC_MAXNR)
> +       }
> +       if (_IOC_NR(cmd) > SNAPSHOT_IOC_MAXNR) {
> +               unlock_kernel();
>                 return -ENOTTY;
> -       if (!capable(CAP_SYS_ADMIN))
> +       }
> +       if (!capable(CAP_SYS_ADMIN)) {
> +               unlock_kernel();
>                 return -EPERM;
> +       }
> 
>         data = filp->private_data;

The more common way to express this is to end the function with

out:
	unlock_kernel();
	return ret;
}

and then jump to that label in the error case. This makes it
much easier to verify that you haven't missed a cased.

	Arnd <><

Re: [PATCH] ioctl conversion

[Stoyan Gaydarov]

On Fri, Jul 11, 2008 at 3:08 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 11 July 2008, Stoyan Gaydarov wrote:
>> -       if (_IOC_TYPE(cmd) != SNAPSHOT_IOC_MAGIC)
>> +       lock_kernel();
>> +
>> +       if (_IOC_TYPE(cmd) != SNAPSHOT_IOC_MAGIC) {
>> +               unlock_kernel();
>>                 return -ENOTTY;
>> -       if (_IOC_NR(cmd) > SNAPSHOT_IOC_MAXNR)
>> +       }
>> +       if (_IOC_NR(cmd) > SNAPSHOT_IOC_MAXNR) {
>> +               unlock_kernel();
>>                 return -ENOTTY;
>> -       if (!capable(CAP_SYS_ADMIN))
>> +       }
>> +       if (!capable(CAP_SYS_ADMIN)) {
>> +               unlock_kernel();
>>                 return -EPERM;
>> +       }
>>
>>         data = filp->private_data;
>
> The more common way to express this is to end the function with
>
> out:
>        unlock_kernel();
>        return ret;
> }

This would work normally but there are three early returns and the
rest are just switch statements that set the return code.

>
> and then jump to that label in the error case. This makes it
> much easier to verify that you haven't missed a cased.
>
>        Arnd <><
>

Re: [PATCH] ioctl conversion

[Arnd Bergmann]

On Friday 11 July 2008, Stoyan Gaydarov wrote:

> This would work normally but there are three early returns and the
> rest are just switch statements that set the return code.

I just looked at linux-next and found that Rafael has already done
the conversion from BKL to his own mutex, after Alan has introduced
the BKL in an earlier patch. Your patch is not needed any more.

It's good to look into the state of the ioctl conversion, but you
should make sure that you're not doing work that has already been
done by someone else, so try checking out linux-next first.

	Arnd <><