[alsa-devel] [PATCH v4 5/6] core: add support for compress_offload

[Takashi Iwai]

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.


Takashi