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

[Vinod Koul]

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

Only question is in these scenarios this is core corruption so what
would be the apt error code to use?


-- 
~Vinod