Re: [patch] dm cache: fix up IS_ERR vs NULL confusion

[Joe Thornber <thornber@redhat.com>]

Dan,

Thanks.  I hate errptrs so my natural inclination is just return NULLs.  I'll fix up and retest.

- Joe

On Wed, Jan 28, 2015 at 09:46:09AM +0300, Dan Carpenter wrote:
> It used to be that this code used ERR_PTRs consistently, but a recent
> change mixed it up.  The code sometimes returns NULL, sometimes an
> ERR_PTR, some code assumes everything is an ERR_PTR and some assumes it
> returns NULL on error.  I've changed it back so that now everything is
> an ERR_PTR again.
> 
> These new bugs were caught by static checking:
> 
> drivers/md/dm-cache-target.c:2409 cache_create() warn: 'cmd' isn't an ERR_PTR
> drivers/md/dm-cache-metadata.c:754 lookup_or_open() error: 'cmd' dereferencing possible ERR_PTR()
> drivers/md/dm-cache-metadata.c:757 lookup_or_open() error: 'cmd' dereferencing possible ERR_PTR()
> 
> I also reversed some tests for failure so that it was more clear and had
> fewer indent levels.
> 
> Fixes: 9b1cc9f251af ('dm cache: share cache-metadata object across inactive and active DM tables')
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> The one question I have is that I made dm_cache_metadata_open() either
> preserve the error code or return -EINVAL.  I haven't tested this so I
> guess review carefully.  This patch should probably be back ported to
> stable or folded into Joe's.
> 
> diff --git a/drivers/md/dm-cache-metadata.c b/drivers/md/dm-cache-metadata.c
> index 21b1562..3b08cdf 100644
> --- a/drivers/md/dm-cache-metadata.c
> +++ b/drivers/md/dm-cache-metadata.c
> @@ -681,10 +681,8 @@ static struct dm_cache_metadata *metadata_open(struct block_device *bdev,
>  	struct dm_cache_metadata *cmd;
>  
>  	cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
> -	if (!cmd) {
> -		DMERR("could not allocate metadata struct");
> -		return NULL;
> -	}
> +	if (!cmd)
> +		return ERR_PTR(-ENOMEM);
>  
>  	atomic_set(&cmd->ref_count, 1);
>  	init_rwsem(&cmd->root_lock);
> @@ -745,18 +743,19 @@ static struct dm_cache_metadata *lookup_or_open(struct block_device *bdev,
>  		return cmd;
>  
>  	cmd = metadata_open(bdev, data_block_size, may_format_device, policy_hint_size);
> -	if (cmd) {
> -		mutex_lock(&table_lock);
> -		cmd2 = lookup(bdev);
> -		if (cmd2) {
> -			mutex_unlock(&table_lock);
> -			__destroy_persistent_data_objects(cmd);
> -			kfree(cmd);
> -			return cmd2;
> -		}
> -		list_add(&cmd->list, &table);
> +	if (IS_ERR(cmd))
> +		return cmd;
> +
> +	mutex_lock(&table_lock);
> +	cmd2 = lookup(bdev);
> +	if (cmd2) {
>  		mutex_unlock(&table_lock);
> +		__destroy_persistent_data_objects(cmd);
> +		kfree(cmd);
> +		return cmd2;
>  	}
> +	list_add(&cmd->list, &table);
> +	mutex_unlock(&table_lock);
>  
>  	return cmd;
>  }
> @@ -778,11 +777,16 @@ struct dm_cache_metadata *dm_cache_metadata_open(struct block_device *bdev,
>  						 bool may_format_device,
>  						 size_t policy_hint_size)
>  {
> -	struct dm_cache_metadata *cmd = lookup_or_open(bdev, data_block_size,
> -						       may_format_device, policy_hint_size);
> -	if (cmd && !same_params(cmd, data_block_size)) {
> +	struct dm_cache_metadata *cmd;
> +
> +	cmd = lookup_or_open(bdev, data_block_size,
> +			     may_format_device, policy_hint_size);
> +	if (IS_ERR(cmd))
> +		return cmd;
> +
> +	if (!same_params(cmd, data_block_size)) {
>  		dm_cache_metadata_close(cmd);
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	return cmd;
> 
> --
> dm-devel mailing list
> dm-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/dm-devel