[PATCH] dm: Add error information printing for dm_register_target()

Yangtao Li posted 1 patch 1 year, 1 month ago
drivers/md/dm-cache-target.c  |  7 ++-----
drivers/md/dm-clone-target.c  | 10 +---------
drivers/md/dm-crypt.c         |  8 +-------
drivers/md/dm-delay.c         | 13 +------------
drivers/md/dm-dust.c          |  7 +------
drivers/md/dm-ebs-target.c    |  7 +------
drivers/md/dm-era-target.c    | 10 +---------
drivers/md/dm-flakey.c        |  7 +------
drivers/md/dm-integrity.c     |  9 +--------
drivers/md/dm-log-writes.c    |  7 +------
drivers/md/dm-mpath.c         |  5 +----
drivers/md/dm-raid1.c         | 16 ++++++----------
drivers/md/dm-snap.c          | 12 +++---------
drivers/md/dm-switch.c        |  8 +-------
drivers/md/dm-target.c        |  7 ++++---
drivers/md/dm-verity-target.c |  8 +-------
drivers/md/dm-writecache.c    | 10 +---------
drivers/md/dm-zero.c          |  7 +------
18 files changed, 29 insertions(+), 129 deletions(-)
[PATCH] dm: Add error information printing for dm_register_target()
Posted by Yangtao Li 1 year, 1 month ago
Ensure that all error handling branches print error information. In this
way, when this function fails, the upper-layer functions can directly
return an error code without missing debugging information. Otherwise,
the error message will be printed redundantly or missing. BTW, remove
redundant printing information.

Signed-off-by: Yangtao Li <frank.li@vivo.com>
---
 drivers/md/dm-cache-target.c  |  7 ++-----
 drivers/md/dm-clone-target.c  | 10 +---------
 drivers/md/dm-crypt.c         |  8 +-------
 drivers/md/dm-delay.c         | 13 +------------
 drivers/md/dm-dust.c          |  7 +------
 drivers/md/dm-ebs-target.c    |  7 +------
 drivers/md/dm-era-target.c    | 10 +---------
 drivers/md/dm-flakey.c        |  7 +------
 drivers/md/dm-integrity.c     |  9 +--------
 drivers/md/dm-log-writes.c    |  7 +------
 drivers/md/dm-mpath.c         |  5 +----
 drivers/md/dm-raid1.c         | 16 ++++++----------
 drivers/md/dm-snap.c          | 12 +++---------
 drivers/md/dm-switch.c        |  8 +-------
 drivers/md/dm-target.c        |  7 ++++---
 drivers/md/dm-verity-target.c |  8 +-------
 drivers/md/dm-writecache.c    | 10 +---------
 drivers/md/dm-zero.c          |  7 +------
 18 files changed, 29 insertions(+), 129 deletions(-)

diff --git a/drivers/md/dm-cache-target.c b/drivers/md/dm-cache-target.c
index dbbcfa580078..dba2d85105f5 100644
--- a/drivers/md/dm-cache-target.c
+++ b/drivers/md/dm-cache-target.c
@@ -3458,13 +3458,10 @@ static int __init dm_cache_init(void)
 		return -ENOMEM;
 
 	r = dm_register_target(&cache_target);
-	if (r) {
-		DMERR("cache target registration failed: %d", r);
+	if (r)
 		kmem_cache_destroy(migration_cache);
-		return r;
-	}
 
-	return 0;
+	return r;
 }
 
 static void __exit dm_cache_exit(void)
diff --git a/drivers/md/dm-clone-target.c b/drivers/md/dm-clone-target.c
index f38a27604c7a..cb0078a7201c 100644
--- a/drivers/md/dm-clone-target.c
+++ b/drivers/md/dm-clone-target.c
@@ -2196,19 +2196,11 @@ static struct target_type clone_target = {
 /* Module functions */
 static int __init dm_clone_init(void)
 {
-	int r;
-
 	_hydration_cache = KMEM_CACHE(dm_clone_region_hydration, 0);
 	if (!_hydration_cache)
 		return -ENOMEM;
 
-	r = dm_register_target(&clone_target);
-	if (r < 0) {
-		DMERR("Failed to register clone target");
-		return r;
-	}
-
-	return 0;
+	return dm_register_target(&clone_target);
 }
 
 static void __exit dm_clone_exit(void)
diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 3ba53dc3cc3f..52615a258e13 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -3662,13 +3662,7 @@ static struct target_type crypt_target = {
 
 static int __init dm_crypt_init(void)
 {
-	int r;
-
-	r = dm_register_target(&crypt_target);
-	if (r < 0)
-		DMERR("register failed %d", r);
-
-	return r;
+	return dm_register_target(&crypt_target);
 }
 
 static void __exit dm_crypt_exit(void)
diff --git a/drivers/md/dm-delay.c b/drivers/md/dm-delay.c
index a425046f88c7..00d18b2070a5 100644
--- a/drivers/md/dm-delay.c
+++ b/drivers/md/dm-delay.c
@@ -370,18 +370,7 @@ static struct target_type delay_target = {
 
 static int __init dm_delay_init(void)
 {
-	int r;
-
-	r = dm_register_target(&delay_target);
-	if (r < 0) {
-		DMERR("register failed %d", r);
-		goto bad_register;
-	}
-
-	return 0;
-
-bad_register:
-	return r;
+	return dm_register_target(&delay_target);
 }
 
 static void __exit dm_delay_exit(void)
diff --git a/drivers/md/dm-dust.c b/drivers/md/dm-dust.c
index 7ae9936752de..9bf3cdf548de 100644
--- a/drivers/md/dm-dust.c
+++ b/drivers/md/dm-dust.c
@@ -573,12 +573,7 @@ static struct target_type dust_target = {
 
 static int __init dm_dust_init(void)
 {
-	int r = dm_register_target(&dust_target);
-
-	if (r < 0)
-		DMERR("dm_register_target failed %d", r);
-
-	return r;
+	return dm_register_target(&dust_target);
 }
 
 static void __exit dm_dust_exit(void)
diff --git a/drivers/md/dm-ebs-target.c b/drivers/md/dm-ebs-target.c
index b1068a68bc46..38da4de3ecbf 100644
--- a/drivers/md/dm-ebs-target.c
+++ b/drivers/md/dm-ebs-target.c
@@ -455,12 +455,7 @@ static struct target_type ebs_target = {
 
 static int __init dm_ebs_init(void)
 {
-	int r = dm_register_target(&ebs_target);
-
-	if (r < 0)
-		DMERR("register failed %d", r);
-
-	return r;
+	return dm_register_target(&ebs_target);
 }
 
 static void dm_ebs_exit(void)
diff --git a/drivers/md/dm-era-target.c b/drivers/md/dm-era-target.c
index c2e7780cdd2d..554d234fca18 100644
--- a/drivers/md/dm-era-target.c
+++ b/drivers/md/dm-era-target.c
@@ -1756,15 +1756,7 @@ static struct target_type era_target = {
 
 static int __init dm_era_init(void)
 {
-	int r;
-
-	r = dm_register_target(&era_target);
-	if (r) {
-		DMERR("era target registration failed: %d", r);
-		return r;
-	}
-
-	return 0;
+	return dm_register_target(&era_target);
 }
 
 static void __exit dm_era_exit(void)
diff --git a/drivers/md/dm-flakey.c b/drivers/md/dm-flakey.c
index 5b7556d2a9d9..14179355e6a1 100644
--- a/drivers/md/dm-flakey.c
+++ b/drivers/md/dm-flakey.c
@@ -509,12 +509,7 @@ static struct target_type flakey_target = {
 
 static int __init dm_flakey_init(void)
 {
-	int r = dm_register_target(&flakey_target);
-
-	if (r < 0)
-		DMERR("register failed %d", r);
-
-	return r;
+	return dm_register_target(&flakey_target);
 }
 
 static void __exit dm_flakey_exit(void)
diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index b0d5057fbdd9..b99c3f98412b 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -4693,8 +4693,6 @@ static struct target_type integrity_target = {
 
 static int __init dm_integrity_init(void)
 {
-	int r;
-
 	journal_io_cache = kmem_cache_create("integrity_journal_io",
 					     sizeof(struct journal_io), 0, 0, NULL);
 	if (!journal_io_cache) {
@@ -4702,12 +4700,7 @@ static int __init dm_integrity_init(void)
 		return -ENOMEM;
 	}
 
-	r = dm_register_target(&integrity_target);
-
-	if (r < 0)
-		DMERR("register failed %d", r);
-
-	return r;
+	return dm_register_target(&integrity_target);
 }
 
 static void __exit dm_integrity_exit(void)
diff --git a/drivers/md/dm-log-writes.c b/drivers/md/dm-log-writes.c
index cbd0f81f4a35..0ce9b01d1393 100644
--- a/drivers/md/dm-log-writes.c
+++ b/drivers/md/dm-log-writes.c
@@ -940,12 +940,7 @@ static struct target_type log_writes_target = {
 
 static int __init dm_log_writes_init(void)
 {
-	int r = dm_register_target(&log_writes_target);
-
-	if (r < 0)
-		DMERR("register failed %d", r);
-
-	return r;
+	return dm_register_target(&log_writes_target);
 }
 
 static void __exit dm_log_writes_exit(void)
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index 61ab1a8d2c9c..bea3cda9938e 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -2235,11 +2235,8 @@ static int __init dm_multipath_init(void)
 	}
 
 	r = dm_register_target(&multipath_target);
-	if (r < 0) {
-		DMERR("request-based register failed %d", r);
-		r = -EINVAL;
+	if (r < 0)
 		goto bad_register_target;
-	}
 
 	return 0;
 
diff --git a/drivers/md/dm-raid1.c b/drivers/md/dm-raid1.c
index bc417a5e5b89..82430b8dedfa 100644
--- a/drivers/md/dm-raid1.c
+++ b/drivers/md/dm-raid1.c
@@ -1498,22 +1498,18 @@ static struct target_type mirror_target = {
 
 static int __init dm_mirror_init(void)
 {
-	int r = -ENOMEM;
+	int r;
 
 	dm_raid1_wq = alloc_workqueue("dm_raid1_wq", 0, 0);
-	if (!dm_raid1_wq)
-		goto bad_target;
+	if (!dm_raid1_wq) {
+		DMERR("Failed to alloc workqueue");
+		return -ENOMEM;
+	}
 
 	r = dm_register_target(&mirror_target);
-	if (r < 0) {
+	if (r < 0)
 		destroy_workqueue(dm_raid1_wq);
-		goto bad_target;
-	}
-
-	return 0;
 
-bad_target:
-	DMERR("Failed to register mirror target");
 	return r;
 }
 
diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index f766c21408f1..9c49f53760d0 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -2815,22 +2815,16 @@ static int __init dm_snapshot_init(void)
 	}
 
 	r = dm_register_target(&snapshot_target);
-	if (r < 0) {
-		DMERR("snapshot target register failed %d", r);
+	if (r < 0)
 		goto bad_register_snapshot_target;
-	}
 
 	r = dm_register_target(&origin_target);
-	if (r < 0) {
-		DMERR("Origin target register failed %d", r);
+	if (r < 0)
 		goto bad_register_origin_target;
-	}
 
 	r = dm_register_target(&merge_target);
-	if (r < 0) {
-		DMERR("Merge target register failed %d", r);
+	if (r < 0)
 		goto bad_register_merge_target;
-	}
 
 	return 0;
 
diff --git a/drivers/md/dm-switch.c b/drivers/md/dm-switch.c
index ee2432927e90..5a5976b0cfb8 100644
--- a/drivers/md/dm-switch.c
+++ b/drivers/md/dm-switch.c
@@ -568,13 +568,7 @@ static struct target_type switch_target = {
 
 static int __init dm_switch_init(void)
 {
-	int r;
-
-	r = dm_register_target(&switch_target);
-	if (r < 0)
-		DMERR("dm_register_target() failed %d", r);
-
-	return r;
+	return dm_register_target(&switch_target);
 }
 
 static void __exit dm_switch_exit(void)
diff --git a/drivers/md/dm-target.c b/drivers/md/dm-target.c
index 26ea22b1a0d7..2653a1c5e084 100644
--- a/drivers/md/dm-target.c
+++ b/drivers/md/dm-target.c
@@ -85,11 +85,12 @@ int dm_register_target(struct target_type *tt)
 	int rv = 0;
 
 	down_write(&_lock);
-	if (__find_target_type(tt->name))
+	if (__find_target_type(tt->name)) {
 		rv = -EEXIST;
-	else
+		DMERR("can't find dm-%s target type", tt->name);
+	} else {
 		list_add(&tt->list, &_targets);
-
+	}
 	up_write(&_lock);
 	return rv;
 }
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index ade83ef3b439..1eed2a9b4e8e 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -1501,13 +1501,7 @@ static struct target_type verity_target = {
 
 static int __init dm_verity_init(void)
 {
-	int r;
-
-	r = dm_register_target(&verity_target);
-	if (r < 0)
-		DMERR("register failed %d", r);
-
-	return r;
+	return dm_register_target(&verity_target);
 }
 
 static void __exit dm_verity_exit(void)
diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
index 3aa5874f0aef..81b60b75a9fa 100644
--- a/drivers/md/dm-writecache.c
+++ b/drivers/md/dm-writecache.c
@@ -2776,15 +2776,7 @@ static struct target_type writecache_target = {
 
 static int __init dm_writecache_init(void)
 {
-	int r;
-
-	r = dm_register_target(&writecache_target);
-	if (r < 0) {
-		DMERR("register failed %d", r);
-		return r;
-	}
-
-	return 0;
+	return dm_register_target(&writecache_target);
 }
 
 static void __exit dm_writecache_exit(void)
diff --git a/drivers/md/dm-zero.c b/drivers/md/dm-zero.c
index 2601cd520384..4d96a9d50894 100644
--- a/drivers/md/dm-zero.c
+++ b/drivers/md/dm-zero.c
@@ -68,12 +68,7 @@ static struct target_type zero_target = {
 
 static int __init dm_zero_init(void)
 {
-	int r = dm_register_target(&zero_target);
-
-	if (r < 0)
-		DMERR("register failed %d", r);
-
-	return r;
+	return dm_register_target(&zero_target);
 }
 
 static void __exit dm_zero_exit(void)
-- 
2.35.1
Re: dm: Add error information printing for dm_register_target()
Posted by Mike Snitzer 1 year ago
On Sat, Mar 18 2023 at  9:16P -0400,
Yangtao Li <frank.li@vivo.com> wrote:

> Ensure that all error handling branches print error information. In this
> way, when this function fails, the upper-layer functions can directly
> return an error code without missing debugging information. Otherwise,
> the error message will be printed redundantly or missing. BTW, remove
> redundant printing information.
> 
> Signed-off-by: Yangtao Li <frank.li@vivo.com>

Hi,

I've picked this up with a few changes (your patch caused me to look
closer at some of the target code and we had some missed cleanup):
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.4&id=6827af4a9a9f5bb664c42abf7c11af4978d72201
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.4&id=6b79a428c02769f2a11f8ae76bf866226d134887

So I rebased your patch and tweaked it slightly (and splitting it), see:
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.4&id=e6c908b5d86faf3dbcf314b1c07c342268e32def
https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.4&id=2a455a0b418f972d61b68f9321b7d1892c16b4f7

Thanks,
Mike