diff mbox series

[21/21] libmultipath: adapt to new semantics of dm_get_uuid()

Message ID 20230901180235.23980-22-mwilck@suse.com (mailing list archive)
State Not Applicable, archived
Delegated to: christophe varoqui
Headers show
Series multipath-tools: user-friendly names rework | expand

Commit Message

Martin Wilck Sept. 1, 2023, 6:02 p.m. UTC 
From: Martin Wilck <mwilck@suse.com>

dm_get_uuid() will return 1 for non-existing maps. Thus we don't need
to call dm_map_present() any more in alias_already_taken(). This changes
our semantics: previously we'd avoid using an alias for which dm_get_uuid()
had failed. Now we treat failure in dm_get_uuid() as indication that the
map doesn't exist. This is not dangerous because dm_task_get_uuid() cannot
fail, and thus the modified dm_get_uuid() will fail if and only if
dm_map_present() would return false.

This makes the "failed alias" test mostly obsolete, as "failed" is now
treated as "unused".

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/alias.c | 23 ++++++++++++-----------
 tests/alias.c        | 30 ++++++------------------------
 2 files changed, 18 insertions(+), 35 deletions(-)

Comments

Benjamin Marzinski Sept. 6, 2023, 10:47 p.m. UTC | #1 
On Fri, Sep 01, 2023 at 08:02:34PM +0200, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> dm_get_uuid() will return 1 for non-existing maps. Thus we don't need
> to call dm_map_present() any more in alias_already_taken(). This changes
> our semantics: previously we'd avoid using an alias for which dm_get_uuid()
> had failed. Now we treat failure in dm_get_uuid() as indication that the
> map doesn't exist. This is not dangerous because dm_task_get_uuid() cannot
> fail, and thus the modified dm_get_uuid() will fail if and only if
> dm_map_present() would return false.
> 
> This makes the "failed alias" test mostly obsolete, as "failed" is now
> treated as "unused".
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/alias.c | 23 ++++++++++++-----------
>  tests/alias.c        | 30 ++++++------------------------
>  2 files changed, 18 insertions(+), 35 deletions(-)
> 
> diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> index 6003df0..0f5af17 100644
> --- a/libmultipath/alias.c
> +++ b/libmultipath/alias.c
> @@ -295,18 +295,19 @@ scan_devname(const char *alias, const char *prefix)
>  static bool alias_already_taken(const char *alias, const char *map_wwid)
>  {
>  
> -	if (dm_map_present(alias)) {
> -		char wwid[WWID_SIZE];
> +	char wwid[WWID_SIZE];
>  
> -		/* If both the name and the wwid match, then it's fine.*/
> -		if (dm_get_uuid(alias, wwid, sizeof(wwid)) == 0 &&
> -		    strncmp(map_wwid, wwid, sizeof(wwid)) == 0)
> -			return false;
> -		condlog(3, "%s: alias '%s' already taken, reselecting alias",
> -			map_wwid, alias);
> -		return true;
> -	}
> -	return false;
> +	/* If the map doesn't exist, it's fine */
> +	if (dm_get_uuid(alias, wwid, sizeof(wwid)) != 0)
> +		return false;
> +
> +	/* If both the name and the wwid match, it's fine.*/
> +	if (strncmp(map_wwid, wwid, sizeof(wwid)) == 0)
> +		return false;
> +
> +	condlog(3, "%s: alias '%s' already taken, reselecting alias",
> +		map_wwid, alias);
> +	return true;
>  }
>  
>  static bool id_already_taken(int id, const char *prefix, const char *map_wwid)
> diff --git a/tests/alias.c b/tests/alias.c
> index 4ac6425..4f54031 100644
> --- a/tests/alias.c
> +++ b/tests/alias.c
> @@ -73,12 +73,6 @@ int __wrap_mkstemp(char *template)
>  	return 10;
>  }
>  
> -int __wrap_dm_map_present(const char * str)
> -{
> -	check_expected(str);
> -	return mock_type(int);
> -}
> -
>  int __wrap_dm_get_uuid(const char *name, char *uuid, int uuid_len)
>  {
>  	int ret;
> @@ -398,14 +392,13 @@ static int test_scan_devname(void)
>  
>  static void mock_unused_alias(const char *alias)
>  {
> -	expect_string(__wrap_dm_map_present, str, alias);
> -	will_return(__wrap_dm_map_present, 0);
> +	expect_string(__wrap_dm_get_uuid, name, alias);
> +	expect_value(__wrap_dm_get_uuid, uuid_len, WWID_SIZE);
> +	will_return(__wrap_dm_get_uuid, 1);
>  }
>  
>  static void mock_self_alias(const char *alias, const char *wwid)
>  {
> -	expect_string(__wrap_dm_map_present, str, alias);
> -	will_return(__wrap_dm_map_present, 1);
>  	expect_string(__wrap_dm_get_uuid, name, alias);
>  	expect_value(__wrap_dm_get_uuid, uuid_len, WWID_SIZE);
>  	will_return(__wrap_dm_get_uuid, 0);
> @@ -432,18 +425,13 @@ static void mock_self_alias(const char *alias, const char *wwid)
>  
>  #define mock_failed_alias(alias, wwid)					\
>  	do {								\
> -		expect_string(__wrap_dm_map_present, str, alias);	\
> -		will_return(__wrap_dm_map_present, 1);			\
>  		expect_string(__wrap_dm_get_uuid, name, alias);		\
>  		expect_value(__wrap_dm_get_uuid, uuid_len, WWID_SIZE);	\
>  		will_return(__wrap_dm_get_uuid, 1);			\
> -		expect_condlog(3, USED_STR(alias, wwid));		\
>  	} while (0)
>  
>  #define mock_used_alias(alias, wwid)					\
>  	do {								\
> -		expect_string(__wrap_dm_map_present, str, alias);	\
> -		will_return(__wrap_dm_map_present, 1);			\
>  		expect_string(__wrap_dm_get_uuid, name, alias);		\
>  		expect_value(__wrap_dm_get_uuid, uuid_len, WWID_SIZE);	\
>  		will_return(__wrap_dm_get_uuid, 0);			\
> @@ -566,9 +554,8 @@ static void lb_empty_failed(void **state)
>  	mock_bindings_file("");
>  	expect_condlog(3, NOMATCH_WWID_STR("WWID0"));
>  	mock_failed_alias("MPATHa", "WWID0");
> -	mock_unused_alias("MPATHb");
>  	rc = lookup_binding(NULL, "WWID0", &alias, "MPATH", 1);
> -	assert_int_equal(rc, 2);
> +	assert_int_equal(rc, 1);
>  	assert_ptr_equal(alias, NULL);
>  	free(alias);
>  }
> @@ -666,9 +653,8 @@ static void lb_nomatch_a_3_used_failed_self(void **state)
>  	mock_used_alias("MPATHc", "WWID1");
>  	mock_used_alias("MPATHd", "WWID1");
>  	mock_failed_alias("MPATHe", "WWID1");
> -	mock_self_alias("MPATHf", "WWID1");
>  	rc = lookup_binding(NULL, "WWID1", &alias, "MPATH", 1);
> -	assert_int_equal(rc, 6);
> +	assert_int_equal(rc, 5);
>  	assert_ptr_equal(alias, NULL);
>  }
>  
> @@ -949,11 +935,7 @@ static void lb_nomatch_b_a_aa_zz(void **state)
>  	 * lookup_binding finds MPATHaaa as next free entry, because MPATHaa is
>  	 * found before MPATHb, and MPATHzz was in the bindings, too.
>  	 */
> -	for (i = 0; i <= 26; i++) {
> -		print_strbuf(&buf,  "MPATH");
> -		format_devname(&buf, i + 1);
> -		print_strbuf(&buf,  " WWID%d\n", i);
> -	}
> +	fill_bindings(&buf, 0, 26);
>  	print_strbuf(&buf, "MPATHzz WWID676\n");
>  	mock_bindings_file(get_strbuf_str(&buf));
>  	expect_condlog(3, NOMATCH_WWID_STR("WWID703"));
> -- 
> 2.41.0
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
diff mbox series

Patch

diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index 6003df0..0f5af17 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -295,18 +295,19 @@  scan_devname(const char *alias, const char *prefix)
 static bool alias_already_taken(const char *alias, const char *map_wwid)
 {
 
-	if (dm_map_present(alias)) {
-		char wwid[WWID_SIZE];
+	char wwid[WWID_SIZE];
 
-		/* If both the name and the wwid match, then it's fine.*/
-		if (dm_get_uuid(alias, wwid, sizeof(wwid)) == 0 &&
-		    strncmp(map_wwid, wwid, sizeof(wwid)) == 0)
-			return false;
-		condlog(3, "%s: alias '%s' already taken, reselecting alias",
-			map_wwid, alias);
-		return true;
-	}
-	return false;
+	/* If the map doesn't exist, it's fine */
+	if (dm_get_uuid(alias, wwid, sizeof(wwid)) != 0)
+		return false;
+
+	/* If both the name and the wwid match, it's fine.*/
+	if (strncmp(map_wwid, wwid, sizeof(wwid)) == 0)
+		return false;
+
+	condlog(3, "%s: alias '%s' already taken, reselecting alias",
+		map_wwid, alias);
+	return true;
 }
 
 static bool id_already_taken(int id, const char *prefix, const char *map_wwid)
diff --git a/tests/alias.c b/tests/alias.c
index 4ac6425..4f54031 100644
--- a/tests/alias.c
+++ b/tests/alias.c
@@ -73,12 +73,6 @@  int __wrap_mkstemp(char *template)
 	return 10;
 }
 
-int __wrap_dm_map_present(const char * str)
-{
-	check_expected(str);
-	return mock_type(int);
-}
-
 int __wrap_dm_get_uuid(const char *name, char *uuid, int uuid_len)
 {
 	int ret;
@@ -398,14 +392,13 @@  static int test_scan_devname(void)
 
 static void mock_unused_alias(const char *alias)
 {
-	expect_string(__wrap_dm_map_present, str, alias);
-	will_return(__wrap_dm_map_present, 0);
+	expect_string(__wrap_dm_get_uuid, name, alias);
+	expect_value(__wrap_dm_get_uuid, uuid_len, WWID_SIZE);
+	will_return(__wrap_dm_get_uuid, 1);
 }
 
 static void mock_self_alias(const char *alias, const char *wwid)
 {
-	expect_string(__wrap_dm_map_present, str, alias);
-	will_return(__wrap_dm_map_present, 1);
 	expect_string(__wrap_dm_get_uuid, name, alias);
 	expect_value(__wrap_dm_get_uuid, uuid_len, WWID_SIZE);
 	will_return(__wrap_dm_get_uuid, 0);
@@ -432,18 +425,13 @@  static void mock_self_alias(const char *alias, const char *wwid)
 
 #define mock_failed_alias(alias, wwid)					\
 	do {								\
-		expect_string(__wrap_dm_map_present, str, alias);	\
-		will_return(__wrap_dm_map_present, 1);			\
 		expect_string(__wrap_dm_get_uuid, name, alias);		\
 		expect_value(__wrap_dm_get_uuid, uuid_len, WWID_SIZE);	\
 		will_return(__wrap_dm_get_uuid, 1);			\
-		expect_condlog(3, USED_STR(alias, wwid));		\
 	} while (0)
 
 #define mock_used_alias(alias, wwid)					\
 	do {								\
-		expect_string(__wrap_dm_map_present, str, alias);	\
-		will_return(__wrap_dm_map_present, 1);			\
 		expect_string(__wrap_dm_get_uuid, name, alias);		\
 		expect_value(__wrap_dm_get_uuid, uuid_len, WWID_SIZE);	\
 		will_return(__wrap_dm_get_uuid, 0);			\
@@ -566,9 +554,8 @@  static void lb_empty_failed(void **state)
 	mock_bindings_file("");
 	expect_condlog(3, NOMATCH_WWID_STR("WWID0"));
 	mock_failed_alias("MPATHa", "WWID0");
-	mock_unused_alias("MPATHb");
 	rc = lookup_binding(NULL, "WWID0", &alias, "MPATH", 1);
-	assert_int_equal(rc, 2);
+	assert_int_equal(rc, 1);
 	assert_ptr_equal(alias, NULL);
 	free(alias);
 }
@@ -666,9 +653,8 @@  static void lb_nomatch_a_3_used_failed_self(void **state)
 	mock_used_alias("MPATHc", "WWID1");
 	mock_used_alias("MPATHd", "WWID1");
 	mock_failed_alias("MPATHe", "WWID1");
-	mock_self_alias("MPATHf", "WWID1");
 	rc = lookup_binding(NULL, "WWID1", &alias, "MPATH", 1);
-	assert_int_equal(rc, 6);
+	assert_int_equal(rc, 5);
 	assert_ptr_equal(alias, NULL);
 }
 
@@ -949,11 +935,7 @@  static void lb_nomatch_b_a_aa_zz(void **state)
 	 * lookup_binding finds MPATHaaa as next free entry, because MPATHaa is
 	 * found before MPATHb, and MPATHzz was in the bindings, too.
 	 */
-	for (i = 0; i <= 26; i++) {
-		print_strbuf(&buf,  "MPATH");
-		format_devname(&buf, i + 1);
-		print_strbuf(&buf,  " WWID%d\n", i);
-	}
+	fill_bindings(&buf, 0, 26);
 	print_strbuf(&buf, "MPATHzz WWID676\n");
 	mock_bindings_file(get_strbuf_str(&buf));
 	expect_condlog(3, NOMATCH_WWID_STR("WWID703"));