Re: [RFC][2.5] generic_usercopy() function (resend, forgot the patches)

Christoph Hellwig (hch@infradead.org)
Fri, 23 May 2003 10:47:22 +0100


This function is small and very useful, I think it should be included unconditional
and the prototype maybe moved to kernel.h.

> +int
> +generic_usercopy(struct inode *inode, struct file *file,
> + unsigned int cmd, unsigned long arg,
> + int (*func)(struct inode *inode, struct file *file,
> + unsigned int cmd, void *arg))

The name is a bit mislead. maybe ioctl_usercopy? ioctl_uaccess?
Also file/inode should go away from the prototype (and the callback).
Only file is needed because inode == file->f_dentry->d_inode, and even
that one should be just some void *data instead.

> + char sbuf[128];
> + void *mbuf = NULL;
> + void *parg = NULL;
> + int err = -EINVAL;
> +
> + /* Copy arguments into temp kernel buffer */
> + switch (_IOC_DIR(cmd)) {
> + case _IOC_NONE:
> + parg = (void *)arg;
> + break;
> + case _IOC_READ: /* some v4l ioctls are marked wrong ... */

That's crap. Please move this workaround to v4l not into generic code.

> + case _IOC_WRITE:
> + case (_IOC_WRITE | _IOC_READ):
> + if (_IOC_SIZE(cmd) <= sizeof(sbuf)) {
> + parg = sbuf;
> + } else {
> + /* too big to allocate from stack */
> + mbuf = kmalloc(_IOC_SIZE(cmd),GFP_KERNEL);
> + if (NULL == mbuf)
> + return -ENOMEM;
> + parg = mbuf;

I wonder whether you should just kmalloc always.

> + /* call driver */
> + err = func(inode, file, cmd, parg);
> + if (err == -ENOIOCTLCMD)
> + err = -EINVAL;

I don't think this is the right place for this substitution - leave it to
the drivers.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/