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

[Benjamin Marzinski <bmarzins@redhat.com>]

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