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 <[email protected]>
> Signed-off-by: Martin Wilck <[email protected]>
> ---
> 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
