[dm-devel] [PATCH 69/74] libmultipath: disassemble_map(): get rid of "is_daemon" argument

[Benjamin Marzinski]

On Wed, Aug 05, 2020 at 10:05:19PM +0200, Martin Wilck wrote:
> On Sun, 2020-07-19 at 00:26 -0500, Benjamin Marzinski wrote:
> > On Thu, Jul 09, 2020 at 12:51:40PM +0200, mwilck at suse.com wrote:
> > > From: Martin Wilck <mwilck at suse.com>
> > > 
> > > The reason for the is_daemon parameter in disassemble_map() lies
> > > deep in multipath-tools' past, in b96dead ("[multipathd] remove the
> > > retry login in uev_remove_path()"): By not adding paths from
> > > disassembled maps to the pathvec, we avoided to re-add removed
> > > paths on
> > > future map reloads (logic in update_mpp_paths()). As we can handle
> > > this with
> > > INIT_REMOVED now, we don't need to distinguish daemon and non-
> > > daemon
> > > mode any more. This fixes a memory leak, because only paths which
> > > are in
> > > pathvec will be freed on program exit.
> > 
> > I don't have any problems with the code in this patch. I just want to
> > reiterate that I don't think that multipathd should automatically be
> > adding paths, just because they were in a multipath device.  In my
> > opinion, multipathd should have the final decision as to what a
> > multipath device should look like.  If it sees an unexpected path,
> > and
> > runs pathinfo on it, and finds that that path does belong, it's fine
> > to
> > add it. But if pathinfo says that the path doesn't belong, multipathd
> > shouldn't add it to the pathvec. It should instead trigger a reload
> > of
> > the device to remove path.
> 
> Got it. I commented in my reply to 65/74. I'll repost this part with
> the minor issues you raised fixed (hopefully). Then let's review /
> discuss this again. If we agree on your PoV, we can probably ditch the
> whole INIT_REMOVED part of my series.

Sure.

> I hope you agree that "if (!is_daemon)" so deep in libmultipath
> is ugly and should be replaced by something cleaner.

That really never bothered me, but I see your point.
 
> We should take the opportunity to agree on a definition on the exact
> semantics of pathvec, i.e. which devices should be members of pathvec,
> and which ones shouldn't. I don't see a clear, consequent definition
> currently.

I think that you're correct that all paths that multipathd is dealing
with should be on the pathvec. Obviously paths that are only accessable
via a local variable are fine, but code shouldn't be dropping the vecs
lock with a path that is accessable globally (for instance from
mpp->paths) but not on the pathvec. What disassemble_map() has been
doing is definitely wrong.

-Ben

> Martin
>