Re: [PATCH 23/44] libmultipath: improve dm_get_wwid() return value logic

[Benjamin Marzinski <bmarzins@redhat.com>]

On Tue, Jul 09, 2024 at 11:39:14PM +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.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/alias.c     |  5 +++--
>  libmultipath/configure.c | 23 +++++++++++------------
>  libmultipath/devmapper.c | 21 ++++++++++++++++-----
>  libmultipath/wwids.c     |  2 +-
>  tests/alias.c            | 10 +++++-----
>  5 files changed, 36 insertions(+), 25 deletions(-)
> 
> diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> index 10e58a7..2ab9499 100644
> --- a/libmultipath/alias.c
> +++ b/libmultipath/alias.c
> @@ -408,13 +408,14 @@ 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)

We used to assume the alias was not taken if dm_get_wwid() failed with
an error. Now we assume that the alias is taken. I think we should
continue to assume we can use the alias if we get DM_ERR.

> +	if (rc == DMP_NOT_FOUND)
>  		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..565ea5c 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -845,18 +845,17 @@ 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;

If we return DMP_NO_MATCH, we'll tell the user that the device is already
present will a WWID of "". This is the same as before, but perhaps it
would be better to tell the user that the alias is in use by a
non-multipath device.

> +		} else if (rc == DMP_OK || rc == DMP_NO_MATCH) {
> +			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;
>  		}
>  	}
>  
> @@ -1320,7 +1319,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 94ef369..1eebcb5 100644
> --- a/libmultipath/devmapper.c
> +++ b/libmultipath/devmapper.c
> @@ -829,19 +829,30 @@ 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
> + */
>  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
> +	else {

This seems a little inconsistent with the change in our returns.
Before, if we found a device but it wasn't multipath device,
dm_get_wwid() would exit with 0 but set uuid to an empty string. Now we
return DMP_NO_MATCH in this case but we still clear uuid. It seems like
perhaps we should either set uuid to an empty string for all cases
except DM_OK or we should not touch it for all cases except DM_OK.
Alternatively, perhaps I'm over-thinking this.

-Ben 

>  		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)
> @@ -1377,7 +1388,7 @@ 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);
>  	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/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