[PATCH 01/50] staging: comedi: prepare for new struct comedi_kcmd

[Dan Carpenter]

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