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

[Michael Hunold]

Hello Christoph,

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

That's ok for me, although I don't know what the exact criteria for 
adding a function to kernel.h are.

>>+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?

These are both fine IMHO.

> 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.

I only copied the function from videodev.c and renamed it, so please 
don't blame me for the way stuff is done there. 8-)

Perhaps Gerd Knorr or Alan Cox can comment on your changes -- I'll have 
to investigate if all of these arguments are used anyway.

>>+	case _IOC_READ: /* some v4l ioctls are marked wrong ... */
> That's crap.  Please move this workaround to v4l not into generic code.

Is it possible to fix the definitons of the v4l ioctls instead without 
breaking binary compatiblity?

>>+	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. 

Since this function is always used in user context and the memory is 
always freed afterwards, it should be possible to use vmalloc() or a 
static buffer (what's the maximum size?) instead.

>>+	/* 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.

Agreed.

CU
Michael.