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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/dm-devel
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
