On Fri, 2022-04-01 at 22:40 -0500, Benjamin Marzinski wrote:
> > On Thu, Mar 31, 2022 at 12:15:02AM +0200, mwilck@xxxxxxxx wrote:
> > uevent merging by WWID relies on the uid_attrs config option. As we
> > drop struct config between calls to uevent_merge(), we must be sure
> > that the WWID is not changed in reconfigure().
> >
> > Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
> > ---
> > multipath/multipath.conf.5 | 2 ++
> > multipathd/main.c | 53 +++++++++++++++++++++++++++++++---
> > ----
> > 2 files changed, 46 insertions(+), 9 deletions(-)
> >
> > diff --git a/multipath/multipath.conf.5
> > b/multipath/multipath.conf.5
> > index 605b46e..a9cd776 100644
> > --- a/multipath/multipath.conf.5
> > +++ b/multipath/multipath.conf.5
> > @@ -264,6 +264,8 @@ If this option is configured and matches the
> > device
> > node name of a device, it overrides any other configured methods
> > for
> > determining the WWID for this device.
> > .PP
> > +This option cannot be changed during runtime with the multipathd
> > \fBreconfigure\fR command.
> > +.PP
> > The default is: \fB<unset>\fR. To enable uevent merging, set it
> > e.g. to
> > \(dqsd:ID_SERIAL dasd:ID_UID nvme:ID_WWN\(dq.
> > .RE
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 13b1948..f514b32 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -2835,11 +2835,52 @@ void rcu_free_config(struct rcu_head *head)
> > free_config(conf);
> > }
> >
> > +static bool reconfigure_check_uid_attrs(const struct _vector
> > *old_attrs,
> > + const struct _vector
> > *new_attrs)
> > +{
> > + int i;
> > + char *old;
> > +
> > + if (VECTOR_SIZE(old_attrs) != VECTOR_SIZE(new_attrs))
> > + return true;
> > +
> > + vector_foreach_slot(old_attrs, old, i) {
> > + char *new = VECTOR_SLOT(new_attrs, i);
> > +
> > + if (strcmp(old, new))
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +static void reconfigure_check(struct config *old, struct config
> > *new)
> > +{
> > + int old_marginal_pathgroups;
> > +
> > + old_marginal_pathgroups = old->marginal_pathgroups;
> > + if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) !=
> > + (new->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN))
> > {
> > + condlog(1, "multipathd must be restarted to turn %s
> > fpin marginal paths",
> > + (old_marginal_pathgroups ==
> > MARGINAL_PATHGROUP_FPIN)?
> > + "off" : "on");
> > + new->marginal_pathgroups = old_marginal_pathgroups;
> > + }
> > +
> > + if (reconfigure_check_uid_attrs(&old->uid_attrs, &new-
> > >uid_attrs)) {
> > + struct _vector v = new->uid_attrs;
> > +
> > + condlog(1, "multipathd must be restarted to change
> > uid_attrs, keeping old values");
> > + new->uid_attrs = old->uid_attrs;
> > + vector_reset(&v);
>
> This leaks the strings that v points to, right?
> This also can happen in
> uid_attrs_handler(), I just noticed.
I was wondering about the same thing. But vector_reset() calls free()
on every slot. So no, I don't think anything is leaked here. The API
behaves admittedly in a surprising manner.
Martin
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
