On Wed, 2023-09-06 at 17:42 -0500, Benjamin Marzinski wrote:
> On Fri, Sep 01, 2023 at 08:02:17PM +0200, mwilck@xxxxxxxx wrote:
> > From: Martin Wilck <mwilck@xxxxxxxx>
> >
> > If the bindings file is changed in a way that multipathd can't
> > handle
> > (e.g. by swapping the aliases of two maps), multipathd must not try
> > to re-use an alias that is already used by another map. Creating
> > or renaming a map with such an alias will fail. We already avoid
> > this for some cases, but not for all. Fix it.
> >
> > Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> > Cc: David Bond <dbond@xxxxxxxx>
> > ---
> > libmultipath/alias.c | 36 +++++++++++++++++++++++++++++++-----
> > tests/alias.c | 2 +-
> > 2 files changed, 32 insertions(+), 6 deletions(-)
> >
> > diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> > index 9b9b789..f7834d1 100644
> > --- a/libmultipath/alias.c
> > +++ b/libmultipath/alias.c
> > @@ -120,7 +120,7 @@ static bool alias_already_taken(const char
> > *alias, const char *map_wwid)
> > if (dm_get_uuid(alias, wwid, sizeof(wwid)) == 0 &&
> > strncmp(map_wwid, wwid, sizeof(wwid)) == 0)
> > return false;
> > - condlog(3, "%s: alias '%s' already taken, but not
> > in bindings file. reselecting alias",
> > + condlog(3, "%s: alias '%s' already taken,
> > reselecting alias",
> > map_wwid, alias);
> > return true;
> > }
> > @@ -363,28 +363,54 @@ char *get_user_friendly_alias(const char
> > *wwid, const char *file, const char *al
> > * allocated correctly
> > */
> > if (strcmp(buff, wwid) == 0) {
>
> I'm confused about this. We should only have alias_old set if there
> already exists a device that matches this WWID, right? That's what I
> though alias_old means. Am I missing some way that alias_old could
> get
> set to something other than the alias of an already existing device
> with
> a matching WWID? Otherwise, once we verify that that this mapping
> also
> exists in the bindings file, we should be fine to use it?
>
This is mainly meant as an additional consistency check, to make sure
that the logic in get_user_friendly_alias() is correct, no matter how
"alias_old" was set by the caller. Note that alias_old is ab^H^Hreused
by the ACT_RENAME action; it is not immediately obvious that alias_old
can never be set to an invalid value in get_user_friendly_alias().
condlog() prints "ERROR" here because it's a condition that shouldn't
occur.
> > - alias = strdup(alias_old);
> > + if (!alias_already_taken(alias_old, wwid))
> > + alias = strdup(alias_old);
> > + else
> > + condlog(0, "ERROR: old alias [%s]
> > for wwid [%s] is used by other map",
> > + alias_old, wwid);
> > goto out;
> > +
> > } else {
> > condlog(0, "alias %s already bound to wwid
> > %s, cannot reuse",
> > alias_old, buff);
> > - goto new_alias;
>
> extra semicolon.
>
> > + goto new_alias; ;
> > }
> > }
> >
> > + /*
> > + * Look for an existing alias in the bindings file.
> > + * Pass prefix = NULL, so lookup_binding() won't try to
> > allocate a new id.
> > + */
>
> There's no point in saving the return value here. We don't use if for
> anything.
>
> > id = lookup_binding(f, wwid, &alias, NULL, 0);
> > if (alias) {
> > - condlog(3, "Use existing binding [%s] for WWID
> > [%s]",
> > - alias, wwid);
> > + if (alias_already_taken(alias, wwid)) {
> > + free(alias);
> > + alias = NULL;
> > + } else
> > + condlog(3, "Use existing binding [%s] for
> > WWID [%s]",
> > + alias, wwid);
> > goto out;
> > }
> >
> > /* allocate the existing alias in the bindings file */
> > id = scan_devname(alias_old, prefix);
>
> Again, unless I'm overlooking something, I don't think we need to
> check if the alias is already taken here. Since we know that a device
> already exists with alias_old and the correct WWID, as long as
> alias_old
> is a valid user_friendly_name we can just use it.
Similar reasoning as above. We could perhaps remove these checks, but
we'd need to replace them by comments explaining why this condition
can't occur.
We could (and maybe should) move the call to find_existing_alias() from
add_map_with_path() to get_user_friendly_alias(), so that we have the
entire alias logic in a single place. The mpp->alias_old field would
then only be used for ACT_RENAME.
Martin
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
