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

[Takashi Iwai]

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