diff mbox series

[v3,04/12] libmultipath: set uuid, name, and dmi if a device is found

Message ID 20241126184224.855459-5-bmarzins@redhat.com (mailing list archive)
State Not Applicable, archived
Delegated to: Benjamin Marzinski
Headers show
Series multipath-tools: Handle tableless DM devices | expand

Commit Message

Benjamin Marzinski Nov. 26, 2024, 6:42 p.m. UTC 
Instead of only setting the uuid, name, and dmi on DMP_OK, return them
whenever we find a device, even if that device doesn't match. To make the
code simpler, we set these values before checks that could possibly return
DMP_ERR.

The only caller of libmp_mapinfo() that could update a multipath value
incorrectly because of this is update_multipath_table(). Fix that.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/devmapper.c   | 47 +++++++++++++++++++-------------------
 libmultipath/devmapper.h   |  5 +++-
 libmultipath/structs_vec.c |  5 +++-
 tests/mapinfo.c            | 45 +++++++++++++++++++++++++-----------
 4 files changed, 63 insertions(+), 39 deletions(-)
diff mbox series

Patch

diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 42176667..38c49bd5 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -736,6 +736,29 @@  static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma
 		&& !(uuid = dm_task_get_uuid(dmt))))
 		return DMP_ERR;
 
+	if (info.name) {
+		strlcpy(info.name, name, WWID_SIZE);
+		condlog(4, "%s: %s: name: \"%s\"", fname__, map_id, info.name);
+	}
+	if (info.uuid) {
+		strlcpy(info.uuid, uuid, DM_UUID_LEN);
+		condlog(4, "%s: %s: uuid: \"%s\"", fname__, map_id, info.uuid);
+	}
+
+	if (info.dmi) {
+		memcpy(info.dmi, &dmi, sizeof(*info.dmi));
+		condlog(4, "%s: %s %d:%d, %d targets, %s table, %s, %s, %d opened, %u events",
+			fname__, map_id,
+			info.dmi->major, info.dmi->minor,
+			info.dmi->target_count,
+			info.dmi->live_table ? "live" :
+				info.dmi->inactive_table ? "inactive" : "no",
+			info.dmi->suspended ? "suspended" : "active",
+			info.dmi->read_only ? "ro" : "rw",
+			info.dmi->open_count,
+			info.dmi->event_nr);
+	}
+
 	if (flags & MAPINFO_CHECK_UUID &&
 	    ((flags & MAPINFO_PART_ONLY && !is_mpath_part_uuid(uuid, NULL)) ||
 	     (!(flags & MAPINFO_PART_ONLY) && !is_mpath_uuid(uuid)))) {
@@ -768,40 +791,16 @@  static int libmp_mapinfo__(int flags, mapid_t id, mapinfo_t info, const char *ma
 
 	/*
 	 * Check possible error conditions.
-	 * If error is returned, don't touch any output parameters.
 	 */
 	if ((info.status && !(tmp_status = strdup(params)))
 	    || (info.target && !tmp_target && !(tmp_target = strdup(params))))
 		return DMP_ERR;
 
-	if (info.name) {
-		strlcpy(info.name, name, WWID_SIZE);
-		condlog(4, "%s: %s: name: \"%s\"", fname__, map_id, info.name);
-	}
-	if (info.uuid) {
-		strlcpy(info.uuid, uuid, DM_UUID_LEN);
-		condlog(4, "%s: %s: uuid: \"%s\"", fname__, map_id, info.uuid);
-	}
-
 	if (info.size) {
 		*info.size = length;
 		condlog(4, "%s: %s: size: %lld", fname__, map_id, *info.size);
 	}
 
-	if (info.dmi) {
-		memcpy(info.dmi, &dmi, sizeof(*info.dmi));
-		condlog(4, "%s: %s %d:%d, %d targets, %s table, %s, %s, %d opened, %u events",
-			fname__, map_id,
-			info.dmi->major, info.dmi->minor,
-			info.dmi->target_count,
-			info.dmi->live_table ? "live" :
-				info.dmi->inactive_table ? "inactive" : "no",
-			info.dmi->suspended ? "suspended" : "active",
-			info.dmi->read_only ? "ro" : "rw",
-			info.dmi->open_count,
-			info.dmi->event_nr);
-	}
-
 	if (info.target) {
 		*info.target = steal_ptr(tmp_target);
 		if (!tgt_set)
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 0bd2b907..4aeefad1 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -115,7 +115,10 @@  typedef struct libmp_map_info {
  *
  * This function obtains the requested information for the device-mapper map
  * identified by the input parameters.
- * Output parameters are only filled in if the return value is DMP_OK.
+ * If non-NULL, the name, uuid, and dmi output paramters may be filled in for
+ * any return value besides DMP_NOT_FOUND and will always be filled in for
+ * return values other than DMP_NOT_FOUND and DMP_ERR.
+ * The other parameters are only filled in if the return value is DMP_OK.
  * For target / status / size information, the  map's table should contain
  * only one target (usually multipath or linear).
  */
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 5df495b3..d22056ca 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -504,6 +504,8 @@  update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
 	int r = DMP_ERR;
 	char __attribute__((cleanup(cleanup_charp))) *params = NULL;
 	char __attribute__((cleanup(cleanup_charp))) *status = NULL;
+	/* only set the actual mpp->dmi if libmp_mapinfo returns DMP_OK */
+	struct dm_info dmi;
 	unsigned long long size;
 	struct config *conf;
 
@@ -522,7 +524,7 @@  update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
 				  .target = &params,
 				  .status = &status,
 				  .size = &mpp->size,
-				  .dmi = &mpp->dmi,
+				  .dmi = &dmi,
 			  });
 
 	if (r != DMP_OK) {
@@ -531,6 +533,7 @@  update_multipath_table (struct multipath *mpp, vector pathvec, int flags)
 	} else if (size != mpp->size)
 		condlog(0, "%s: size changed from %llu to %llu", mpp->alias, size, mpp->size);
 
+	mpp->dmi = dmi;
 	return update_multipath_table__(mpp, pathvec, flags, params, status);
 }
 
diff --git a/tests/mapinfo.c b/tests/mapinfo.c
index ee487989..36511607 100644
--- a/tests/mapinfo.c
+++ b/tests/mapinfo.c
@@ -451,15 +451,21 @@  static void test_mapinfo_bad_check_uuid_00(void **state)
 static void test_mapinfo_bad_check_uuid_01(void **state)
 {
 	int rc;
+	struct dm_info dmi = { .suspended = 0 };
+	char name[WWID_SIZE] = { 0 };
+	char uuid[DM_UUID_LEN] = { 0 };
 
 	mock_mapinfo_name_1(DM_DEVICE_INFO, 1, "foo", 1, 1, 0);
 	WRAP_DM_TASK_GET_INFO(1);
 	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_01);
+	will_return(__wrap_dm_task_get_name, MPATH_NAME_01);
 	will_return(__wrap_dm_task_get_uuid, BAD_UUID_01);
 	rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID,
 			   (mapid_t) { .str = "foo", },
-			   (mapinfo_t) { .name = NULL });
+			   (mapinfo_t) { .dmi = &dmi, .name = name, .uuid = uuid });
 	assert_int_equal(rc, DMP_NO_MATCH);
+	assert_true(!strcmp(name, MPATH_NAME_01));
+	assert_true(!strcmp(uuid, BAD_UUID_01));
 }
 
 static void test_mapinfo_bad_check_uuid_02(void **state)
@@ -887,17 +893,26 @@  static void test_mapinfo_bad_next_target_01(void **state)
 static void test_mapinfo_bad_next_target_02(void **state)
 {
 	int rc;
-	unsigned long long size;
+	struct dm_info dmi = { .suspended = 0 };
+	char name[WWID_SIZE] = { 0 };
+	char uuid[DM_UUID_LEN] = { 0 };
+	unsigned long long size = 0;
 
 	mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 1, 0);
 	WRAP_DM_TASK_GET_INFO(1);
 	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_02);
+	will_return(__wrap_dm_task_get_name, MPATH_NAME_01);
+	will_return(__wrap_dm_task_get_uuid, MPATH_UUID_01);
 	/* no targets */
 	mock_dm_get_next_target(0, NULL, NULL, NULL);
 	rc = libmp_mapinfo(DM_MAP_BY_NAME,
 			   (mapid_t) { .str = "foo", },
-			   (mapinfo_t) { .size = &size });
+			   (mapinfo_t) { .dmi = &dmi, .name = name, .uuid = uuid, .size = &size });
 	assert_int_equal(rc, DMP_EMPTY);
+	assert_int_equal(size, 0);
+	assert_memory_equal(&dmi, &MPATH_DMI_02, sizeof(dmi));
+	assert_true(!strcmp(name, MPATH_NAME_01));
+	assert_true(!strcmp(uuid, MPATH_UUID_01));
 }
 
 /* libmp_mapinfo needs to do a DM_DEVICE_STATUS ioctl */
@@ -946,10 +961,10 @@  static void test_mapinfo_bad_target_type_03(void **state)
 			   (mapid_t) { .str = "foo", },
 			   (mapinfo_t) { .dmi = &dmi, .name = name, .uuid = uuid });
 	assert_int_equal(rc, DMP_NO_MATCH);
-	/* make sure memory content is not changed */
-	assert_memory_equal(&dmi, &((struct dm_info) { .exists = 0 }), sizeof(dmi));
-	assert_memory_equal(&name, &((char[WWID_SIZE]) { 0 }), WWID_SIZE);
-	assert_memory_equal(&uuid, &((char[DM_UUID_LEN]) { 0 }), DM_UUID_LEN);
+	/* make sure that the ouput was filled in */
+	assert_memory_equal(&dmi, &MPATH_DMI_01, sizeof(dmi));
+	assert_true(!strcmp(name, MPATH_NAME_01));
+	assert_true(!strcmp(uuid, MPATH_UUID_01));
 }
 
 static void test_mapinfo_bad_target_type_04(void **state)
@@ -1065,23 +1080,32 @@  static void test_mapinfo_no_table_01(void **state)
 			   (mapid_t) { .str = "foo", },
 			   (mapinfo_t) { .dmi = &dmi });
 	assert_int_equal(rc, DMP_EMPTY);
+	assert_memory_equal(&dmi, &MPATH_DMI_02, sizeof(dmi));
 }
 
 static void test_mapinfo_no_table_02(void **state)
 {
 	int rc;
 	struct dm_info dmi = { .suspended = 0 };
+	char name[WWID_SIZE] = { 0 };
+	char uuid[DM_UUID_LEN] = { 0 };
+	unsigned long long size = 0;
 
 	mock_mapinfo_name_1(DM_DEVICE_STATUS, 1, "foo", 1, 1, 0);
 	WRAP_DM_TASK_GET_INFO(1);
 	/* DMI with no live table, MAPINFO_CHECK_UUID set */
 	WRAP_DM_TASK_GET_INFO(&MPATH_DMI_02);
+	will_return(__wrap_dm_task_get_name, MPATH_NAME_01);
 	mock_dm_get_next_target(0, NULL, NULL, NULL);
 	will_return(__wrap_dm_task_get_uuid, MPATH_UUID_01);
 	rc = libmp_mapinfo(DM_MAP_BY_NAME | MAPINFO_CHECK_UUID | MAPINFO_MPATH_ONLY,
 			   (mapid_t) { .str = "foo", },
-			   (mapinfo_t) { .dmi = &dmi });
+			   (mapinfo_t) { .dmi = &dmi, .name = name, .uuid = uuid, .size = &size });
 	assert_int_equal(rc, DMP_EMPTY);
+	assert_int_equal(size, 0);
+	assert_memory_equal(&dmi, &MPATH_DMI_02, sizeof(dmi));
+	assert_true(!strcmp(name, MPATH_NAME_01));
+	assert_true(!strcmp(uuid, MPATH_UUID_01));
 }
 
 static void test_mapinfo_good_status_01(void **state)
@@ -1121,8 +1145,6 @@  static void test_mapinfo_bad_strdup_01(void **state)
 			   (mapinfo_t) { .status = &status, .uuid = uuid, .name = name });
 	assert_int_equal(rc, DMP_ERR);
 	assert_null(status);
-	assert_memory_equal(&name, &((char[WWID_SIZE]) { 0 }), WWID_SIZE);
-	assert_memory_equal(&uuid, &((char[DM_UUID_LEN]) { 0 }), DM_UUID_LEN);
 
 }
 
@@ -1284,9 +1306,6 @@  static void test_mapinfo_bad_strdup_02(void **state)
 	assert_int_equal(rc, DMP_ERR);
 	assert_null(status);
 	assert_null(target);
-	assert_memory_equal(&dmi, &((struct dm_info) { .suspended = 0 }), sizeof(dmi));
-	assert_memory_equal(&name, &((char[WWID_SIZE]) { 0 }), WWID_SIZE);
-	assert_memory_equal(&uuid, &((char[DM_UUID_LEN]) { 0 }), DM_UUID_LEN);
 }
 
 static void test_mapinfo_bad_strdup_03(void **state)