At Thu, 22 Dec 2011 14:01:46 +0530, Vinod Koul wrote:
On Thu, 2011-12-22 at 09:27 +0100, Takashi Iwai wrote:
At Thu, 22 Dec 2011 13:49:50 +0530, Vinod Koul wrote:
On Thu, 2011-12-22 at 08:55 +0100, Takashi Iwai wrote:
At Tue, 13 Dec 2011 14:32:59 +0530, Vinod Koul wrote:
+static int snd_compr_free(struct inode *inode, struct file *f) +{
- struct snd_compr_file *data = f->private_data;
- data->stream.ops->free(&data->stream);
Better to add a NULL check.
Not really. open, free and set_params callbacks are mandatory and there is BUG_ON for them so these wont be checked here
OK, but don't use BUG_ON().
+static int +snd_compr_ioctl_avail(struct snd_compr_stream *stream, unsigned long arg) +{
- struct snd_compr_avail ioctl_avail;
- size_t avail;
- avail = snd_compr_calc_avail(stream, &ioctl_avail);
- ioctl_avail.avail = avail;
- if (copy_to_user((unsigned long __user *)arg,
&ioctl_avail, sizeof(ioctl_avail)))Use a correct cast.
+static ssize_t snd_compr_write(struct file *f, const char __user *buf,
size_t count, loff_t *offset)+{
- struct snd_compr_file *data = f->private_data;
- struct snd_compr_stream *stream;
- size_t avail;
- int retval;
- BUG_ON(!data);
Avoid BUG_ON(). This is only for really critical bugs for which you must stop the whole machine. (BTW, snd_BUG_ON() is expanded to WARN_ON(), so it's not equivalent with BUG_ON().)
Well write can be called only after open. And open should have set the data. If data is null that means something really horrible happened (not at all possible unless corruption) so BUG_ON :)
Nah, do you really want to stop the machine here completely? It means that you can't debug it any more. If it's just a NULL-dereference, even a normal Oops would be better than this BUG_ON().
Again, BUG_ON() is only for the critical bug that blocks any further operations of the whole machine, e.g. when there is a clear risk of immediate data corruption, etc. A bug like this in a driver code can't belong to such category.
Got the point :) Will move probe BUG_ON to return error
You can use WARN() or snd_BUG_ON().
Only question is in these scenarios this is core corruption so what would be the apt error code to use?
Well, there is no standard definition. Just use a sensible errno that will abort the user-space appropriately (i.e. not like EPIPE). Even -EFAULT would be OK, I think.
Takashi