[dm-devel] [PATCH 0/2] libmutipath: only give out free user_friendly_names

[dm-devel] [PATCH 0/2] libmutipath: only give out free user_friendly_names

[Benjamin Marzinski]

This patchset fixes a bug that we're run into on installation. Right
now, multipath doesn't ever check that the user_friendly_name it
selected is actually not in-use.  Normally, this is fine, since it won't
pick a name without first adding it to the bindings file.  However, if
the device was created in the initramfs, the bindings file in the
regular filesystem won't be updated until multipathd processes those
paths when starting up after the switchroot. If new path devices are
discovered when multipath starts up after the switchroot, they could be
processed first, and pick the already in use user_friendly_name. In this
case multipath will fail back to using the WWID for the new device, and
renaming the old device.  Later, if multipathd is reconfigured, the new
device will get renamed to the previously in-use user_friendly_name it
added to the bindings file.  All this renaming of devices is messing
with the installer.

To deal with this, this patchset checks that the selected alias is not
in use by an existing dm device, before writing it to the bindings file.
If it is, multipath just steps through the free aliases in the bindings
file, checking until it finds a one that's not in use. I've updated the
lookup_binding() tests to test this functionality as well.

Benjamin Marzinski (2):
  libmultipath: check if user_friendly_name is in use
  tests: add tests for checking if alias is in use

 libmultipath/alias.c |  48 ++++-
 tests/alias.c        | 427 +++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 459 insertions(+), 16 deletions(-)

-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 1/2] libmultipath: check if user_friendly_name is in use

[Benjamin Marzinski]

If there are multipath devices that have user_friendly_names but do not
have their bindings in the bindings_file, get_user_friendly_alias() can
currently give out those names again. This can result in an incorrect
entry in the bindings file, and a device that gets created with a WWID
alias instead of a user_friendly_name. This situation can happen after
the pivot root, if a multipath device is created in the initramfs.  If
this device doesn't have a binding in the regular filesystem
bindings_file and a new multipath device is created before it can add
its binding, the new device can steal that user_friendly_name during
multipathd's initial configure.

To solve this, get_user_friendly_alias() now calls lookup_binding() with
a new paramter, telling it to check if the id it found is already in use
by a diffent device. If so, lookup_binding() will continue to check open
ids, until it finds one that it not currently in use by a dm device.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/alias.c | 48 +++++++++++++++++++++++++++++++++++++++++---
 tests/alias.c        | 22 ++++++++++----------
 2 files changed, 56 insertions(+), 14 deletions(-)

diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index a7ba485a..02bc9d65 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -21,6 +21,7 @@
 #include "config.h"
 #include "util.h"
 #include "errno.h"
+#include "devmapper.h"
 
 
 /*
@@ -119,6 +120,28 @@ scan_devname(const char *alias, const char *prefix)
 	return n;
 }
 
+static int
+id_already_taken(int id, const char *prefix, const char *map_wwid)
+{
+	char alias[LINE_MAX];
+
+	if (format_devname(alias, id, LINE_MAX, prefix) < 0)
+		return 0;
+
+	if (dm_map_present(alias)) {
+		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 0;
+		condlog(3, "%s: alias '%s' already taken, but not in bindings file. reselecting alias", map_wwid, alias);
+		return 1;
+	}
+	return 0;
+}
+
+
 /*
  * Returns: 0   if matching entry in WWIDs file found
  *         -1   if an error occurs
@@ -128,7 +151,7 @@ scan_devname(const char *alias, const char *prefix)
  */
 static int
 lookup_binding(FILE *f, const char *map_wwid, char **map_alias,
-	       const char *prefix)
+	       const char *prefix, int check_if_taken)
 {
 	char buf[LINE_MAX];
 	unsigned int line_nr = 0;
@@ -183,12 +206,31 @@ lookup_binding(FILE *f, const char *map_wwid, char **map_alias,
 			return 0;
 		}
 	}
+	if (!prefix && check_if_taken)
+		id = -1;
 	if (id >= smallest_bigger_id) {
 		if (biggest_id < INT_MAX)
 			id = biggest_id + 1;
 		else
 			id = -1;
 	}
+	if (id > 0 && check_if_taken) {
+		while(id_already_taken(id, prefix, map_wwid)) {
+			if (id == INT_MAX) {
+				id = -1;
+				break;
+			}
+			id++;
+			if (id == smallest_bigger_id) {
+				if (biggest_id == INT_MAX) {
+					id = -1;
+					break;
+				}
+				if (biggest_id >= smallest_bigger_id)
+					id = biggest_id + 1;
+			}
+		}
+	}
 	if (id < 0) {
 		condlog(0, "no more available user_friendly_names");
 		return -1;
@@ -331,7 +373,7 @@ use_existing_alias (const char *wwid, const char *file, const char *alias_old,
 		goto out;
 	}
 
-	id = lookup_binding(f, wwid, &alias, NULL);
+	id = lookup_binding(f, wwid, &alias, NULL, 0);
 	if (alias) {
 		condlog(3, "Use existing binding [%s] for WWID [%s]",
 			alias, wwid);
@@ -388,7 +430,7 @@ get_user_friendly_alias(const char *wwid, const char *file, const char *prefix,
 		return NULL;
 	}
 
-	id = lookup_binding(f, wwid, &alias, prefix);
+	id = lookup_binding(f, wwid, &alias, prefix, 1);
 	if (id < 0) {
 		fclose(f);
 		return NULL;
diff --git a/tests/alias.c b/tests/alias.c
index 5e0bfea3..344aba73 100644
--- a/tests/alias.c
+++ b/tests/alias.c
@@ -357,7 +357,7 @@ static void lb_empty(void **state)
 
 	will_return(__wrap_fgets, NULL);
 	expect_condlog(3, "No matching wwid [WWID0] in bindings file.\n");
-	rc = lookup_binding(NULL, "WWID0", &alias, NULL);
+	rc = lookup_binding(NULL, "WWID0", &alias, NULL, 0);
 	assert_int_equal(rc, 1);
 	assert_ptr_equal(alias, NULL);
 }
@@ -370,7 +370,7 @@ static void lb_match_a(void **state)
 	will_return(__wrap_fgets, "MPATHa WWID0\n");
 	expect_condlog(3, "Found matching wwid [WWID0] in bindings file."
 		       " Setting alias to MPATHa\n");
-	rc = lookup_binding(NULL, "WWID0", &alias, "MPATH");
+	rc = lookup_binding(NULL, "WWID0", &alias, "MPATH", 0);
 	assert_int_equal(rc, 0);
 	assert_ptr_not_equal(alias, NULL);
 	assert_string_equal(alias, "MPATHa");
@@ -385,7 +385,7 @@ static void lb_nomatch_a(void **state)
 	will_return(__wrap_fgets, "MPATHa WWID0\n");
 	will_return(__wrap_fgets, NULL);
 	expect_condlog(3, "No matching wwid [WWID1] in bindings file.\n");
-	rc = lookup_binding(NULL, "WWID1", &alias, "MPATH");
+	rc = lookup_binding(NULL, "WWID1", &alias, "MPATH", 0);
 	assert_int_equal(rc, 2);
 	assert_ptr_equal(alias, NULL);
 }
@@ -399,7 +399,7 @@ static void lb_match_c(void **state)
 	will_return(__wrap_fgets, "MPATHc WWID1\n");
 	expect_condlog(3, "Found matching wwid [WWID1] in bindings file."
 		       " Setting alias to MPATHc\n");
-	rc = lookup_binding(NULL, "WWID1", &alias, "MPATH");
+	rc = lookup_binding(NULL, "WWID1", &alias, "MPATH", 0);
 	assert_int_equal(rc, 0);
 	assert_ptr_not_equal(alias, NULL);
 	assert_string_equal(alias, "MPATHc");
@@ -415,7 +415,7 @@ static void lb_nomatch_a_c(void **state)
 	will_return(__wrap_fgets, "MPATHc WWID1\n");
 	will_return(__wrap_fgets, NULL);
 	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
-	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0);
 	assert_int_equal(rc, 2);
 	assert_ptr_equal(alias, NULL);
 }
@@ -429,7 +429,7 @@ static void lb_nomatch_c_a(void **state)
 	will_return(__wrap_fgets, "MPATHa WWID0\n");
 	will_return(__wrap_fgets, NULL);
 	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
-	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0);
 	assert_int_equal(rc, 2);
 	assert_ptr_equal(alias, NULL);
 }
@@ -444,7 +444,7 @@ static void lb_nomatch_a_b(void **state)
 	will_return(__wrap_fgets, "MPATHb WWID1\n");
 	will_return(__wrap_fgets, NULL);
 	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
-	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0);
 	assert_int_equal(rc, 3);
 	assert_ptr_equal(alias, NULL);
 }
@@ -460,7 +460,7 @@ static void lb_nomatch_a_b_bad(void **state)
 	will_return(__wrap_fgets, NULL);
 	expect_condlog(3, "Ignoring malformed line 3 in bindings file\n");
 	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
-	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0);
 	assert_int_equal(rc, 3);
 	assert_ptr_equal(alias, NULL);
 }
@@ -475,7 +475,7 @@ static void lb_nomatch_b_a(void **state)
 	will_return(__wrap_fgets, "MPATHa WWID0\n");
 	will_return(__wrap_fgets, NULL);
 	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
-	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0);
 	assert_int_equal(rc, 27);
 	assert_ptr_equal(alias, NULL);
 }
@@ -491,7 +491,7 @@ static void lb_nomatch_int_max(void **state)
 	will_return(__wrap_fgets, "MPATHa WWID0\n");
 	will_return(__wrap_fgets, NULL);
 	expect_condlog(0, "no more available user_friendly_names\n");
-	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0);
 	assert_int_equal(rc, -1);
 	assert_ptr_equal(alias, NULL);
 }
@@ -506,7 +506,7 @@ static void lb_nomatch_int_max_m1(void **state)
 	will_return(__wrap_fgets, "MPATHa WWID0\n");
 	will_return(__wrap_fgets, NULL);
 	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
-	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0);
 	assert_int_equal(rc, INT_MAX);
 	assert_ptr_equal(alias, NULL);
 }
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 2/2] tests: add tests for checking if alias is in use

[Benjamin Marzinski]

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 tests/alias.c | 409 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 405 insertions(+), 4 deletions(-)

diff --git a/tests/alias.c b/tests/alias.c
index 344aba73..ebe1209e 100644
--- a/tests/alias.c
+++ b/tests/alias.c
@@ -62,6 +62,25 @@ int __wrap_ftruncate(int fd, off_t length)
 	return __set_errno(mock_type(int));
 }
 
+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;
+
+	check_expected(name);
+	check_expected(uuid_len);
+	assert_non_null(uuid);
+	ret = mock_type(int);
+	if (ret == 0)
+		strcpy(uuid, mock_ptr_type(char *));
+	return ret;
+}
+
 static void fd_mpatha(void **state)
 {
 	char buf[32];
@@ -350,6 +369,45 @@ static int test_scan_devname(void)
 	return cmocka_run_group_tests(tests, NULL, NULL);
 }
 
+static void mock_unused_alias(const char *alias)
+{
+	expect_string(__wrap_dm_map_present, str, alias);
+	will_return(__wrap_dm_map_present, 0);
+}
+
+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);
+	will_return(__wrap_dm_get_uuid, wwid);
+}
+
+#define USED_STR(alias_str, wwid_str) wwid_str ": alias '" alias_str "' already taken, but not in bindings file. reselecting alias\n"
+
+static void mock_failed_alias(const char *alias, char *msg)
+{
+	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, msg);
+}
+
+static void mock_used_alias(const char *alias, char *msg)
+{
+	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);
+	will_return(__wrap_dm_get_uuid, "WWID_USED");
+	expect_condlog(3, msg);
+}
+
 static void lb_empty(void **state)
 {
 	int rc;
@@ -362,6 +420,65 @@ static void lb_empty(void **state)
 	assert_ptr_equal(alias, NULL);
 }
 
+static void lb_empty_unused(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, NULL);
+	mock_unused_alias("MPATHa");
+	expect_condlog(3, "No matching wwid [WWID0] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID0", &alias, "MPATH", 1);
+	assert_int_equal(rc, 1);
+	assert_ptr_equal(alias, NULL);
+	free(alias);
+}
+
+static void lb_empty_failed(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, NULL);
+	mock_failed_alias("MPATHa", USED_STR("MPATHa", "WWID0"));
+	mock_unused_alias("MPATHb");
+	expect_condlog(3, "No matching wwid [WWID0] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID0", &alias, "MPATH", 1);
+	assert_int_equal(rc, 2);
+	assert_ptr_equal(alias, NULL);
+	free(alias);
+}
+
+static void lb_empty_1_used(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, NULL);
+	mock_used_alias("MPATHa", USED_STR("MPATHa", "WWID0"));
+	mock_unused_alias("MPATHb");
+	expect_condlog(3, "No matching wwid [WWID0] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID0", &alias, "MPATH", 1);
+	assert_int_equal(rc, 2);
+	assert_ptr_equal(alias, NULL);
+	free(alias);
+}
+
+static void lb_empty_1_used_self(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, NULL);
+	mock_used_alias("MPATHa", USED_STR("MPATHa", "WWID0"));
+	mock_self_alias("MPATHb", "WWID0");
+	expect_condlog(3, "No matching wwid [WWID0] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID0", &alias, "MPATH", 1);
+	assert_int_equal(rc, 2);
+	assert_ptr_equal(alias, NULL);
+	free(alias);
+}
+
 static void lb_match_a(void **state)
 {
 	int rc;
@@ -390,7 +507,52 @@ static void lb_nomatch_a(void **state)
 	assert_ptr_equal(alias, NULL);
 }
 
-static void lb_match_c(void **state)
+static void lb_nomatch_a_bad_check(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHa WWID0\n");
+	will_return(__wrap_fgets, NULL);
+	expect_condlog(0, "no more available user_friendly_names\n");
+	rc = lookup_binding(NULL, "WWID1", &alias, NULL, 1);
+	assert_int_equal(rc, -1);
+	assert_ptr_equal(alias, NULL);
+}
+
+static void lb_nomatch_a_unused(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHa WWID0\n");
+	will_return(__wrap_fgets, NULL);
+	mock_unused_alias("MPATHb");
+	expect_condlog(3, "No matching wwid [WWID1] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID1", &alias, "MPATH", 1);
+	assert_int_equal(rc, 2);
+	assert_ptr_equal(alias, NULL);
+}
+
+static void lb_nomatch_a_3_used_failed_self(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHa WWID0\n");
+	will_return(__wrap_fgets, NULL);
+	mock_used_alias("MPATHb", USED_STR("MPATHb", "WWID1"));
+	mock_used_alias("MPATHc", USED_STR("MPATHc", "WWID1"));
+	mock_used_alias("MPATHd", USED_STR("MPATHd", "WWID1"));
+	mock_failed_alias("MPATHe", USED_STR("MPATHe", "WWID1"));
+	mock_self_alias("MPATHf", "WWID1");
+	expect_condlog(3, "No matching wwid [WWID1] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID1", &alias, "MPATH", 1);
+	assert_int_equal(rc, 6);
+	assert_ptr_equal(alias, NULL);
+}
+
+static void do_lb_match_c(void **state, int check_if_taken)
 {
 	int rc;
 	char *alias;
@@ -399,13 +561,23 @@ static void lb_match_c(void **state)
 	will_return(__wrap_fgets, "MPATHc WWID1\n");
 	expect_condlog(3, "Found matching wwid [WWID1] in bindings file."
 		       " Setting alias to MPATHc\n");
-	rc = lookup_binding(NULL, "WWID1", &alias, "MPATH", 0);
+	rc = lookup_binding(NULL, "WWID1", &alias, "MPATH", check_if_taken);
 	assert_int_equal(rc, 0);
 	assert_ptr_not_equal(alias, NULL);
 	assert_string_equal(alias, "MPATHc");
 	free(alias);
 }
 
+static void lb_match_c(void **state)
+{
+	do_lb_match_c(state, 0);
+}
+
+static void lb_match_c_check(void **state)
+{
+	do_lb_match_c(state, 1);
+}
+
 static void lb_nomatch_a_c(void **state)
 {
 	int rc;
@@ -420,6 +592,72 @@ static void lb_nomatch_a_c(void **state)
 	assert_ptr_equal(alias, NULL);
 }
 
+static void lb_nomatch_a_d_unused(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHa WWID0\n");
+	will_return(__wrap_fgets, "MPATHd WWID1\n");
+	will_return(__wrap_fgets, NULL);
+	mock_unused_alias("MPATHb");
+	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 1);
+	assert_int_equal(rc, 2);
+	assert_ptr_equal(alias, NULL);
+}
+
+static void lb_nomatch_a_d_1_used(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHa WWID0\n");
+	will_return(__wrap_fgets, "MPATHd WWID1\n");
+	will_return(__wrap_fgets, NULL);
+	mock_used_alias("MPATHb", USED_STR("MPATHb", "WWID2"));
+	mock_unused_alias("MPATHc");
+	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 1);
+	assert_int_equal(rc, 3);
+	assert_ptr_equal(alias, NULL);
+}
+
+static void lb_nomatch_a_d_2_used(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHa WWID0\n");
+	will_return(__wrap_fgets, "MPATHd WWID1\n");
+	will_return(__wrap_fgets, NULL);
+	mock_used_alias("MPATHb", USED_STR("MPATHb", "WWID2"));
+	mock_used_alias("MPATHc", USED_STR("MPATHc", "WWID2"));
+	mock_unused_alias("MPATHe");
+	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 1);
+	assert_int_equal(rc, 5);
+	assert_ptr_equal(alias, NULL);
+}
+
+static void lb_nomatch_a_d_3_used(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHa WWID0\n");
+	will_return(__wrap_fgets, "MPATHd WWID1\n");
+	will_return(__wrap_fgets, NULL);
+	mock_used_alias("MPATHb", USED_STR("MPATHb", "WWID2"));
+	mock_used_alias("MPATHc", USED_STR("MPATHc", "WWID2"));
+	mock_used_alias("MPATHe", USED_STR("MPATHe", "WWID2"));
+	mock_unused_alias("MPATHf");
+	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 1);
+	assert_int_equal(rc, 6);
+	assert_ptr_equal(alias, NULL);
+}
+
 static void lb_nomatch_c_a(void **state)
 {
 	int rc;
@@ -434,6 +672,39 @@ static void lb_nomatch_c_a(void **state)
 	assert_ptr_equal(alias, NULL);
 }
 
+static void lb_nomatch_d_a_unused(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHc WWID1\n");
+	will_return(__wrap_fgets, "MPATHa WWID0\n");
+	will_return(__wrap_fgets, "MPATHd WWID0\n");
+	will_return(__wrap_fgets, NULL);
+	mock_unused_alias("MPATHb");
+	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 1);
+	assert_int_equal(rc, 2);
+	assert_ptr_equal(alias, NULL);
+}
+
+static void lb_nomatch_d_a_1_used(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHc WWID1\n");
+	will_return(__wrap_fgets, "MPATHa WWID0\n");
+	will_return(__wrap_fgets, "MPATHd WWID0\n");
+	will_return(__wrap_fgets, NULL);
+	mock_used_alias("MPATHb", USED_STR("MPATHb", "WWID2"));
+	mock_unused_alias("MPATHe");
+	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 1);
+	assert_int_equal(rc, 5);
+	assert_ptr_equal(alias, NULL);
+}
+
 static void lb_nomatch_a_b(void **state)
 {
 	int rc;
@@ -465,6 +736,23 @@ static void lb_nomatch_a_b_bad(void **state)
 	assert_ptr_equal(alias, NULL);
 }
 
+static void lb_nomatch_a_b_bad_self(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHa WWID0\n");
+	will_return(__wrap_fgets, "MPATHz WWID26\n");
+	will_return(__wrap_fgets, "MPATHb\n");
+	will_return(__wrap_fgets, NULL);
+	expect_condlog(3, "Ignoring malformed line 3 in bindings file\n");
+	mock_self_alias("MPATHc", "WWID2");
+	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 1);
+	assert_int_equal(rc, 3);
+	assert_ptr_equal(alias, NULL);
+}
+
 static void lb_nomatch_b_a(void **state)
 {
 	int rc;
@@ -480,8 +768,27 @@ static void lb_nomatch_b_a(void **state)
 	assert_ptr_equal(alias, NULL);
 }
 
+static void lb_nomatch_b_a_3_used(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHb WWID1\n");
+	will_return(__wrap_fgets, "MPATHz WWID26\n");
+	will_return(__wrap_fgets, "MPATHa WWID0\n");
+	will_return(__wrap_fgets, NULL);
+	mock_used_alias("MPATHaa", USED_STR("MPATHaa", "WWID2"));
+	mock_used_alias("MPATHab", USED_STR("MPATHab", "WWID2"));
+	mock_used_alias("MPATHac", USED_STR("MPATHac", "WWID2"));
+	mock_unused_alias("MPATHad");
+	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 1);
+	assert_int_equal(rc, 30);
+	assert_ptr_equal(alias, NULL);
+}
+
 #ifdef MPATH_ID_INT_MAX
-static void lb_nomatch_int_max(void **state)
+static void do_lb_nomatch_int_max(void **state, int check_if_taken)
 {
 	int rc;
 	char *alias;
@@ -491,7 +798,32 @@ static void lb_nomatch_int_max(void **state)
 	will_return(__wrap_fgets, "MPATHa WWID0\n");
 	will_return(__wrap_fgets, NULL);
 	expect_condlog(0, "no more available user_friendly_names\n");
-	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 0);
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", check_if_taken);
+	assert_int_equal(rc, -1);
+	assert_ptr_equal(alias, NULL);
+}
+
+static void lb_nomatch_int_max(void **state)
+{
+	do_lb_nomatch_int_max(state, 0);
+}
+
+static void lb_nomatch_int_max_check(void **state)
+{
+	do_lb_nomatch_int_max(state, 1);
+}
+
+static void lb_nomatch_int_max_used(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHb WWID1\n");
+	will_return(__wrap_fgets, "MPATH" MPATH_ID_INT_MAX " WWIDMAX\n");
+	will_return(__wrap_fgets, NULL);
+	mock_used_alias("MPATHa", USED_STR("MPATHa", "WWID2"));
+	expect_condlog(0, "no more available user_friendly_names\n");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 1);
 	assert_int_equal(rc, -1);
 	assert_ptr_equal(alias, NULL);
 }
@@ -510,23 +842,92 @@ static void lb_nomatch_int_max_m1(void **state)
 	assert_int_equal(rc, INT_MAX);
 	assert_ptr_equal(alias, NULL);
 }
+
+static void lb_nomatch_int_max_m1_used(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHb WWID1\n");
+	will_return(__wrap_fgets, "MPATH" MPATH_ID_INT_MAX_m1 " WWIDMAX\n");
+	will_return(__wrap_fgets, "MPATHa WWID0\n");
+	will_return(__wrap_fgets, NULL);
+	mock_used_alias("MPATH" MPATH_ID_INT_MAX, USED_STR("MPATH" MPATH_ID_INT_MAX, "WWID2"));
+	expect_condlog(0, "no more available user_friendly_names\n");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 1);
+	assert_int_equal(rc, -1);
+	assert_ptr_equal(alias, NULL);
+}
+
+static void lb_nomatch_int_max_m1_1_used(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHb WWID1\n");
+	will_return(__wrap_fgets, "MPATH" MPATH_ID_INT_MAX_m1 " WWIDMAX\n");
+	will_return(__wrap_fgets, NULL);
+	mock_used_alias("MPATHa", USED_STR("MPATHa", "WWID2"));
+	mock_unused_alias("MPATH" MPATH_ID_INT_MAX);
+	expect_condlog(3, "No matching wwid [WWID2] in bindings file.\n");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 1);
+	assert_int_equal(rc, INT_MAX);
+	assert_ptr_equal(alias, NULL);
+}
+
+static void lb_nomatch_int_max_m1_2_used(void **state)
+{
+	int rc;
+	char *alias;
+
+	will_return(__wrap_fgets, "MPATHb WWID1\n");
+	will_return(__wrap_fgets, "MPATH" MPATH_ID_INT_MAX_m1 " WWIDMAX\n");
+	will_return(__wrap_fgets, NULL);
+	mock_used_alias("MPATHa", USED_STR("MPATHa", "WWID2"));
+	mock_used_alias("MPATH" MPATH_ID_INT_MAX, USED_STR("MPATH" MPATH_ID_INT_MAX, "WWID2"));
+	expect_condlog(0, "no more available user_friendly_names\n");
+	rc = lookup_binding(NULL, "WWID2", &alias, "MPATH", 1);
+	assert_int_equal(rc, -1);
+	assert_ptr_equal(alias, NULL);
+}
 #endif
 
 static int test_lookup_binding(void)
 {
 	const struct CMUnitTest tests[] = {
 		cmocka_unit_test(lb_empty),
+		cmocka_unit_test(lb_empty_unused),
+		cmocka_unit_test(lb_empty_failed),
+		cmocka_unit_test(lb_empty_1_used),
+		cmocka_unit_test(lb_empty_1_used_self),
 		cmocka_unit_test(lb_match_a),
 		cmocka_unit_test(lb_nomatch_a),
+		cmocka_unit_test(lb_nomatch_a_bad_check),
+		cmocka_unit_test(lb_nomatch_a_unused),
+		cmocka_unit_test(lb_nomatch_a_3_used_failed_self),
 		cmocka_unit_test(lb_match_c),
+		cmocka_unit_test(lb_match_c_check),
 		cmocka_unit_test(lb_nomatch_a_c),
+		cmocka_unit_test(lb_nomatch_a_d_unused),
+		cmocka_unit_test(lb_nomatch_a_d_1_used),
+		cmocka_unit_test(lb_nomatch_a_d_2_used),
+		cmocka_unit_test(lb_nomatch_a_d_3_used),
 		cmocka_unit_test(lb_nomatch_c_a),
+		cmocka_unit_test(lb_nomatch_d_a_unused),
+		cmocka_unit_test(lb_nomatch_d_a_1_used),
 		cmocka_unit_test(lb_nomatch_a_b),
 		cmocka_unit_test(lb_nomatch_a_b_bad),
+		cmocka_unit_test(lb_nomatch_a_b_bad_self),
 		cmocka_unit_test(lb_nomatch_b_a),
+		cmocka_unit_test(lb_nomatch_b_a_3_used),
 #ifdef MPATH_ID_INT_MAX
 		cmocka_unit_test(lb_nomatch_int_max),
+		cmocka_unit_test(lb_nomatch_int_max_check),
+		cmocka_unit_test(lb_nomatch_int_max_used),
 		cmocka_unit_test(lb_nomatch_int_max_m1),
+		cmocka_unit_test(lb_nomatch_int_max_m1_used),
+		cmocka_unit_test(lb_nomatch_int_max_m1_1_used),
+		cmocka_unit_test(lb_nomatch_int_max_m1_2_used),
 #endif
 	};
 
-- 
2.17.2

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 1/2] libmultipath: check if user_friendly_name is in use

[Martin Wilck]

On Wed, 2021-03-10 at 15:15 -0600, Benjamin Marzinski wrote:
> If there are multipath devices that have user_friendly_names but do
> not
> have their bindings in the bindings_file, get_user_friendly_alias()
> can
> currently give out those names again. This can result in an incorrect
> entry in the bindings file, and a device that gets created with a
> WWID
> alias instead of a user_friendly_name. This situation can happen
> after
> the pivot root, if a multipath device is created in the initramfs. 
> If
> this device doesn't have a binding in the regular filesystem
> bindings_file and a new multipath device is created before it can add
> its binding, the new device can steal that user_friendly_name during
> multipathd's initial configure.
> 
> To solve this, get_user_friendly_alias() now calls lookup_binding()
> with
> a new paramter, telling it to check if the id it found is already in
> use
> by a diffent device. If so, lookup_binding() will continue to check
> open
> ids, until it finds one that it not currently in use by a dm device.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> 


>  /*
>   * Returns: 0   if matching entry in WWIDs file found
>   *         -1   if an error occurs
> @@ -128,7 +151,7 @@ scan_devname(const char *alias, const char
> *prefix)
>   */
>  static int
>  lookup_binding(FILE *f, const char *map_wwid, char **map_alias,
> -              const char *prefix)
> +              const char *prefix, int check_if_taken)
>  {
>         char buf[LINE_MAX];
>         unsigned int line_nr = 0;
> @@ -183,12 +206,31 @@ lookup_binding(FILE *f, const char *map_wwid,
> char **map_alias,
>                         return 0;
>                 }
>         }
> +       if (!prefix && check_if_taken)
> +               id = -1;
>         if (id >= smallest_bigger_id) {
>                 if (biggest_id < INT_MAX)
>                         id = biggest_id + 1;
>                 else
>                         id = -1;
>         }
> +       if (id > 0 && check_if_taken) {
> +               while(id_already_taken(id, prefix, map_wwid)) {
> +                       if (id == INT_MAX) {
> +                               id = -1;
> +                               break;
> +                       }
> +                       id++;
> +                       if (id == smallest_bigger_id) {
> +                               if (biggest_id == INT_MAX) {
> +                                       id = -1;
> +                                       break;
> +                               }
> +                               if (biggest_id >= smallest_bigger_id)
> +                                       id = biggest_id + 1;
> +                       }
> +               }
> +       }

This seems to be correct, but I am never certain when I look at this
code. I truly dislike the awkward logic of lookup_binding(), and adding
more complexity to it doesn't improve matters. I really only trust that
logic because of the unit test.

Rather than re-reading the file on every lookup, we should cache its
contents in a sorted array. That would make it much easier to 
write an algorithm for locating free slots that could be reviewed by
average human beings. We could even add the aliases of detected
existing maps to that array, so that no additional "already taken"
check would be necessary.

I'd actually started working on that some time ago, but never finished
that work.

Anyway, it't not a cause against your patch.

Martin

Reviewed-by: Martin Wilck <mwilck@suse.com>>


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH 2/2] tests: add tests for checking if alias is in use

[Martin Wilck]

On Wed, 2021-03-10 at 15:15 -0600, Benjamin Marzinski wrote:
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

Added this and your other series to my "queue" branch.


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel