[dm-devel] [PATCH 3/6] multipath: centralize validation code

[Martin Wilck]

On Mon, 2020-05-18 at 13:53 -0500, Benjamin Marzinski wrote:
> On Fri, May 15, 2020 at 08:37:16PM +0000, Martin Wilck wrote:
> > On Thu, 2020-05-14 at 20:59 -0500, Benjamin Marzinski wrote:
> > > This code pulls the multipath path validation code out of
> > > configure(),
> > > and puts it into its own function, check_path_valid(). This
> > > function
> > > calls a new libmultipath function, is_path_valid() to check just
> > > path
> > > requested. This seperation exists so that is_path_valid() can be
> > > reused
> > > by future code. This code will give almost the same answer as the
> > > existing code, with the exception that now, if a device is
> > > currently
> > > multipathed, it will always be a valid multipath path.
> > > 
> > > Signed-off-by: Benjamin Marzinski <bmarzins at redhat.com>
> > 
> > Great job getting the logic right! Readability massively improved.
> > Almost ack, a few comments and questions below.
> > 
> > Regards,
> > Martin
> > 
> > 

> > > -			conf->find_multipaths |= _FIND_MULTIPATHS_I;
> > > +			if (conf->find_multipaths == FIND_MULTIPATHS_ON
> > > +			    conf->find_multipaths ==
> > > FIND_MULTIPATHS_STRICT)
> > > +				conf->find_multipaths =
> > > FIND_MULTIPATHS_SMART;
> > > +			else if (conf->find_multipaths ==
> > > FIND_MULTIPATHS_OFF)
> > > +				conf->find_multipaths =
> > > FIND_MULTIPATHS_GREEDY;
> > 
> > Ok. Previously FIND_MULTIPATHS_SMART was not the same value as
> > FIND_MULTIPATHS_STRICT + _FIND_MULTIPATHS_I. Effectively, this
> > doesn't
> > change logic, but only because the check for ignore_new_devs_on()
> > in
> > should_multipath() is actually redundant. (IIRC in the past we'd
> > determined that "strict" + "ignore_wwids" makes no sense).
> > 
> 
> And I still feel like it doesn't make any sense, so this was
> intentional.  Are you arguing that we need that state, or are you
> just
> pointing this out?

Just pointing it out. That's why I wrote "Ok" in the first place. But I
stumbled on it at first sight, and took a while to realize that your
patch got it right. Which is worthwhile to note IMO, in order not to
stumble on it next time.

Sorry for not expressing that clearly enough.

Martin

-- 
Dr. Martin Wilck <mwilck at suse.com>, Tel. +49 (0)911 74053 2107
SUSE  Software Solutions Germany GmbH
HRB 36809, AG Nürnberg GF: Felix
Imendörffer