Re: [PATCH v5 03/18] gfs2: add compat_ioctl support

[Arnd Bergmann]

On Sun, Aug 18, 2019 at 10:17 PM Andreas GrÃnbacher
<andreas.gruenbacher@xxxxxxxxx> wrote:
> Am So., 18. Aug. 2019 um 21:32 Uhr schrieb Arnd Bergmann <arnd@xxxxxxxx>:
> > On Fri, Aug 16, 2019 at 7:32 PM Andreas Gruenbacher <agruenba@xxxxxxxxxx> wrote:
> > > On Wed, Aug 14, 2019 at 10:45 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > > > +       /* These are just misnamed, they actually get/put from/to user an int */
> > > > +       switch(cmd) {
> > > > +       case FS_IOC32_GETFLAGS:
> > > > +               cmd = FS_IOC_GETFLAGS;
> > > > +               break;
> > > > +       case FS_IOC32_SETFLAGS:
> > > > +               cmd = FS_IOC_SETFLAGS;
> > > > +               break;
> > >
> > > I'd like the code to be more explicit here:
> > >
> > >         case FITRIM:
> > >         case FS_IOC_GETFSLABEL:
> > >               break;
> > >         default:
> > >               return -ENOIOCTLCMD;
> >
> > I've looked at it again: if we do this, the function actually becomes
> > longer than the native gfs2_ioctl(). Should we just make a full copy then?
>
> I don't think the length of gfs2_compat_ioctl is really an issue as
> long as the function is that simple.

True. The most important goal should just be to make it easy to
add the correct handler the next time another command is added
to the ioctl function.

Just let me know which version you want for that:

1. my original patch
2. the version from your reply
3. my version below with compat_ptr() added
4. ...

> > static long gfs2_compat_ioctl(struct file *filp, unsigned int cmd,
> > unsigned long arg)
> > {
> >         switch(cmd) {
> >         case FS_IOC32_GETFLAGS:
> >                 return gfs2_get_flags(filp, (u32 __user *)arg);
> >         case FS_IOC32_SETFLAGS:
> >                 return gfs2_set_flags(filp, (u32 __user *)arg);
> >         case FITRIM:
> >                 return gfs2_fitrim(filp, (void __user *)arg);
> >         case FS_IOC_GETFSLABEL:
> >                 return gfs2_getlabel(filp, (char __user *)arg);
> >         }
> >
> >         return -ENOTTY;
> > }
>
> Don't we still need the compat_ptr conversion? That seems to be the
> main point of having a compat_ioctl operation.

Right, of course. Fixed now in my tree.

         Arnd