On Wed, 2024-07-10 at 19:34 -0400, Benjamin Marzinski wrote:
> 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@xxxxxxxx>
> > ---
> > 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.
I am not sure about this. but I can change it. I guess it's a corner
case anyway, where there's no obvious right or wrong behavior.
A DM_DEVICE_INFO call with this map name failed with an error other
than ENXIO. By going on with this map name, we clearly risk that future
failures will happen when we try to create or reload the map in
question.
>
> > + 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.
Ok.
>
> > + } 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.
Right. In libmp_mapinfo(), I followed the idea that no output should be
touched if any errors happened. Here, I wanted to preserve the behavior
but didn't do it correctly.
I tend to think that "don't touch output in error case" is the correct
behavior, but we'd need to review the callers and make sure that they
don't assume the WWID will be automatically 0-terminated.
I'll do that.
Martin
