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

[Dan Carpenter]

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 at 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;