On Wed, Sep 19, 2012 at 05:57:55AM +0100, Ian Abbott wrote: > On 18/09/12 20:48, Dan Carpenter wrote: > >On Tue, Sep 18, 2012 at 12:39:58PM -0700, Ian Abbott wrote: > >>On 2012-09-18 20:29, Dan Carpenter wrote: > >>>I don't like this at all... I feel like you're taking something > >>>which is so simple and making it so complicated. > >>> > >>>This is only used in an unusual way in do_cmd_ioctl() and > >>>do_cmdtest_ioctl(). It takes literally 5 casts (5 lines of code > >>>and a few comments) to explain what we are doing. Doing the casts > >>>actually increases the readability of the code. > >>> > >>>The whole point is that we should hide these implementation details > >>>from the driver. I don't like the k prefix at all. I don't like > >>>having two huge struct which are virtually identical but except for > >>>the __user tag. We'd have to keep them in sync and that's a > >>>nightmare. > >> > >>I don't think the overall contents of the structure are going to > >>change much as it has to be compatible with existing user-space > >>stuff. So keeping it in sync is mostly a no-op. > >> > >>The convention for ioctl interface structures in the kernel's > >>include/linux/ seems to require the tagging user-space pointers with > >>__user, so I think comedi's ioctl interface structures should follow > >>the same convention. > > > >Adding __user annotations is one way of avoiding bugs, so it's > >better to put them throughout, yes. But if you look at how badly it > >mangles the code then it's not worth it at all. Complicated, messy > >code causes more bugs than anything else. > > I have an alternate suggestion for discussion which keeps the > `_user` tags, avoids the casting, and avoids shadowing the `struct > comedi_cmd`. The suggestion is to avoid using the `chanlist` in > this structure for point to the kernel copy of the channel list in > the same way that we currently avoid using the `data` pointer in > `struct comedi_insn` to point to the kernel copy of the instruction > data. > > This change would involve: > > (a) adding a `chanlist` pointer to `struct comedi_async`; > (b) modifying all the low-level `do_cmd()` handlers to use > `async->chanlist` instead of `async->cmd.chanlist`; > (c) adding a `chanlist` parameter to the low-level `do_cmdtest()` > handlers (and changing them to use it instead of `cmd->chanlist`). > > This would have to be done as one or two massive patches to avoid > intermediate breakage. I don't have strong feeling either way about this. regards, dan carpenter _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
