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.
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti@xxxxxxxxx> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
