Re: [PATCH] [CAIF-RFC 5/8-v2] CAIF Protocol Stack

[Stefano Babic <stefano.babic@babic.homelinux.org>]

sjur.brandeland@stericsson.com wrote:
> From: Sjur Braendeland <sjur.brandeland@stericsson.com>

Hi Sjur,

> diff --git a/net/caif/caif_chr.c b/net/caif/caif_chr.c

> +#define caif_assert(assert) BUG_ON(!(assert))

Do we need special assert for each module (cfglu_assert,
caif_assert,...) ? They are all defined in the same way.
At this point I have already set a comment about using BUG_ON in a
previous patch.

I see a mixed policy in this patch, using sometimes _assert and
sometimes directly BUG_ON, too.

> diff --git a/net/caif/chnl_chr.c b/net/caif/chnl_chr.c

> +	/* Lock in order to try to stop someone from opening the device
> +	   too early. The misc device has its own lock. We cannot take our
> +	   lock until misc_register() is finished, because in open() the
> +	   locks are taken in this order (misc first and then dev).
> +	   So anyone managing to open the device between the misc_register
> +	   and the mutex_lock will get a "device not found" error. Don't
> +	   think it can be avoided.
> +	 */
> +	mutex_lock_interruptible(&dev->mutex);

The return value is not checked and it must be, as in all other cases.

> +	/* Find device from name */
> +	dev = find_device(-1, name, 0);
> +	if (!dev)
> +		return -EBADF;
> +
> +

> +	mutex_lock_interruptible(&dev->mutex);

The return value of mutex_lock_interruptible() must be checked.

Stefano

-- 
stefano <stefano.babic@babic.homelinux.org>
GPG Key: 0x55814DDE
Fingerprint 4E85 2A66 4CBA 497A 2A7B D3BF 5973 F216 5581 4DDE