Re: [JANITOR PROPOSAL] Switch ioctl functions to ->unlocked_ioctl

["Paolo Ciarrocchi" <paolo.ciarrocchi@gmail.com>]

On Jan 8, 2008 9:42 PM, Andi Kleen <andi@firstfloor.org> wrote:
> > I grepped and tried to do what you suggested.
>
> First a quick question: how would you rate your C knowledge? Did you
> ever write a program yourself?

Yes I did but I probably beeing inactive for too much time,
I need to back studing a bit before submitting another patch.

> My proposal assumes that you have at least basic C knowledge.
>
> > The first file that git grep reported was:
> > arch/arm/common/rtctime.c:static const struct file_operations rtc_fops = {
>
> It's probably better to only do that on files which you can easily compile.
> For ARM you would need a cross compiler.
>
> >
> > So I cooked up the following patch (probably mangled, just to give you
> > a rough idea of what I did):
> > diff --git a/arch/arm/common/rtctime.c b/arch/arm/common/rtctime.c
> > index bf1075e..19dedb5 100644
> > --- a/arch/arm/common/rtctime.c
> > +++ b/arch/arm/common/rtctime.c
> > @@ -189,13 +189,16 @@ static int rtc_ioctl(struct inode *inode, struct
> > file *file, unsigned int cmd,
> >               if (ret)
> >                       break;
> >               ret = copy_to_user(uarg, &alrm.time, sizeof(tm));
> > -             if (ret)
> > +             if (ret) {
> > +                     unlock_kernel();
> >                       ret = -EFAULT;
>
> In this case it would be better to just put the unlock_kernel() directly
> before the single return. You only need to sprinkle them all over when
> the function has multiple returns.

Understood. As Matthew did in his patch.

> Or then change the flow to only
> have a single return, but that would be slightly advanced.
>
>
> > -             if (ret)
> > +             if (ret) {
> > +                     unlock_kernel();
> >                       ret = -EFAULT;
> > +             }
> >               break;
> >
> >       default:
> > @@ -329,15 +340,18 @@ static int rtc_fasync(int fd, struct file *file, int on)
> >       return fasync_helper(fd, file, on, &rtc_async_queue);
> >  }
> >
> > -static const struct file_operations rtc_fops = {
> > +static long rtc_fioctl(struct file_operations rtc_fops)
> > +{
> > +     lock_kernel();
>
> No the lock_kernel() of course has to be in the function, not in the structure
> definition which does not contain any code.

Yes, understood now. Silly me :-/

> Also the _operations structure stays the same (except for .ioctl -> .unlocked_ioctl);
> only the the ioctl function prototype changes.
>
>
> > Am I'm working in the right direction or should I completely redo the patch?
>
> I would suggest to only work on files that compile. e.g. do a
>
> make allyesconfig
> make -j$[$(grep -c processor /proc/cpuinfo)*2] &1 |tee LOG             (will probably take a long time)
>
> first and then only modify files when are mentioned in "LOG"

Thanks you.

Ciao,
-- 
Paolo
http://paolo.ciarrocchi.googlepages.com/