diff mbox series

[v2,25/49] libmultipath: improve dm_get_wwid() return value logic

Message ID 20240712171458.77611-26-mwilck@suse.com (mailing list archive)
State Not Applicable, archived
Delegated to: christophe varoqui
Headers show
Series multipath-tools: devmapper API refactored | expand

Commit Message

Martin Wilck July 12, 2024, 5:14 p.m. UTC 
Make dm_get_wwid() return different status codes for non-existing maps,
maps that exists but are not multipath maps, and generic error case,
and handle these return codes appropriately in callers.

The error handling is als changed; dm_get_wwid() doesn't take
care of making the ouput 0-terminated if anything fails. The
caller is responsible for that. Change callers accordingly.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/alias.c     | 11 ++++++++---
 libmultipath/configure.c | 27 +++++++++++++++------------
 libmultipath/devmapper.c | 22 +++++++++++++++++-----
 libmultipath/wwids.c     |  2 +-
 multipathd/main.c        |  7 +++++--
 tests/alias.c            | 10 +++++-----
 6 files changed, 51 insertions(+), 28 deletions(-)

Comments

Benjamin Marzinski July 15, 2024, 10:42 p.m. UTC | #1 
On Fri, Jul 12, 2024 at 07:14:33PM +0200, Martin Wilck wrote:
> Make dm_get_wwid() return different status codes for non-existing maps,
> maps that exists but are not multipath maps, and generic error case,
> and handle these return codes appropriately in callers.
> 
> The error handling is als changed; dm_get_wwid() doesn't take
> care of making the ouput 0-terminated if anything fails. The
> caller is responsible for that. Change callers accordingly.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/alias.c     | 11 ++++++++---
>  libmultipath/configure.c | 27 +++++++++++++++------------
>  libmultipath/devmapper.c | 22 +++++++++++++++++-----
>  libmultipath/wwids.c     |  2 +-
>  multipathd/main.c        |  7 +++++--
>  tests/alias.c            | 10 +++++-----
>  6 files changed, 51 insertions(+), 28 deletions(-)
> 
> diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> index 10e58a7..c4eb5d8 100644
> --- a/libmultipath/alias.c
> +++ b/libmultipath/alias.c
> @@ -408,13 +408,18 @@ static bool alias_already_taken(const char *alias, const char *map_wwid)
>  {
>  
>  	char wwid[WWID_SIZE];
> +	int rc = dm_get_wwid(alias, wwid, sizeof(wwid));
>  
> -	/* If the map doesn't exist, it's fine */
> -	if (dm_get_wwid(alias, wwid, sizeof(wwid)) != 0)
> +	/*
> +	 * If the map doesn't exist, it's fine.
> +	 * In the generic error case, assume that the device is not
> +	 * taken, and try to proceed.
> +	 */
> +	if (rc == DMP_NOT_FOUND || rc == DMP_ERR)
>  		return false;
>  
>  	/* If both the name and the wwid match, it's fine.*/
> -	if (strncmp(map_wwid, wwid, sizeof(wwid)) == 0)
> +	if (rc == DMP_OK && strncmp(map_wwid, wwid, sizeof(wwid)) == 0)
>  		return false;
>  
>  	condlog(3, "%s: alias '%s' already taken, reselecting alias",
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 666d4e8..2fdaca8 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -845,18 +845,21 @@ int domap(struct multipath *mpp, char *params, int is_daemon)
>  
>  	if (mpp->action == ACT_CREATE && dm_map_present(mpp->alias)) {
>  		char wwid[WWID_SIZE];
> +		int rc = dm_get_wwid(mpp->alias, wwid, sizeof(wwid));
>  
> -		if (dm_get_wwid(mpp->alias, wwid, sizeof(wwid)) == 0) {
> -			if (!strncmp(mpp->wwid, wwid, sizeof(wwid))) {
> -				condlog(3, "%s: map already present",
> -					mpp->alias);
> -				mpp->action = ACT_RELOAD;
> -			} else {
> -				condlog(0, "%s: map \"%s\" already present with WWID %s, skipping",
> -					mpp->wwid, mpp->alias, wwid);
> -				condlog(0, "please check alias settings in config and bindings file");
> -				mpp->action = ACT_REJECT;
> -			}
> +		if (rc == DMP_OK && !strncmp(mpp->wwid, wwid, sizeof(wwid))) {
> +			condlog(3, "%s: map already present",
> +				mpp->alias);
> +			mpp->action = ACT_RELOAD;
> +		} else if (rc == DMP_OK) {
> +			condlog(1, "%s: map \"%s\" already present with WWID \"%s\", skipping\n"
> +				   "please check alias settings in config and bindings file",
> +				mpp->wwid, mpp->alias, wwid);
> +			mpp->action = ACT_REJECT;
> +		} else if (rc == DMP_NO_MATCH) {
> +			condlog(1, "%s: alias \"%s\" already taken by a non-multipath map",
> +				mpp->wwid, mpp->alias);
> +			mpp->action = ACT_REJECT;
>  		}
>  	}
>  
> @@ -1320,7 +1323,7 @@ static int _get_refwwid(enum mpath_cmds cmd, const char *dev,
>  		break;
>  
>  	case DEV_DEVMAP:
> -		if (((dm_get_wwid(dev, tmpwwid, WWID_SIZE)) == 0)
> +		if (((dm_get_wwid(dev, tmpwwid, WWID_SIZE)) == DMP_OK)
>  		    && (strlen(tmpwwid)))
>  			refwwid = tmpwwid;
>  
> diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> index 3fca08c..003d834 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -840,19 +840,29 @@ int dm_get_map(const char *name, unsigned long long *size, char **outparams)
>  	}
>  }
>  
> +/**
> + * dm_get_wwid(): return WWID for a multipath map
> + * @returns:
> + *    DMP_OK if successful
> + *    DMP_NOT_FOUND if the map doesn't exist
> + *    DMP_NO_MATCH if the map exists but is not a multipath map
> + *    DMP_ERR for other errors
> + * Caller may access uuid if and only if DMP_OK is returned.
> + */
>  int dm_get_wwid(const char *name, char *uuid, int uuid_len)
>  {
>  	char tmp[DM_UUID_LEN];
> +	int rc = dm_get_dm_uuid(name, tmp);
>  
> -	if (dm_get_dm_uuid(name, tmp) != DMP_OK)
> -		return 1;
> +	if (rc != DMP_OK)
> +		return rc;
>  
>  	if (!strncmp(tmp, UUID_PREFIX, UUID_PREFIX_LEN))
>  		strlcpy(uuid, tmp + UUID_PREFIX_LEN, uuid_len);
>  	else
> -		uuid[0] = '\0';
> +		return DMP_NO_MATCH;
>  
> -	return 0;
> +	return DMP_OK;
>  }
>  
>  static int is_mpath_part(const char *part_name, const char *map_name)
> @@ -1388,8 +1398,10 @@ struct multipath *dm_get_multipath(const char *name)
>  	if (dm_get_map(name, &mpp->size, NULL) != DMP_OK)
>  		goto out;
>  
> -	if (dm_get_wwid(name, mpp->wwid, WWID_SIZE) != 0)
> +	if (dm_get_wwid(name, mpp->wwid, WWID_SIZE) != DMP_OK) {
>  		condlog(2, "%s: failed to get uuid for %s", __func__, name);
> +		mpp->wwid[0] = '\0';
> +	}
>  	if (dm_get_info(name, &mpp->dmi) != 0)
>  		condlog(2, "%s: failed to get info for %s", __func__, name);
>  
> diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
> index 7a4cb74..aac18c0 100644
> --- a/libmultipath/wwids.c
> +++ b/libmultipath/wwids.c
> @@ -295,7 +295,7 @@ should_multipath(struct path *pp1, vector pathvec, vector mpvec)
>  		struct multipath *mp = find_mp_by_wwid(mpvec, pp1->wwid);
>  
>  		if (mp != NULL &&
> -		    dm_get_wwid(mp->alias, tmp_wwid, WWID_SIZE) == 0 &&
> +		    dm_get_wwid(mp->alias, tmp_wwid, WWID_SIZE) == DMP_OK &&
>  		    !strncmp(tmp_wwid, pp1->wwid, WWID_SIZE)) {
>  			condlog(3, "wwid %s is already multipathed, keeping it",
>  				pp1->wwid);
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 442a154..1e7a6ac 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -726,8 +726,11 @@ add_map_without_path (struct vectors *vecs, const char *alias)
>  		condlog(3, "%s: cannot access table", mpp->alias);
>  		goto out;
>  	}
> -	if (!strlen(mpp->wwid))
> -		dm_get_wwid(mpp->alias, mpp->wwid, WWID_SIZE);
> +	if (!strlen(mpp->wwid) &&
> +	    dm_get_wwid(mpp->alias, mpp->wwid, WWID_SIZE) != DMP_OK) {
> +		condlog(3, "%s: cannot obtain WWID", mpp->alias);
> +		goto out;
> +	}
>  	if (!strlen(mpp->wwid))
>  		condlog(1, "%s: adding map with empty WWID", mpp->alias);
>  	conf = get_multipath_config();
> diff --git a/tests/alias.c b/tests/alias.c
> index 1f78656..a95b308 100644
> --- a/tests/alias.c
> +++ b/tests/alias.c
> @@ -84,7 +84,7 @@ int __wrap_dm_get_wwid(const char *name, char *uuid, int uuid_len)
>  	check_expected(uuid_len);
>  	assert_non_null(uuid);
>  	ret = mock_type(int);
> -	if (ret == 0)
> +	if (ret == DMP_OK)
>  		strcpy(uuid, mock_ptr_type(char *));
>  	return ret;
>  }
> @@ -438,14 +438,14 @@ static void mock_unused_alias(const char *alias)
>  {
>  	expect_string(__wrap_dm_get_wwid, name, alias);
>  	expect_value(__wrap_dm_get_wwid, uuid_len, WWID_SIZE);
> -	will_return(__wrap_dm_get_wwid, 1);
> +	will_return(__wrap_dm_get_wwid, DMP_NOT_FOUND);
>  }
>  
>  static void mock_self_alias(const char *alias, const char *wwid)
>  {
>  	expect_string(__wrap_dm_get_wwid, name, alias);
>  	expect_value(__wrap_dm_get_wwid, uuid_len, WWID_SIZE);
> -	will_return(__wrap_dm_get_wwid, 0);
> +	will_return(__wrap_dm_get_wwid, DMP_OK);
>  	will_return(__wrap_dm_get_wwid, wwid);
>  }
>  
> @@ -471,14 +471,14 @@ static void mock_self_alias(const char *alias, const char *wwid)
>  	do {								\
>  		expect_string(__wrap_dm_get_wwid, name, alias);		\
>  		expect_value(__wrap_dm_get_wwid, uuid_len, WWID_SIZE);	\
> -		will_return(__wrap_dm_get_wwid, 1);			\
> +		will_return(__wrap_dm_get_wwid, DMP_NOT_FOUND);		\
>  	} while (0)
>  
>  #define mock_used_alias(alias, wwid)					\
>  	do {								\
>  		expect_string(__wrap_dm_get_wwid, name, alias);		\
>  		expect_value(__wrap_dm_get_wwid, uuid_len, WWID_SIZE);	\
> -		will_return(__wrap_dm_get_wwid, 0);			\
> +		will_return(__wrap_dm_get_wwid, DMP_OK);		\
>  		will_return(__wrap_dm_get_wwid, "WWID_USED");		\
>  		expect_condlog(3, USED_STR(alias, wwid));		\
>  	} while(0)
> -- 
> 2.45.2
diff mbox series

Patch

diff --git a/libmultipath/alias.c b/libmultipath/alias.c
index 10e58a7..c4eb5d8 100644
--- a/libmultipath/alias.c
+++ b/libmultipath/alias.c
@@ -408,13 +408,18 @@  static bool alias_already_taken(const char *alias, const char *map_wwid)
 {
 
 	char wwid[WWID_SIZE];
+	int rc = dm_get_wwid(alias, wwid, sizeof(wwid));
 
-	/* If the map doesn't exist, it's fine */
-	if (dm_get_wwid(alias, wwid, sizeof(wwid)) != 0)
+	/*
+	 * If the map doesn't exist, it's fine.
+	 * In the generic error case, assume that the device is not
+	 * taken, and try to proceed.
+	 */
+	if (rc == DMP_NOT_FOUND || rc == DMP_ERR)
 		return false;
 
 	/* If both the name and the wwid match, it's fine.*/
-	if (strncmp(map_wwid, wwid, sizeof(wwid)) == 0)
+	if (rc == DMP_OK && strncmp(map_wwid, wwid, sizeof(wwid)) == 0)
 		return false;
 
 	condlog(3, "%s: alias '%s' already taken, reselecting alias",
diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 666d4e8..2fdaca8 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -845,18 +845,21 @@  int domap(struct multipath *mpp, char *params, int is_daemon)
 
 	if (mpp->action == ACT_CREATE && dm_map_present(mpp->alias)) {
 		char wwid[WWID_SIZE];
+		int rc = dm_get_wwid(mpp->alias, wwid, sizeof(wwid));
 
-		if (dm_get_wwid(mpp->alias, wwid, sizeof(wwid)) == 0) {
-			if (!strncmp(mpp->wwid, wwid, sizeof(wwid))) {
-				condlog(3, "%s: map already present",
-					mpp->alias);
-				mpp->action = ACT_RELOAD;
-			} else {
-				condlog(0, "%s: map \"%s\" already present with WWID %s, skipping",
-					mpp->wwid, mpp->alias, wwid);
-				condlog(0, "please check alias settings in config and bindings file");
-				mpp->action = ACT_REJECT;
-			}
+		if (rc == DMP_OK && !strncmp(mpp->wwid, wwid, sizeof(wwid))) {
+			condlog(3, "%s: map already present",
+				mpp->alias);
+			mpp->action = ACT_RELOAD;
+		} else if (rc == DMP_OK) {
+			condlog(1, "%s: map \"%s\" already present with WWID \"%s\", skipping\n"
+				   "please check alias settings in config and bindings file",
+				mpp->wwid, mpp->alias, wwid);
+			mpp->action = ACT_REJECT;
+		} else if (rc == DMP_NO_MATCH) {
+			condlog(1, "%s: alias \"%s\" already taken by a non-multipath map",
+				mpp->wwid, mpp->alias);
+			mpp->action = ACT_REJECT;
 		}
 	}
 
@@ -1320,7 +1323,7 @@  static int _get_refwwid(enum mpath_cmds cmd, const char *dev,
 		break;
 
 	case DEV_DEVMAP:
-		if (((dm_get_wwid(dev, tmpwwid, WWID_SIZE)) == 0)
+		if (((dm_get_wwid(dev, tmpwwid, WWID_SIZE)) == DMP_OK)
 		    && (strlen(tmpwwid)))
 			refwwid = tmpwwid;
 
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 3fca08c..003d834 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -840,19 +840,29 @@  int dm_get_map(const char *name, unsigned long long *size, char **outparams)
 	}
 }
 
+/**
+ * dm_get_wwid(): return WWID for a multipath map
+ * @returns:
+ *    DMP_OK if successful
+ *    DMP_NOT_FOUND if the map doesn't exist
+ *    DMP_NO_MATCH if the map exists but is not a multipath map
+ *    DMP_ERR for other errors
+ * Caller may access uuid if and only if DMP_OK is returned.
+ */
 int dm_get_wwid(const char *name, char *uuid, int uuid_len)
 {
 	char tmp[DM_UUID_LEN];
+	int rc = dm_get_dm_uuid(name, tmp);
 
-	if (dm_get_dm_uuid(name, tmp) != DMP_OK)
-		return 1;
+	if (rc != DMP_OK)
+		return rc;
 
 	if (!strncmp(tmp, UUID_PREFIX, UUID_PREFIX_LEN))
 		strlcpy(uuid, tmp + UUID_PREFIX_LEN, uuid_len);
 	else
-		uuid[0] = '\0';
+		return DMP_NO_MATCH;
 
-	return 0;
+	return DMP_OK;
 }
 
 static int is_mpath_part(const char *part_name, const char *map_name)
@@ -1388,8 +1398,10 @@  struct multipath *dm_get_multipath(const char *name)
 	if (dm_get_map(name, &mpp->size, NULL) != DMP_OK)
 		goto out;
 
-	if (dm_get_wwid(name, mpp->wwid, WWID_SIZE) != 0)
+	if (dm_get_wwid(name, mpp->wwid, WWID_SIZE) != DMP_OK) {
 		condlog(2, "%s: failed to get uuid for %s", __func__, name);
+		mpp->wwid[0] = '\0';
+	}
 	if (dm_get_info(name, &mpp->dmi) != 0)
 		condlog(2, "%s: failed to get info for %s", __func__, name);
 
diff --git a/libmultipath/wwids.c b/libmultipath/wwids.c
index 7a4cb74..aac18c0 100644
--- a/libmultipath/wwids.c
+++ b/libmultipath/wwids.c
@@ -295,7 +295,7 @@  should_multipath(struct path *pp1, vector pathvec, vector mpvec)
 		struct multipath *mp = find_mp_by_wwid(mpvec, pp1->wwid);
 
 		if (mp != NULL &&
-		    dm_get_wwid(mp->alias, tmp_wwid, WWID_SIZE) == 0 &&
+		    dm_get_wwid(mp->alias, tmp_wwid, WWID_SIZE) == DMP_OK &&
 		    !strncmp(tmp_wwid, pp1->wwid, WWID_SIZE)) {
 			condlog(3, "wwid %s is already multipathed, keeping it",
 				pp1->wwid);
diff --git a/multipathd/main.c b/multipathd/main.c
index 442a154..1e7a6ac 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -726,8 +726,11 @@  add_map_without_path (struct vectors *vecs, const char *alias)
 		condlog(3, "%s: cannot access table", mpp->alias);
 		goto out;
 	}
-	if (!strlen(mpp->wwid))
-		dm_get_wwid(mpp->alias, mpp->wwid, WWID_SIZE);
+	if (!strlen(mpp->wwid) &&
+	    dm_get_wwid(mpp->alias, mpp->wwid, WWID_SIZE) != DMP_OK) {
+		condlog(3, "%s: cannot obtain WWID", mpp->alias);
+		goto out;
+	}
 	if (!strlen(mpp->wwid))
 		condlog(1, "%s: adding map with empty WWID", mpp->alias);
 	conf = get_multipath_config();
diff --git a/tests/alias.c b/tests/alias.c
index 1f78656..a95b308 100644
--- a/tests/alias.c
+++ b/tests/alias.c
@@ -84,7 +84,7 @@  int __wrap_dm_get_wwid(const char *name, char *uuid, int uuid_len)
 	check_expected(uuid_len);
 	assert_non_null(uuid);
 	ret = mock_type(int);
-	if (ret == 0)
+	if (ret == DMP_OK)
 		strcpy(uuid, mock_ptr_type(char *));
 	return ret;
 }
@@ -438,14 +438,14 @@  static void mock_unused_alias(const char *alias)
 {
 	expect_string(__wrap_dm_get_wwid, name, alias);
 	expect_value(__wrap_dm_get_wwid, uuid_len, WWID_SIZE);
-	will_return(__wrap_dm_get_wwid, 1);
+	will_return(__wrap_dm_get_wwid, DMP_NOT_FOUND);
 }
 
 static void mock_self_alias(const char *alias, const char *wwid)
 {
 	expect_string(__wrap_dm_get_wwid, name, alias);
 	expect_value(__wrap_dm_get_wwid, uuid_len, WWID_SIZE);
-	will_return(__wrap_dm_get_wwid, 0);
+	will_return(__wrap_dm_get_wwid, DMP_OK);
 	will_return(__wrap_dm_get_wwid, wwid);
 }
 
@@ -471,14 +471,14 @@  static void mock_self_alias(const char *alias, const char *wwid)
 	do {								\
 		expect_string(__wrap_dm_get_wwid, name, alias);		\
 		expect_value(__wrap_dm_get_wwid, uuid_len, WWID_SIZE);	\
-		will_return(__wrap_dm_get_wwid, 1);			\
+		will_return(__wrap_dm_get_wwid, DMP_NOT_FOUND);		\
 	} while (0)
 
 #define mock_used_alias(alias, wwid)					\
 	do {								\
 		expect_string(__wrap_dm_get_wwid, name, alias);		\
 		expect_value(__wrap_dm_get_wwid, uuid_len, WWID_SIZE);	\
-		will_return(__wrap_dm_get_wwid, 0);			\
+		will_return(__wrap_dm_get_wwid, DMP_OK);		\
 		will_return(__wrap_dm_get_wwid, "WWID_USED");		\
 		expect_condlog(3, USED_STR(alias, wwid));		\
 	} while(0)