On Wed, 2024-07-10 at 23:38 -0400, Benjamin Marzinski wrote:
> On Tue, Jul 09, 2024 at 11:39:22PM +0200, Martin Wilck wrote:
> > Export it, as it will be used by multipathd and libmpathpersist.
> >
>
> This is fine. But I was thinking that if you wanted to actually add
> some optional flags to the flags argument of libmp_mapinfo(), this
> would
> be a good use. You could have a flag like DM_MAP_MPATH_WWID, which
> would
> verify that the device had a multipath UUID_PREFIX (and possibly
> force a
> check for a multipath target type) and then return the wwid without
> the
> multipath uuid prefix.
>
> Just a thought.
Interesting idea. My thinking was that libmp_mapinfo() should only be
an interface to libdevmapper, and should not interpret the data it
returns in any way.
The only exception is the target type. In theory, the function could
just have returned the target type, and left the interpretation to the
caller. But then, it would have to perform several strdup()s for the
results, and the caller would have to free() the data it wasn't
interested in, which would lead to clumsy and inefficent code in the
frequent case that we're just interested in "multipath" or "linear"
targets. That's why I decided to pass the tgt_type field in, and return
DMP_NO_MATCH if it didn't match, even though it's arguably odd in the
mapinfo_t structure, which is otherwise only used for output fields.
Your flag idea would work, too (an earlier version of this patch set
used flags to indicate which output fields should be filled in; I
discarded that because it was duplicated information, and more elegant
to simply check it the respective output field was non-NULL).
Instead of tgt_type, we could also pass a "filter" callback function
with which the caller could determine whether or not it was interested
in a given map. But I wonder if that'd be over-engineered.
Whatever we eventually do, I think the is_mpath_uuid() helper will be
useful. I am not sure whether we can unexport it even if we implement
more sophisticated "filtering" in libmp_mapinfo().
In general, I wonder what multipath-tools should do if it encounters a
map that has "multipath" target type, but the WWID of which doesn't
conform to the mpath-XYZ convention, or vice versa. This can only
happen if another tool like dmsetup had been used to create the map.
IMO, if multipathd encounters such a situation, it should warn about it
and keep its fingers off the map in question. But we might also play it
simple and just assume that multipath maps always adhere to our
convention. I don't think we currently behave consistently in this
respect.
AFAIU, both the WWID and the target type are immutable and can't be
altered without destroying the map completely (because "multipath" is
an "immutable target" in DM). Thus it should be sufficient to check the
WWID when a map is first encountered.
Thoughts?
Martin
> -Ben
>
> > Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> > ---
> > libmultipath/devmapper.c | 13 +++++++++----
> > libmultipath/devmapper.h | 2 ++
> > libmultipath/libmultipath.version | 1 +
> > 3 files changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> > index 806ffb8..5f6c0c8 100644
> > --- a/libmultipath/devmapper.c
> > +++ b/libmultipath/devmapper.c
> > @@ -829,6 +829,11 @@ int dm_get_map(const char *name, unsigned long
> > long *size, char **outparams)
> > }
> > }
> >
> > +bool is_mpath_uuid(const char uuid[DM_UUID_LEN])
> > +{
> > + return !strncmp(uuid, UUID_PREFIX, UUID_PREFIX_LEN);
> > +}
> > +
> > /**
> > * dm_get_wwid(): return WWID for a multipath map
> > * @returns:
> > @@ -845,7 +850,7 @@ int dm_get_wwid(const char *name, char *uuid,
> > int uuid_len)
> > if (rc != DMP_OK)
> > return rc;
> >
> > - if (!strncmp(tmp, UUID_PREFIX, UUID_PREFIX_LEN))
> > + if (is_mpath_uuid(tmp))
> > strlcpy(uuid, tmp + UUID_PREFIX_LEN, uuid_len);
> > else {
> > uuid[0] = '\0';
> > @@ -970,7 +975,7 @@ int dm_is_mpath(const char *name)
> > (mapid_t) { .str = name },
> > (mapinfo_t) { .uuid = uuid,
> > .tgt_type = TGT_MPATH });
> >
> > - if (rc != DMP_OK || strncmp(uuid, UUID_PREFIX,
> > UUID_PREFIX_LEN))
> > + if (rc != DMP_OK || !is_mpath_uuid(uuid))
> > return DM_IS_MPATH_NO;
> > else
> > return DM_IS_MPATH_YES;
> > @@ -1065,7 +1070,7 @@ int _dm_flush_map (const char *mapname, int
> > flags, int retries)
> > .uuid = uuid,
> > .tgt_type = TGT_MPATH,
> > .target = ¶ms }) != DMP_OK
> > - || strncmp(uuid, UUID_PREFIX, UUID_PREFIX_LEN))
> > + || !is_mpath_uuid(uuid))
> > return DM_FLUSH_OK; /* nothing to do */
> >
> > /* if the device currently has no partitions, do not
> > @@ -1309,7 +1314,7 @@ struct multipath *dm_get_multipath(const char
> > *name)
> > }) != DMP_OK)
> > return NULL;
> >
> > - if (strncmp(uuid, UUID_PREFIX, UUID_PREFIX_LEN))
> > + if (!is_mpath_uuid(uuid))
> > return NULL;
> >
> > strlcpy(mpp->wwid, uuid + UUID_PREFIX_LEN, sizeof(mpp-
> > >wwid));
> > diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> > index db5c5fd..a2b2837 100644
> > --- a/libmultipath/devmapper.h
> > +++ b/libmultipath/devmapper.h
> > @@ -131,6 +131,8 @@ enum {
> > DM_IS_MPATH_YES,
> > DM_IS_MPATH_ERR,
> > };
> > +
> > +bool is_mpath_uuid(const char uuid[DM_UUID_LEN]);
> > int dm_is_mpath(const char *name);
> >
> > enum {
> > diff --git a/libmultipath/libmultipath.version
> > b/libmultipath/libmultipath.version
> > index 7d3ff63..5b8f9e0 100644
> > --- a/libmultipath/libmultipath.version
> > +++ b/libmultipath/libmultipath.version
> > @@ -127,6 +127,7 @@ global:
> > init_foreign;
> > init_prio;
> > io_err_stat_handle_pathfail;
> > + is_mpath_uuid;
> > is_path_valid;
> > libmp_dm_task_create;
> > libmp_get_version;
> > --
> > 2.45.2
>
