[lkml]   [2018]   [Sep]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCHES] tty ioctls cleanups, compat and not only
On Fri, Sep 14, 2018 at 10:21:53AM +0200, Arnd Bergmann wrote:

> This does sound very appealing, but there is a small downside:
> The difference between ".compat_ioctl = NULL" and
> ".compat_ioctl=native_ioctl" is now very subtle, and I wouldn't
> necessarily expect casual readers to understand that.


One is "all non-generics take pointers to wordsize-neutral objects",
another "all non-generics take integers".

That solution certainly needs to be documented more than just in commit
message, though; IMO the method descriptions next to declaration are the
best place for that. Will update...

> If we go with my file_operations patch for generic_compat_ioctl_ptrarg
> and add generic_compat_ioctl_intarg, we can do the same thing here
> with ldisc_compat_ioctl_ptrarg/ldisc_compat_ioctl_intarg to make it
> a little more consistent with fops and self-documenting.

No, we can't - ldisc ->ioctl() (or ->compat_ioctl()) doesn't get ldisc
in arguments. Besides, indirect calls are costly these days...

> + if (retval == -ENOIOCTLCMD && _IOC_TYPE(cmd) == 'T') {
> + retval = tty_ioctl(file, cmd, (unsigned long)compat_ptr(arg));
> + WARN_ON_ONCE(retval != -ENOIOCTLCMD && retval != -ENOTTY);
> + }
> Seeing that you list every single 'T' command in tty_compat_ioctl()
> that we handle in the native case, that WARN_ON_ONCE should not
> trigger for any input, but it would catch (and warn about) any of those
> that might get added in the future to the native code path without the
> compat entry.

Anything adding new ioctls needs careful review anyway. And blind bets upon
that stuff taking compat pointer are, AFAICS, completely unfounded.

 \ /
  Last update: 2018-09-14 17:11    [W:0.113 / U:13.432 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site