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

[Martin Wilck]

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.

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

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.

Martin