On Thu, Aug 15, 2019 at 12:26 PM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > > On Thu, Aug 15, 2019 at 01:02:11AM -0700, Christoph Hellwig wrote: > > In many ways I'd actually much rather have a table driven approach. > > Let me try something.. > > Ok, it seems like we don't even need a table containing native and > compat as we can just fall back. The tables still seem nicer to read, > though. > > Let me know what you think of this: > > http://git.infradead.org/users/hch/xfs.git/shortlog/refs/heads/xfs-ioctl-table These all look like useful cleanups, but I'm a little worried about introducing merge conflicts with my own patches. I would want to have my series get merged as a complete branch since each patch that removes a bit of fs/compat_ioctl.c would clash with a patch removing the adjacent bits otherwise. I still haven't heard from Al regarding what he thinks of my v5 series. If he wants me to send a pull request for it, I can of course add in your patches after they are fully reviewed. > I also wonder if we should life the ioctl handler tables to the VFS. The idea of these tables has come up a few times in the past, and there are a couple of subsystems that have something like it, e.g. drivers/media. Usually you'd want to combine the table with a more generic way to do the copy_from_user()/copy_to_user() on the argument, but that in turn requires all commands to be defined correctly (a lot of drivers have some commands that specify the wrong direction or the wrong size, or one that predates the _IO() macro). What I could imaging having in the long run is to have the ioctl table attached to the file_operations structure, and then define it in a way that handles at least the more common variations: - copy_from_user to stack, pass a kernel pointer to handler - a single entry for commands that are 32/64-bit compatible - entries that are only used for native vs compat mode if they have incompatible arguments (this could also be handled by calling in_compat_syscall() in the handler itself). - a flag to specify handlers that require the __user pointer instead of the implied copy. Doing this right will certainly require several revisions of patch series and lots of discussions, and is unrelated to the removal of fs/compat_ioctl.c, so I'd much prefer to get this series merged before we start working on that. Arnd
