[PATCH 0/2] multipath-tools: hwhandler autodetection

[PATCH 0/2] multipath-tools: hwhandler autodetection

[Martin Wilck]

This is patch set is a follow-up of the late discussion
"Can we drop 'hardware_handler "1 alua"'?". With patch 1/2 in place,
I think we can - multipath uses the TPGS flag to determine whether
or not the paths support ALUA, and sets the hwhandler accordingly.
Unlike the "detect_checker" and "detect_prio" options, I have coded this
so that explicit "hardware_hander" settings take priority over autodetection,
except for the case in which we determine that TPGS is unsupported.

Thus I don't think an additional "detect_hwhandler" config option is
necessary.

To be actually effective to solve the problems with certain types of IBM IPR
controllers, patch 1/2 must be combined with my previous 
patch "libmultipath: don't reject maps with undefined prio".

Patch 2/2 is just a cleanup that looked logical to do after 1/2.

Martin Wilck (2):
  libmultipath: hwhandler auto-detection for ALUA
  libmultipath/propsel: (re)use static const vars for origin

 libmultipath/propsel.c | 187 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 114 insertions(+), 73 deletions(-)

-- 
2.16.1

[PATCH 1/2] libmultipath: hwhandler auto-detection for ALUA

[Martin Wilck]

If the hardware handler isn't explicitly set, infer ALUA support
from the pp->tpgs attribute. Likewise, if ALUA is selected, but
not supported by the hardware, fall back to no hardware handler.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/propsel.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index 93974a482336..dc24450eb775 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -43,10 +43,13 @@ do {									\
 		goto out;						\
 	}								\
 } while(0)
+
+static char default_origin[] = "(setting: multipath internal)";
+
 #define do_default(dest, value)						\
 do {									\
 	dest = value;							\
-	origin = "(setting: multipath internal)";			\
+	origin = default_origin;					\
 } while(0)
 
 #define mp_set_mpe(var)							\
@@ -373,16 +376,20 @@ static int get_dh_state(struct path *pp, char *value, size_t value_len)
 
 int select_hwhandler(struct config *conf, struct multipath *mp)
 {
-	char *origin;
+	const char *origin;
 	struct path *pp;
 	/* dh_state is no longer than "detached" */
 	char handler[12];
+	static char alua_name[] = "1 alua";
+	static const char tpgs_origin[]= "(setting: autodetected from TPGS)";
 	char *dh_state;
 	int i;
+	bool all_tpgs = true;
 
 	dh_state = &handler[2];
 	if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) {
 		vector_foreach_slot(mp->paths, pp, i) {
+			all_tpgs = all_tpgs && (pp->tpgs > 0);
 			if (get_dh_state(pp, dh_state, sizeof(handler) - 2) > 0
 			    && strcmp(dh_state, "detached")) {
 				memcpy(handler, "1 ", 2);
@@ -397,6 +404,14 @@ int select_hwhandler(struct config *conf, struct multipath *mp)
 	mp_set_conf(hwhandler);
 	mp_set_default(hwhandler, DEFAULT_HWHANDLER);
 out:
+	if (all_tpgs && !strcmp(mp->hwhandler, DEFAULT_HWHANDLER) &&
+		origin == default_origin) {
+		mp->hwhandler = alua_name;
+		origin = tpgs_origin;
+	} else if (!all_tpgs && !strcmp(mp->hwhandler, alua_name)) {
+		mp->hwhandler = DEFAULT_HWHANDLER;
+		origin = tpgs_origin;
+	}
 	mp->hwhandler = STRDUP(mp->hwhandler);
 	condlog(3, "%s: hardware_handler = \"%s\" %s", mp->alias, mp->hwhandler,
 		origin);
-- 
2.16.1

Re: [PATCH 1/2] libmultipath: hwhandler auto-detection for ALUA

[Benjamin Marzinski]

On Tue, Mar 27, 2018 at 11:50:52PM +0200, Martin Wilck wrote:
> If the hardware handler isn't explicitly set, infer ALUA support
> from the pp->tpgs attribute. Likewise, if ALUA is selected, but
> not supported by the hardware, fall back to no hardware handler.

Weren't you worried before about temporary ALUA failures? If you had a
temporary failure while configuring a device that you explicitly set to
be ALUA, then this would cause the device to be misconfigured? If the
hardware handler isn't set, inferring ALUA is fine. But what is the case
where we want to say that a device that is explicitly set to ALUA
shouldn't actually be ALUA?  It seem like if there is some uncertaintly,
we should just not set the hardware handler, and allow multipath to
infer it via the pp->tpgs value.

I'm not strongly against this patch. I just don't see the value in
overriding an explicit configuration, if we believe that temporary
failures are possible.

-Ben

> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/propsel.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index 93974a482336..dc24450eb775 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -43,10 +43,13 @@ do {									\
>  		goto out;						\
>  	}								\
>  } while(0)
> +
> +static char default_origin[] = "(setting: multipath internal)";
> +
>  #define do_default(dest, value)						\
>  do {									\
>  	dest = value;							\
> -	origin = "(setting: multipath internal)";			\
> +	origin = default_origin;					\
>  } while(0)
>  
>  #define mp_set_mpe(var)							\
> @@ -373,16 +376,20 @@ static int get_dh_state(struct path *pp, char *value, size_t value_len)
>  
>  int select_hwhandler(struct config *conf, struct multipath *mp)
>  {
> -	char *origin;
> +	const char *origin;
>  	struct path *pp;
>  	/* dh_state is no longer than "detached" */
>  	char handler[12];
> +	static char alua_name[] = "1 alua";
> +	static const char tpgs_origin[]= "(setting: autodetected from TPGS)";
>  	char *dh_state;
>  	int i;
> +	bool all_tpgs = true;
>  
>  	dh_state = &handler[2];
>  	if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) {
>  		vector_foreach_slot(mp->paths, pp, i) {
> +			all_tpgs = all_tpgs && (pp->tpgs > 0);
>  			if (get_dh_state(pp, dh_state, sizeof(handler) - 2) > 0
>  			    && strcmp(dh_state, "detached")) {
>  				memcpy(handler, "1 ", 2);
> @@ -397,6 +404,14 @@ int select_hwhandler(struct config *conf, struct multipath *mp)
>  	mp_set_conf(hwhandler);
>  	mp_set_default(hwhandler, DEFAULT_HWHANDLER);
>  out:
> +	if (all_tpgs && !strcmp(mp->hwhandler, DEFAULT_HWHANDLER) &&
> +		origin == default_origin) {
> +		mp->hwhandler = alua_name;
> +		origin = tpgs_origin;
> +	} else if (!all_tpgs && !strcmp(mp->hwhandler, alua_name)) {
> +		mp->hwhandler = DEFAULT_HWHANDLER;
> +		origin = tpgs_origin;
> +	}
>  	mp->hwhandler = STRDUP(mp->hwhandler);
>  	condlog(3, "%s: hardware_handler = \"%s\" %s", mp->alias, mp->hwhandler,
>  		origin);
> -- 
> 2.16.1

Re: [PATCH 1/2] libmultipath: hwhandler auto-detection for ALUA

[Martin Wilck]

On Tue, 2018-04-03 at 15:31 -0500, Benjamin Marzinski wrote:
> On Tue, Mar 27, 2018 at 11:50:52PM +0200, Martin Wilck wrote:
> > If the hardware handler isn't explicitly set, infer ALUA support
> > from the pp->tpgs attribute. Likewise, if ALUA is selected, but
> > not supported by the hardware, fall back to no hardware handler.
> 
> Weren't you worried before about temporary ALUA failures? If you had
> a
> temporary failure while configuring a device that you explicitly set
> to
> be ALUA, then this would cause the device to be misconfigured? 

I believe that if TGPS is 0, the device will never be able to support
ALUA. The kernel also looks at the TPGS bits and won't try ALUA if they
are unset. Once the device is configured and actual ALUA RTPG/STPG
calls are performed, they may fail for a variety of temporary reasons -
I wanted to avoid resetting the prio algorithm to "const" for such
cases. That's my understanding, correct me if I'm wrong.

> If the
> hardware handler isn't set, inferring ALUA is fine. But what is the
> case
> where we want to say that a device that is explicitly set to ALUA
> shouldn't actually be ALUA?  It seem like if there is some
> uncertaintly,
> we should just not set the hardware handler, and allow multipath to
> infer it via the pp->tpgs value.
> 
> I'm not strongly against this patch. I just don't see the value in
> overriding an explicit configuration, if we believe that temporary
> failures are possible.

That would be fine if we didn't have any explicit "hardware_handler
alua" settings in the hardcoded hwtable any more, or at least if we're 
positive that those devices where we have "hardware_handler alua"
really support it.

We can also adopt the philosophy of "detect_prio" and "detect_checker",
add an additional config file option "detect_hwhandler", and look at
tpgs only if the latter it set (which would be the default). Like
detect_prio, users could then enforce their config file settings with
"detect_hwhandler no".

I was hoping we could find a simpler approach, without yet another
rarely-used config option.

Btw, at SUSE we solved our problem with the controller at hand by
simply removing "hardware_handler alua" and "prio alua" from the IBM
IPR entry. If the scsi_dh_alua module is loaded early (default on
SUSE), this results in ALUA hwhandler and sysfs prio being used for IPR
controllers that do support ALUA, and no hwhandler / const prio =
PRIO_UNDEF for those that don't. I'm not sure if that simple solution
suits upstream, because upstream doesn't enforce early loading of
device handler modules.

Regards,
Martin


> 
> -Ben
> 
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/propsel.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> > index 93974a482336..dc24450eb775 100644
> > --- a/libmultipath/propsel.c
> > +++ b/libmultipath/propsel.c
> > @@ -43,10 +43,13 @@ do {						
> > 			\
> >  		goto out;						
> > \
> >  	}								
> > \
> >  } while(0)
> > +
> > +static char default_origin[] = "(setting: multipath internal)";
> > +
> >  #define do_default(dest, value)					
> > 	\
> >  do {								
> > 	\
> >  	dest = value;						
> > 	\
> > -	origin = "(setting: multipath internal)";			
> > \
> > +	origin = default_origin;					
> > \
> >  } while(0)
> >  
> >  #define mp_set_mpe(var)						
> > 	\
> > @@ -373,16 +376,20 @@ static int get_dh_state(struct path *pp, char
> > *value, size_t value_len)
> >  
> >  int select_hwhandler(struct config *conf, struct multipath *mp)
> >  {
> > -	char *origin;
> > +	const char *origin;
> >  	struct path *pp;
> >  	/* dh_state is no longer than "detached" */
> >  	char handler[12];
> > +	static char alua_name[] = "1 alua";
> > +	static const char tpgs_origin[]= "(setting: autodetected
> > from TPGS)";
> >  	char *dh_state;
> >  	int i;
> > +	bool all_tpgs = true;
> >  
> >  	dh_state = &handler[2];
> >  	if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) {
> >  		vector_foreach_slot(mp->paths, pp, i) {
> > +			all_tpgs = all_tpgs && (pp->tpgs > 0);
> >  			if (get_dh_state(pp, dh_state,
> > sizeof(handler) - 2) > 0
> >  			    && strcmp(dh_state, "detached")) {
> >  				memcpy(handler, "1 ", 2);
> > @@ -397,6 +404,14 @@ int select_hwhandler(struct config *conf,
> > struct multipath *mp)
> >  	mp_set_conf(hwhandler);
> >  	mp_set_default(hwhandler, DEFAULT_HWHANDLER);
> >  out:
> > +	if (all_tpgs && !strcmp(mp->hwhandler, DEFAULT_HWHANDLER)
> > &&
> > +		origin == default_origin) {
> > +		mp->hwhandler = alua_name;
> > +		origin = tpgs_origin;
> > +	} else if (!all_tpgs && !strcmp(mp->hwhandler, alua_name))
> > {
> > +		mp->hwhandler = DEFAULT_HWHANDLER;
> > +		origin = tpgs_origin;
> > +	}
> >  	mp->hwhandler = STRDUP(mp->hwhandler);
> >  	condlog(3, "%s: hardware_handler = \"%s\" %s", mp->alias,
> > mp->hwhandler,
> >  		origin);
> > -- 
> > 2.16.1
> 
> 

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 1/2] libmultipath: hwhandler auto-detection for ALUA

[Benjamin Marzinski]

On Tue, Apr 03, 2018 at 10:53:29PM +0200, Martin Wilck wrote:
> On Tue, 2018-04-03 at 15:31 -0500, Benjamin Marzinski wrote:
> > On Tue, Mar 27, 2018 at 11:50:52PM +0200, Martin Wilck wrote:
> > > If the hardware handler isn't explicitly set, infer ALUA support
> > > from the pp->tpgs attribute. Likewise, if ALUA is selected, but
> > > not supported by the hardware, fall back to no hardware handler.
> > 
> > Weren't you worried before about temporary ALUA failures? If you had
> > a
> > temporary failure while configuring a device that you explicitly set
> > to
> > be ALUA, then this would cause the device to be misconfigured? 
> 
> I believe that if TGPS is 0, the device will never be able to support
> ALUA. The kernel also looks at the TPGS bits and won't try ALUA if they
> are unset. Once the device is configured and actual ALUA RTPG/STPG
> calls are performed, they may fail for a variety of temporary reasons -
> I wanted to avoid resetting the prio algorithm to "const" for such
> cases. That's my understanding, correct me if I'm wrong.

Devices that were not correctly supporing ALUA returned > 0 for
get_target_port_group_support, so detect_alua actually does all the work
necessary to verify that it can get a priority. Without doing this,
multiple deviecs that didn't support ALUA were being detected as
supporting ALUA.

> 
> > If the
> > hardware handler isn't set, inferring ALUA is fine. But what is the
> > case
> > where we want to say that a device that is explicitly set to ALUA
> > shouldn't actually be ALUA?  It seem like if there is some
> > uncertaintly,
> > we should just not set the hardware handler, and allow multipath to
> > infer it via the pp->tpgs value.
> > 
> > I'm not strongly against this patch. I just don't see the value in
> > overriding an explicit configuration, if we believe that temporary
> > failures are possible.
> 
> That would be fine if we didn't have any explicit "hardware_handler
> alua" settings in the hardcoded hwtable any more, or at least if we're 
> positive that those devices where we have "hardware_handler alua"
> really support it.
> 
> We can also adopt the philosophy of "detect_prio" and "detect_checker",
> add an additional config file option "detect_hwhandler", and look at
> tpgs only if the latter it set (which would be the default). Like
> detect_prio, users could then enforce their config file settings with
> "detect_hwhandler no".
> 
> I was hoping we could find a simpler approach, without yet another
> rarely-used config option.
> 
> Btw, at SUSE we solved our problem with the controller at hand by
> simply removing "hardware_handler alua" and "prio alua" from the IBM
> IPR entry. If the scsi_dh_alua module is loaded early (default on
> SUSE), this results in ALUA hwhandler and sysfs prio being used for IPR
> controllers that do support ALUA, and no hwhandler / const prio =
> PRIO_UNDEF for those that don't. I'm not sure if that simple solution
> suits upstream, because upstream doesn't enforce early loading of
> device handler modules.

By using retain_attached_hwhandler at all, we are implicitly requiring
the scsi_dh_alua module to be loaded before devices with indeterminate
configurations are discovered for them to work correctly. right? For
instance, commit 715c48d93dd00930534ce6a55d0e3705466df5d6 did this for
netapp devices, and that was in 2013. I don't see how this is different.

-Ben

> Regards,
> Martin
> 
> 
> > 
> > -Ben
> > 
> > > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  libmultipath/propsel.c | 19 +++++++++++++++++--
> > >  1 file changed, 17 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> > > index 93974a482336..dc24450eb775 100644
> > > --- a/libmultipath/propsel.c
> > > +++ b/libmultipath/propsel.c
> > > @@ -43,10 +43,13 @@ do {						
> > > 			\
> > >  		goto out;						
> > > \
> > >  	}								
> > > \
> > >  } while(0)
> > > +
> > > +static char default_origin[] = "(setting: multipath internal)";
> > > +
> > >  #define do_default(dest, value)					
> > > 	\
> > >  do {								
> > > 	\
> > >  	dest = value;						
> > > 	\
> > > -	origin = "(setting: multipath internal)";			
> > > \
> > > +	origin = default_origin;					
> > > \
> > >  } while(0)
> > >  
> > >  #define mp_set_mpe(var)						
> > > 	\
> > > @@ -373,16 +376,20 @@ static int get_dh_state(struct path *pp, char
> > > *value, size_t value_len)
> > >  
> > >  int select_hwhandler(struct config *conf, struct multipath *mp)
> > >  {
> > > -	char *origin;
> > > +	const char *origin;
> > >  	struct path *pp;
> > >  	/* dh_state is no longer than "detached" */
> > >  	char handler[12];
> > > +	static char alua_name[] = "1 alua";
> > > +	static const char tpgs_origin[]= "(setting: autodetected
> > > from TPGS)";
> > >  	char *dh_state;
> > >  	int i;
> > > +	bool all_tpgs = true;
> > >  
> > >  	dh_state = &handler[2];
> > >  	if (mp->retain_hwhandler != RETAIN_HWHANDLER_OFF) {
> > >  		vector_foreach_slot(mp->paths, pp, i) {
> > > +			all_tpgs = all_tpgs && (pp->tpgs > 0);
> > >  			if (get_dh_state(pp, dh_state,
> > > sizeof(handler) - 2) > 0
> > >  			    && strcmp(dh_state, "detached")) {
> > >  				memcpy(handler, "1 ", 2);
> > > @@ -397,6 +404,14 @@ int select_hwhandler(struct config *conf,
> > > struct multipath *mp)
> > >  	mp_set_conf(hwhandler);
> > >  	mp_set_default(hwhandler, DEFAULT_HWHANDLER);
> > >  out:
> > > +	if (all_tpgs && !strcmp(mp->hwhandler, DEFAULT_HWHANDLER)
> > > &&
> > > +		origin == default_origin) {
> > > +		mp->hwhandler = alua_name;
> > > +		origin = tpgs_origin;
> > > +	} else if (!all_tpgs && !strcmp(mp->hwhandler, alua_name))
> > > {
> > > +		mp->hwhandler = DEFAULT_HWHANDLER;
> > > +		origin = tpgs_origin;
> > > +	}
> > >  	mp->hwhandler = STRDUP(mp->hwhandler);
> > >  	condlog(3, "%s: hardware_handler = \"%s\" %s", mp->alias,
> > > mp->hwhandler,
> > >  		origin);
> > > -- 
> > > 2.16.1
> > 
> > 
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)

Re: [PATCH 1/2] libmultipath: hwhandler auto-detection for ALUA

[Martin Wilck]

On Tue, 2018-04-03 at 16:29 -0500, Benjamin Marzinski wrote:
> On Tue, Apr 03, 2018 at 10:53:29PM +0200, Martin Wilck wrote:
> > On Tue, 2018-04-03 at 15:31 -0500, Benjamin Marzinski wrote:
> > > On Tue, Mar 27, 2018 at 11:50:52PM +0200, Martin Wilck wrote:
> > > > If the hardware handler isn't explicitly set, infer ALUA
> > > > support
> > > > from the pp->tpgs attribute. Likewise, if ALUA is selected, but
> > > > not supported by the hardware, fall back to no hardware
> > > > handler.
> > > 
> > > Weren't you worried before about temporary ALUA failures? If you
> > > had
> > > a
> > > temporary failure while configuring a device that you explicitly
> > > set
> > > to
> > > be ALUA, then this would cause the device to be misconfigured? 
> > 
> > I believe that if TGPS is 0, the device will never be able to
> > support
> > ALUA. The kernel also looks at the TPGS bits and won't try ALUA if
> > they
> > are unset. Once the device is configured and actual ALUA RTPG/STPG
> > calls are performed, they may fail for a variety of temporary
> > reasons -
> > I wanted to avoid resetting the prio algorithm to "const" for such
> > cases. That's my understanding, correct me if I'm wrong.
> 
> Devices that were not correctly supporing ALUA returned > 0 for
> get_target_port_group_support, so detect_alua actually does all the
> work
> necessary to verify that it can get a priority. Without doing this,
> multiple deviecs that didn't support ALUA were being detected as
> supporting ALUA.

So, detect_alua() tests TPGS *and* tries and actual alua call, and sets
pp->tpgs to anything other than TPGS_NONE only if the latter is
successful. That's fine. My patch was looking at pp->tpgs, so it was
implicitly using this logic of detect_alua(). But does that guarantee
that future alua->getprio() calls will never fail at some later point
in time?

Maybe I misunderstood your original proposition. What I'm saying is
that resetting the prio algorithm from "alua" to "const" because of an
error code in get_prio() is wrong, because that error code may be
transient.

If we give "hardware_handler" config options preference over ALUA
autodetection, and thus enforce hwhandler "1 alua" on such devices that
have no ALUA support, domap() is guaranteed to fail, because the kernel
refuses to set up a map with a given hwhandler if any device doesn't
support that handler.

> By using retain_attached_hwhandler at all, we are implicitly
> requiring
> the scsi_dh_alua module to be loaded before devices with
> indeterminate
> configurations are discovered for them to work correctly. right? For
> instance, commit 715c48d93dd00930534ce6a55d0e3705466df5d6 did this
> for
> netapp devices, and that was in 2013. I don't see how this is
> different.

You're right, we are "implicitly requiring" this sort-of, but we have
no code that enforces the early loading of the device handlers. We
should be shipping a modules-load.d file, or a modprobe.d softdep, or
something similar that would enforce this setting if we _really_ depend
on it. "Implicit requirements" are bad. We should either make the
requirement explicit, or not hard-depend on it. So far I was thinking
the latter. After all, SCSI device-handler support is configurable in
the kernel.

Regards,
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 1/2] libmultipath: hwhandler auto-detection for ALUA

[Martin Wilck]

Hi Ben,

On Wed, 2018-04-04 at 10:04 +0200, Martin Wilck wrote:
> On Tue, 2018-04-03 at 16:29 -0500, Benjamin Marzinski wrote:

> > > I believe that if TGPS is 0, the device will never be able to
> > > support
> > > ALUA. The kernel also looks at the TPGS bits and won't try ALUA
> > > if
> > > they
> > > are unset. Once the device is configured and actual ALUA
> > > RTPG/STPG
> > > calls are performed, they may fail for a variety of temporary
> > > reasons -
> > > I wanted to avoid resetting the prio algorithm to "const" for
> > > such
> > > cases. That's my understanding, correct me if I'm wrong.
> > 
> > Devices that were not correctly supporing ALUA returned > 0 for
> > get_target_port_group_support, so detect_alua actually does all the
> > work
> > necessary to verify that it can get a priority. Without doing this,
> > multiple deviecs that didn't support ALUA were being detected as
> > supporting ALUA.
> 
> So, detect_alua() tests TPGS *and* tries and actual alua call, and
> sets
> pp->tpgs to anything other than TPGS_NONE only if the latter is
> successful. That's fine. My patch was looking at pp->tpgs, so it was
> implicitly using this logic of detect_alua(). But does that guarantee
> that future alua->getprio() calls will never fail at some later point
> in time?
> 
> Maybe I misunderstood your original proposition. What I'm saying is
> that resetting the prio algorithm from "alua" to "const" because of
> an
> error code in get_prio() is wrong, because that error code may be
> transient.
> 
> If we give "hardware_handler" config options preference over ALUA
> autodetection, and thus enforce hwhandler "1 alua" on such devices
> that
> have no ALUA support, domap() is guaranteed to fail, because the
> kernel
> refuses to set up a map with a given hwhandler if any device doesn't
> support that handler.
> 
> > By using retain_attached_hwhandler at all, we are implicitly
> > requiring
> > the scsi_dh_alua module to be loaded before devices with
> > indeterminate
> > configurations are discovered for them to work correctly. right?
> > For
> > instance, commit 715c48d93dd00930534ce6a55d0e3705466df5d6 did this
> > for
> > netapp devices, and that was in 2013. I don't see how this is
> > different.
> 
> You're right, we are "implicitly requiring" this sort-of, but we have
> no code that enforces the early loading of the device handlers. We
> should be shipping a modules-load.d file, or a modprobe.d softdep, or
> something similar that would enforce this setting if we _really_
> depend
> on it. "Implicit requirements" are bad. We should either make the
> requirement explicit, or not hard-depend on it. So far I was thinking
> the latter. After all, SCSI device-handler support is configurable in
> the kernel.

I'm unsure what to do. Do you still reject my patch? Or have you been
convinced by Hannes and my arguments? 
Or are you requesting changes? If yes, what? 

Regards,
Martin

-- 
Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [PATCH 1/2] libmultipath: hwhandler auto-detection for ALUA

[Benjamin Marzinski]

On Thu, Apr 12, 2018 at 05:43:39PM +0200, Martin Wilck wrote:
> Hi Ben,
> 
> I'm unsure what to do. Do you still reject my patch? Or have you been
> convinced by Hannes and my arguments? 
> Or are you requesting changes? If yes, what? 

I still feel that it's better to make the default config const for
devices that may or may not be ALUA, and let detect_alua figure it out,
rather than allowing multipathd to override a specifically requested
ALUA hardware handler. This is especially true if
get_target_port_group_support() and get_target_port_group succeed, but
get_asymmetric_access_state() fails in detect_alua().  But I don't think
that transient alua errors like this are very likely during multpath
creation, so I not going to reject the patch.

Reviewed-by: Benjmain Marzinski <bmarzins@redhat.com>

> 
> Regards,
> Martin
> 
> -- 
> Dr. Martin Wilck <mwilck@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)

Re: [PATCH 1/2] libmultipath: hwhandler auto-detection for ALUA

[Hannes Reinecke]

On Tue, 3 Apr 2018 15:31:32 -0500
"Benjamin Marzinski" <bmarzins@redhat.com> wrote:

> On Tue, Mar 27, 2018 at 11:50:52PM +0200, Martin Wilck wrote:
> > If the hardware handler isn't explicitly set, infer ALUA support
> > from the pp->tpgs attribute. Likewise, if ALUA is selected, but
> > not supported by the hardware, fall back to no hardware handler.  
> 
> Weren't you worried before about temporary ALUA failures? If you had a
> temporary failure while configuring a device that you explicitly set
> to be ALUA, then this would cause the device to be misconfigured? If
> the hardware handler isn't set, inferring ALUA is fine. But what is
> the case where we want to say that a device that is explicitly set to
> ALUA shouldn't actually be ALUA?  It seem like if there is some
> uncertaintly, we should just not set the hardware handler, and allow
> multipath to infer it via the pp->tpgs value.
> 
> I'm not strongly against this patch. I just don't see the value in
> overriding an explicit configuration, if we believe that temporary
> failures are possible.
> 
We _do_ have an definitive guide, namely the TGPS bit.
If that isn't set it's pretty much pointless to try alua, regardless
what the configuration says.
If it's set but ALUA configuration fails we do have an error.
If it's not set and ALUA configuration fails then it's 'just' a
misconfiguration.

Which is precisely what bit us with the IPR controller; all devices
appear as 'IPR', but only for some configuration the TPGS bit is
set.

And as the hardware handler was set to 'ALUA' the hardware handler
always tried to attach, but failed for those devices which did not
support ALUA.

_And_ as we don't have a distinction between 'configuration error' and
'hardware failure' these devices failed to setup, and booting would
stop.

So this patch is just how to handle devices which are configured to use
the ALUA hardware handler, but which do not have the TPGS bit set.
For these devices attaching ALUA _will_ fail, but that's _actually_
expected, as the devices never claimed to support alua.

Hence I'm perfectly fine with this patch.

Cheers,

Hannes

[PATCH 2/2] libmultipath/propsel: (re)use static const vars for origin

[Martin Wilck]

Define a set of "origin" strings and reuse them throughout the code.
That avoids duplication and makes it easier to verify that we use
consistent messages.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/propsel.c | 170 ++++++++++++++++++++++++++++---------------------
 1 file changed, 98 insertions(+), 72 deletions(-)

diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
index dc24450eb775..2d3b7adfdfe2 100644
--- a/libmultipath/propsel.c
+++ b/libmultipath/propsel.c
@@ -44,7 +44,19 @@ do {									\
 	}								\
 } while(0)
 
-static char default_origin[] = "(setting: multipath internal)";
+static const char default_origin[] = "(setting: multipath internal)";
+static const char hwe_origin[] =
+	"(setting: storage device configuration)";
+static const char multipaths_origin[] =
+	"(setting: multipath.conf multipaths section)";
+static const char conf_origin[] =
+	"(setting: multipath.conf defaults/devices section)";
+static const char overrides_origin[] =
+	"(setting: multipath.conf overrides section)";
+static const char cmdline_origin[] =
+	"(setting: multipath command line [-p] flag)";
+static const char autodetect_origin[] =
+	"(setting: storage device autodetected)";
 
 #define do_default(dest, value)						\
 do {									\
@@ -53,24 +65,24 @@ do {									\
 } while(0)
 
 #define mp_set_mpe(var)							\
-do_set(var, mp->mpe, mp->var, "(setting: multipath.conf multipaths section)")
+do_set(var, mp->mpe, mp->var, multipaths_origin)
 #define mp_set_hwe(var)							\
-do_set(var, mp->hwe, mp->var, "(setting: storage device configuration)")
+do_set(var, mp->hwe, mp->var, hwe_origin)
 #define mp_set_ovr(var)							\
-do_set(var, conf->overrides, mp->var, "(setting: multipath.conf overrides section)")
+do_set(var, conf->overrides, mp->var, overrides_origin)
 #define mp_set_conf(var)						\
-do_set(var, conf, mp->var, "(setting: multipath.conf defaults/devices section)")
+do_set(var, conf, mp->var, conf_origin)
 #define mp_set_default(var, value)					\
 do_default(mp->var, value)
 
 #define pp_set_mpe(var)							\
-do_set(var, mpe, pp->var, "(setting: multipath.conf multipaths section)")
+do_set(var, mpe, pp->var, multipaths_origin)
 #define pp_set_hwe(var)							\
-do_set(var, pp->hwe, pp->var, "(setting: storage device configuration)")
+do_set(var, pp->hwe, pp->var, hwe_origin)
 #define pp_set_conf(var)						\
-do_set(var, conf, pp->var, "(setting: multipath.conf defaults/devices section)")
+do_set(var, conf, pp->var, conf_origin)
 #define pp_set_ovr(var)							\
-do_set(var, conf->overrides, pp->var, "(setting: multipath.conf overrides section)")
+do_set(var, conf->overrides, pp->var, overrides_origin)
 #define pp_set_default(var, value)					\
 do_default(pp->var, value)
 
@@ -101,7 +113,7 @@ do {									\
 
 int select_mode(struct config *conf, struct multipath *mp)
 {
-	char *origin;
+	const char *origin;
 
 	set_attr_mpe(mode, ATTR_MODE);
 	set_attr_conf(mode, ATTR_MODE);
@@ -114,7 +126,7 @@ out:
 
 int select_uid(struct config *conf, struct multipath *mp)
 {
-	char *origin;
+	const char *origin;
 
 	set_attr_mpe(uid, ATTR_UID);
 	set_attr_conf(uid, ATTR_UID);
@@ -127,7 +139,7 @@ out:
 
 int select_gid(struct config *conf, struct multipath *mp)
 {
-	char *origin;
+	const char *origin;
 
 	set_attr_mpe(gid, ATTR_GID);
 	set_attr_conf(gid, ATTR_GID);
@@ -145,7 +157,8 @@ out:
  */
 int select_rr_weight(struct config *conf, struct multipath * mp)
 {
-	char *origin, buff[13];
+	const char *origin;
+	char buff[13];
 
 	mp_set_mpe(rr_weight);
 	mp_set_ovr(rr_weight);
@@ -160,7 +173,8 @@ out:
 
 int select_pgfailback(struct config *conf, struct multipath * mp)
 {
-	char *origin, buff[13];
+	const char *origin;
+	char buff[13];
 
 	mp_set_mpe(pgfailback);
 	mp_set_ovr(pgfailback);
@@ -175,11 +189,12 @@ out:
 
 int select_pgpolicy(struct config *conf, struct multipath * mp)
 {
-	char *origin, buff[POLICY_NAME_SIZE];
+	const char *origin;
+	char buff[POLICY_NAME_SIZE];
 
 	if (conf->pgpolicy_flag > 0) {
 		mp->pgpolicy = conf->pgpolicy_flag;
-		origin = "(setting: multipath command line [-p] flag)";
+		origin = cmdline_origin;
 		goto out;
 	}
 	mp_set_mpe(pgpolicy);
@@ -196,7 +211,7 @@ out:
 
 int select_selector(struct config *conf, struct multipath * mp)
 {
-	char *origin;
+	const char *origin;
 
 	mp_set_mpe(selector);
 	mp_set_ovr(selector);
@@ -213,7 +228,7 @@ out:
 static void
 select_alias_prefix (struct config *conf, struct multipath * mp)
 {
-	char *origin;
+	const char *origin;
 
 	mp_set_ovr(alias_prefix);
 	mp_set_hwe(alias_prefix);
@@ -228,17 +243,17 @@ static int
 want_user_friendly_names(struct config *conf, struct multipath * mp)
 {
 
-	char *origin;
+	const char *origin;
 	int user_friendly_names;
 
 	do_set(user_friendly_names, mp->mpe, user_friendly_names,
-	       "(setting: multipath.conf multipaths section)");
+	       multipaths_origin);
 	do_set(user_friendly_names, conf->overrides, user_friendly_names,
-	       "(setting: multipath.conf overrides section)");
+	       overrides_origin);
 	do_set(user_friendly_names, mp->hwe, user_friendly_names,
-	       "(setting: storage device configuration)");
+	       hwe_origin);
 	do_set(user_friendly_names, conf, user_friendly_names,
-	       "(setting: multipath.conf defaults/devices section)");
+	       conf_origin);
 	do_default(user_friendly_names, DEFAULT_USER_FRIENDLY_NAMES);
 out:
 	condlog(3, "%s: user_friendly_names = %s %s", mp->wwid,
@@ -249,11 +264,11 @@ out:
 
 int select_alias(struct config *conf, struct multipath * mp)
 {
-	char *origin = NULL;
+	const char *origin = NULL;
 
 	if (mp->mpe && mp->mpe->alias) {
 		mp->alias = STRDUP(mp->mpe->alias);
-		origin = "(setting: multipath.conf multipaths section)";
+		origin = multipaths_origin;
 		goto out;
 	}
 
@@ -342,7 +357,7 @@ void reconcile_features_with_options(const char *id, char **features, int* no_pa
 
 int select_features(struct config *conf, struct multipath *mp)
 {
-	char *origin;
+	const char *origin;
 
 	mp_set_mpe(features);
 	mp_set_ovr(features);
@@ -441,11 +456,12 @@ check_rdac(struct path * pp)
 
 int select_checker(struct config *conf, struct path *pp)
 {
-	char *origin, *checker_name;
+	const char *origin;
+	char *checker_name;
 	struct checker * c = &pp->checker;
 
 	if (pp->detect_checker == DETECT_CHECKER_ON) {
-		origin = "(setting: storage device autodetected)";
+		origin = autodetect_origin;
 		if (check_rdac(pp)) {
 			checker_name = RDAC;
 			goto out;
@@ -454,32 +470,32 @@ int select_checker(struct config *conf, struct path *pp)
 			goto out;
 		}
  	}
-	do_set(checker_name, conf->overrides, checker_name, "(setting: multipath.conf overrides section)");
-	do_set(checker_name, pp->hwe, checker_name, "(setting: storage device configuration)");
-	do_set(checker_name, conf, checker_name, "(setting: multipath.conf defaults/devices section)");
+	do_set(checker_name, conf->overrides, checker_name, overrides_origin);
+	do_set(checker_name, pp->hwe, checker_name, hwe_origin);
+	do_set(checker_name, conf, checker_name, conf_origin);
 	do_default(checker_name, DEFAULT_CHECKER);
 out:
 	checker_get(conf->multipath_dir, c, checker_name);
 	condlog(3, "%s: path_checker = %s %s", pp->dev, c->name, origin);
 	if (conf->checker_timeout) {
 		c->timeout = conf->checker_timeout;
-		condlog(3, "%s: checker timeout = %u s (setting: multipath.conf defaults/devices section)",
-				pp->dev, c->timeout);
+		condlog(3, "%s: checker timeout = %u s %s",
+			pp->dev, c->timeout, conf_origin);
 	}
 	else if (sysfs_get_timeout(pp, &c->timeout) > 0)
 		condlog(3, "%s: checker timeout = %u s (setting: kernel sysfs)",
-				pp->dev, c->timeout);
+			pp->dev, c->timeout);
 	else {
 		c->timeout = DEF_TIMEOUT;
-		condlog(3, "%s: checker timeout = %u s (setting: multipath internal)",
-				pp->dev, c->timeout);
+		condlog(3, "%s: checker timeout = %u s %s",
+			pp->dev, c->timeout, default_origin);
 	}
 	return 0;
 }
 
 int select_getuid(struct config *conf, struct path *pp)
 {
-	char *origin;
+	const char *origin;
 
 	pp->uid_attribute = parse_uid_attribute_by_attrs(conf->uid_attrs, pp->dev);
 	if (pp->uid_attribute) {
@@ -531,24 +547,24 @@ do {									\
 
 int select_prio(struct config *conf, struct path *pp)
 {
-	char *origin;
+	const char *origin;
 	struct mpentry * mpe;
 	struct prio * p = &pp->prio;
 
 	if (pp->detect_prio == DETECT_PRIO_ON) {
 		detect_prio(conf, pp);
 		if (prio_selected(p)) {
-			origin = "(setting: storage device autodetected)";
+			origin = autodetect_origin;
 			goto out;
 		}
 	}
 	mpe = find_mpe(conf->mptable, pp->wwid);
-	set_prio(conf->multipath_dir, mpe, "(setting: multipath.conf multipaths section)");
-	set_prio(conf->multipath_dir, conf->overrides, "(setting: multipath.conf overrides section)");
-	set_prio(conf->multipath_dir, pp->hwe, "(setting: storage device configuration)");
-	set_prio(conf->multipath_dir, conf, "(setting: multipath.conf defaults/devices section)");
+	set_prio(conf->multipath_dir, mpe, multipaths_origin);
+	set_prio(conf->multipath_dir, conf->overrides, overrides_origin);
+	set_prio(conf->multipath_dir, pp->hwe, hwe_origin);
+	set_prio(conf->multipath_dir, conf, conf_origin);
 	prio_get(conf->multipath_dir, p, DEFAULT_PRIO, DEFAULT_PRIO_ARGS);
-	origin = "(setting: multipath internal)";
+	origin = default_origin;
 out:
 	/*
 	 * fetch tpgs mode for alua, if its not already obtained
@@ -568,7 +584,7 @@ out:
 
 int select_no_path_retry(struct config *conf, struct multipath *mp)
 {
-	char *origin = NULL;
+	const char *origin = NULL;
 	char buff[12];
 
 	if (mp->disable_queueing) {
@@ -586,20 +602,20 @@ out:
 		condlog(3, "%s: no_path_retry = %s %s", mp->alias, buff,
 			origin);
 	else
-		condlog(3, "%s: no_path_retry = undef (setting: multipath internal)",
-			mp->alias);
+		condlog(3, "%s: no_path_retry = undef %s",
+			mp->alias, default_origin);
 	return 0;
 }
 
 int
 select_minio_rq (struct config *conf, struct multipath * mp)
 {
-	char *origin;
+	const char *origin;
 
-	do_set(minio_rq, mp->mpe, mp->minio, "(setting: multipath.conf multipaths section)");
-	do_set(minio_rq, conf->overrides, mp->minio, "(setting: multipath.conf overrides section)");
-	do_set(minio_rq, mp->hwe, mp->minio, "(setting: storage device configuration)");
-	do_set(minio_rq, conf, mp->minio, "(setting: multipath.conf defaults/devices section)");
+	do_set(minio_rq, mp->mpe, mp->minio, multipaths_origin);
+	do_set(minio_rq, conf->overrides, mp->minio, overrides_origin);
+	do_set(minio_rq, mp->hwe, mp->minio, hwe_origin);
+	do_set(minio_rq, conf, mp->minio, conf_origin);
 	do_default(mp->minio, DEFAULT_MINIO_RQ);
 out:
 	condlog(3, "%s: minio = %i %s", mp->alias, mp->minio, origin);
@@ -609,7 +625,7 @@ out:
 int
 select_minio_bio (struct config *conf, struct multipath * mp)
 {
-	char *origin;
+	const char *origin;
 
 	mp_set_mpe(minio);
 	mp_set_ovr(minio);
@@ -633,7 +649,8 @@ int select_minio(struct config *conf, struct multipath *mp)
 
 int select_fast_io_fail(struct config *conf, struct multipath *mp)
 {
-	char *origin, buff[12];
+	const char *origin;
+	char buff[12];
 
 	mp_set_ovr(fast_io_fail);
 	mp_set_hwe(fast_io_fail);
@@ -647,7 +664,8 @@ out:
 
 int select_dev_loss(struct config *conf, struct multipath *mp)
 {
-	char *origin, buff[12];
+	const char *origin;
+	char buff[12];
 
 	mp_set_ovr(dev_loss);
 	mp_set_hwe(dev_loss);
@@ -662,7 +680,7 @@ out:
 
 int select_flush_on_last_del(struct config *conf, struct multipath *mp)
 {
-	char *origin;
+	const char *origin;
 
 	mp_set_mpe(flush_on_last_del);
 	mp_set_ovr(flush_on_last_del);
@@ -677,12 +695,13 @@ out:
 
 int select_reservation_key(struct config *conf, struct multipath *mp)
 {
-	char *origin, buff[PRKEY_SIZE];
+	const char *origin;
+	char buff[PRKEY_SIZE];
 	char *from_file = "";
 	uint64_t prkey = 0;
 
-	do_prkey_set(mp->mpe, "(setting: multipath.conf multipaths section)");
-	do_prkey_set(conf, "(setting: multipath.conf defaults/devices section)");
+	do_prkey_set(mp->mpe, multipaths_origin);
+	do_prkey_set(conf, conf_origin);
 	put_be64(mp->reservation_key, 0);
 	mp->prkey_source = PRKEY_SOURCE_NONE;
 	return 0;
@@ -703,7 +722,7 @@ out:
 
 int select_retain_hwhandler(struct config *conf, struct multipath *mp)
 {
-	char *origin;
+	const char *origin;
 	unsigned int minv_dm_retain[3] = {1, 5, 0};
 
 	if (!VERSION_GE(conf->version, minv_dm_retain)) {
@@ -729,7 +748,7 @@ out:
 
 int select_detect_prio(struct config *conf, struct path *pp)
 {
-	char *origin;
+	const char *origin;
 
 	pp_set_ovr(detect_prio);
 	pp_set_hwe(detect_prio);
@@ -743,7 +762,7 @@ out:
 
 int select_detect_checker(struct config *conf, struct path *pp)
 {
-	char *origin;
+	const char *origin;
 
 	pp_set_ovr(detect_checker);
 	pp_set_hwe(detect_checker);
@@ -758,7 +777,7 @@ out:
 
 int select_deferred_remove(struct config *conf, struct multipath *mp)
 {
-	char *origin;
+	const char *origin;
 
 #ifndef LIBDM_API_DEFERRED
 	mp->deferred_remove = DEFERRED_REMOVE_OFF;
@@ -783,7 +802,8 @@ out:
 
 int select_delay_watch_checks(struct config *conf, struct multipath *mp)
 {
-	char *origin, buff[12];
+	const char *origin;
+	char buff[12];
 
 	mp_set_mpe(delay_watch_checks);
 	mp_set_ovr(delay_watch_checks);
@@ -798,7 +818,8 @@ out:
 
 int select_delay_wait_checks(struct config *conf, struct multipath *mp)
 {
-	char *origin, buff[12];
+	const char *origin;
+	char buff[12];
 
 	mp_set_mpe(delay_wait_checks);
 	mp_set_ovr(delay_wait_checks);
@@ -814,7 +835,8 @@ out:
 
 int select_marginal_path_err_sample_time(struct config *conf, struct multipath *mp)
 {
-	char *origin, buff[12];
+	const char *origin;
+	char buff[12];
 
 	mp_set_mpe(marginal_path_err_sample_time);
 	mp_set_ovr(marginal_path_err_sample_time);
@@ -830,7 +852,8 @@ out:
 
 int select_marginal_path_err_rate_threshold(struct config *conf, struct multipath *mp)
 {
-	char *origin, buff[12];
+	const char *origin;
+	char buff[12];
 
 	mp_set_mpe(marginal_path_err_rate_threshold);
 	mp_set_ovr(marginal_path_err_rate_threshold);
@@ -846,7 +869,8 @@ out:
 
 int select_marginal_path_err_recheck_gap_time(struct config *conf, struct multipath *mp)
 {
-	char *origin, buff[12];
+	const char *origin;
+	char buff[12];
 
 	mp_set_mpe(marginal_path_err_recheck_gap_time);
 	mp_set_ovr(marginal_path_err_recheck_gap_time);
@@ -862,7 +886,8 @@ out:
 
 int select_marginal_path_double_failed_time(struct config *conf, struct multipath *mp)
 {
-	char *origin, buff[12];
+	const char *origin;
+	char buff[12];
 
 	mp_set_mpe(marginal_path_double_failed_time);
 	mp_set_ovr(marginal_path_double_failed_time);
@@ -878,7 +903,7 @@ out:
 
 int select_skip_kpartx (struct config *conf, struct multipath * mp)
 {
-	char *origin;
+	const char *origin;
 
 	mp_set_mpe(skip_kpartx);
 	mp_set_ovr(skip_kpartx);
@@ -894,7 +919,7 @@ out:
 
 int select_max_sectors_kb(struct config *conf, struct multipath * mp)
 {
-	char *origin;
+	const char *origin;
 
 	mp_set_mpe(max_sectors_kb);
 	mp_set_ovr(max_sectors_kb);
@@ -915,7 +940,8 @@ out:
 
 int select_ghost_delay (struct config *conf, struct multipath * mp)
 {
-	char *origin, buff[12];
+	const char *origin;
+	char buff[12];
 
 	mp_set_mpe(ghost_delay);
 	mp_set_ovr(ghost_delay);
-- 
2.16.1

Re: [PATCH 2/2] libmultipath/propsel: (re)use static const vars for origin

[Benjamin Marzinski]

On Tue, Mar 27, 2018 at 11:50:53PM +0200, Martin Wilck wrote:
> Define a set of "origin" strings and reuse them throughout the code.
> That avoids duplication and makes it easier to verify that we use
> consistent messages.
> 
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/propsel.c | 170 ++++++++++++++++++++++++++++---------------------
>  1 file changed, 98 insertions(+), 72 deletions(-)
> 
> diff --git a/libmultipath/propsel.c b/libmultipath/propsel.c
> index dc24450eb775..2d3b7adfdfe2 100644
> --- a/libmultipath/propsel.c
> +++ b/libmultipath/propsel.c
> @@ -44,7 +44,19 @@ do {									\
>  	}								\
>  } while(0)
>  
> -static char default_origin[] = "(setting: multipath internal)";
> +static const char default_origin[] = "(setting: multipath internal)";
> +static const char hwe_origin[] =
> +	"(setting: storage device configuration)";
> +static const char multipaths_origin[] =
> +	"(setting: multipath.conf multipaths section)";
> +static const char conf_origin[] =
> +	"(setting: multipath.conf defaults/devices section)";
> +static const char overrides_origin[] =
> +	"(setting: multipath.conf overrides section)";
> +static const char cmdline_origin[] =
> +	"(setting: multipath command line [-p] flag)";
> +static const char autodetect_origin[] =
> +	"(setting: storage device autodetected)";
>  
>  #define do_default(dest, value)						\
>  do {									\
> @@ -53,24 +65,24 @@ do {									\
>  } while(0)
>  
>  #define mp_set_mpe(var)							\
> -do_set(var, mp->mpe, mp->var, "(setting: multipath.conf multipaths section)")
> +do_set(var, mp->mpe, mp->var, multipaths_origin)
>  #define mp_set_hwe(var)							\
> -do_set(var, mp->hwe, mp->var, "(setting: storage device configuration)")
> +do_set(var, mp->hwe, mp->var, hwe_origin)
>  #define mp_set_ovr(var)							\
> -do_set(var, conf->overrides, mp->var, "(setting: multipath.conf overrides section)")
> +do_set(var, conf->overrides, mp->var, overrides_origin)
>  #define mp_set_conf(var)						\
> -do_set(var, conf, mp->var, "(setting: multipath.conf defaults/devices section)")
> +do_set(var, conf, mp->var, conf_origin)
>  #define mp_set_default(var, value)					\
>  do_default(mp->var, value)
>  
>  #define pp_set_mpe(var)							\
> -do_set(var, mpe, pp->var, "(setting: multipath.conf multipaths section)")
> +do_set(var, mpe, pp->var, multipaths_origin)
>  #define pp_set_hwe(var)							\
> -do_set(var, pp->hwe, pp->var, "(setting: storage device configuration)")
> +do_set(var, pp->hwe, pp->var, hwe_origin)
>  #define pp_set_conf(var)						\
> -do_set(var, conf, pp->var, "(setting: multipath.conf defaults/devices section)")
> +do_set(var, conf, pp->var, conf_origin)
>  #define pp_set_ovr(var)							\
> -do_set(var, conf->overrides, pp->var, "(setting: multipath.conf overrides section)")
> +do_set(var, conf->overrides, pp->var, overrides_origin)
>  #define pp_set_default(var, value)					\
>  do_default(pp->var, value)
>  
> @@ -101,7 +113,7 @@ do {									\
>  
>  int select_mode(struct config *conf, struct multipath *mp)
>  {
> -	char *origin;
> +	const char *origin;
>  
>  	set_attr_mpe(mode, ATTR_MODE);
>  	set_attr_conf(mode, ATTR_MODE);
> @@ -114,7 +126,7 @@ out:
>  
>  int select_uid(struct config *conf, struct multipath *mp)
>  {
> -	char *origin;
> +	const char *origin;
>  
>  	set_attr_mpe(uid, ATTR_UID);
>  	set_attr_conf(uid, ATTR_UID);
> @@ -127,7 +139,7 @@ out:
>  
>  int select_gid(struct config *conf, struct multipath *mp)
>  {
> -	char *origin;
> +	const char *origin;
>  
>  	set_attr_mpe(gid, ATTR_GID);
>  	set_attr_conf(gid, ATTR_GID);
> @@ -145,7 +157,8 @@ out:
>   */
>  int select_rr_weight(struct config *conf, struct multipath * mp)
>  {
> -	char *origin, buff[13];
> +	const char *origin;
> +	char buff[13];
>  
>  	mp_set_mpe(rr_weight);
>  	mp_set_ovr(rr_weight);
> @@ -160,7 +173,8 @@ out:
>  
>  int select_pgfailback(struct config *conf, struct multipath * mp)
>  {
> -	char *origin, buff[13];
> +	const char *origin;
> +	char buff[13];
>  
>  	mp_set_mpe(pgfailback);
>  	mp_set_ovr(pgfailback);
> @@ -175,11 +189,12 @@ out:
>  
>  int select_pgpolicy(struct config *conf, struct multipath * mp)
>  {
> -	char *origin, buff[POLICY_NAME_SIZE];
> +	const char *origin;
> +	char buff[POLICY_NAME_SIZE];
>  
>  	if (conf->pgpolicy_flag > 0) {
>  		mp->pgpolicy = conf->pgpolicy_flag;
> -		origin = "(setting: multipath command line [-p] flag)";
> +		origin = cmdline_origin;
>  		goto out;
>  	}
>  	mp_set_mpe(pgpolicy);
> @@ -196,7 +211,7 @@ out:
>  
>  int select_selector(struct config *conf, struct multipath * mp)
>  {
> -	char *origin;
> +	const char *origin;
>  
>  	mp_set_mpe(selector);
>  	mp_set_ovr(selector);
> @@ -213,7 +228,7 @@ out:
>  static void
>  select_alias_prefix (struct config *conf, struct multipath * mp)
>  {
> -	char *origin;
> +	const char *origin;
>  
>  	mp_set_ovr(alias_prefix);
>  	mp_set_hwe(alias_prefix);
> @@ -228,17 +243,17 @@ static int
>  want_user_friendly_names(struct config *conf, struct multipath * mp)
>  {
>  
> -	char *origin;
> +	const char *origin;
>  	int user_friendly_names;
>  
>  	do_set(user_friendly_names, mp->mpe, user_friendly_names,
> -	       "(setting: multipath.conf multipaths section)");
> +	       multipaths_origin);
>  	do_set(user_friendly_names, conf->overrides, user_friendly_names,
> -	       "(setting: multipath.conf overrides section)");
> +	       overrides_origin);
>  	do_set(user_friendly_names, mp->hwe, user_friendly_names,
> -	       "(setting: storage device configuration)");
> +	       hwe_origin);
>  	do_set(user_friendly_names, conf, user_friendly_names,
> -	       "(setting: multipath.conf defaults/devices section)");
> +	       conf_origin);
>  	do_default(user_friendly_names, DEFAULT_USER_FRIENDLY_NAMES);
>  out:
>  	condlog(3, "%s: user_friendly_names = %s %s", mp->wwid,
> @@ -249,11 +264,11 @@ out:
>  
>  int select_alias(struct config *conf, struct multipath * mp)
>  {
> -	char *origin = NULL;
> +	const char *origin = NULL;
>  
>  	if (mp->mpe && mp->mpe->alias) {
>  		mp->alias = STRDUP(mp->mpe->alias);
> -		origin = "(setting: multipath.conf multipaths section)";
> +		origin = multipaths_origin;
>  		goto out;
>  	}
>  
> @@ -342,7 +357,7 @@ void reconcile_features_with_options(const char *id, char **features, int* no_pa
>  
>  int select_features(struct config *conf, struct multipath *mp)
>  {
> -	char *origin;
> +	const char *origin;
>  
>  	mp_set_mpe(features);
>  	mp_set_ovr(features);
> @@ -441,11 +456,12 @@ check_rdac(struct path * pp)
>  
>  int select_checker(struct config *conf, struct path *pp)
>  {
> -	char *origin, *checker_name;
> +	const char *origin;
> +	char *checker_name;
>  	struct checker * c = &pp->checker;
>  
>  	if (pp->detect_checker == DETECT_CHECKER_ON) {
> -		origin = "(setting: storage device autodetected)";
> +		origin = autodetect_origin;
>  		if (check_rdac(pp)) {
>  			checker_name = RDAC;
>  			goto out;
> @@ -454,32 +470,32 @@ int select_checker(struct config *conf, struct path *pp)
>  			goto out;
>  		}
>   	}
> -	do_set(checker_name, conf->overrides, checker_name, "(setting: multipath.conf overrides section)");
> -	do_set(checker_name, pp->hwe, checker_name, "(setting: storage device configuration)");
> -	do_set(checker_name, conf, checker_name, "(setting: multipath.conf defaults/devices section)");
> +	do_set(checker_name, conf->overrides, checker_name, overrides_origin);
> +	do_set(checker_name, pp->hwe, checker_name, hwe_origin);
> +	do_set(checker_name, conf, checker_name, conf_origin);
>  	do_default(checker_name, DEFAULT_CHECKER);
>  out:
>  	checker_get(conf->multipath_dir, c, checker_name);
>  	condlog(3, "%s: path_checker = %s %s", pp->dev, c->name, origin);
>  	if (conf->checker_timeout) {
>  		c->timeout = conf->checker_timeout;
> -		condlog(3, "%s: checker timeout = %u s (setting: multipath.conf defaults/devices section)",
> -				pp->dev, c->timeout);
> +		condlog(3, "%s: checker timeout = %u s %s",
> +			pp->dev, c->timeout, conf_origin);
>  	}
>  	else if (sysfs_get_timeout(pp, &c->timeout) > 0)
>  		condlog(3, "%s: checker timeout = %u s (setting: kernel sysfs)",
> -				pp->dev, c->timeout);
> +			pp->dev, c->timeout);
>  	else {
>  		c->timeout = DEF_TIMEOUT;
> -		condlog(3, "%s: checker timeout = %u s (setting: multipath internal)",
> -				pp->dev, c->timeout);
> +		condlog(3, "%s: checker timeout = %u s %s",
> +			pp->dev, c->timeout, default_origin);
>  	}
>  	return 0;
>  }
>  
>  int select_getuid(struct config *conf, struct path *pp)
>  {
> -	char *origin;
> +	const char *origin;
>  
>  	pp->uid_attribute = parse_uid_attribute_by_attrs(conf->uid_attrs, pp->dev);
>  	if (pp->uid_attribute) {
> @@ -531,24 +547,24 @@ do {									\
>  
>  int select_prio(struct config *conf, struct path *pp)
>  {
> -	char *origin;
> +	const char *origin;
>  	struct mpentry * mpe;
>  	struct prio * p = &pp->prio;
>  
>  	if (pp->detect_prio == DETECT_PRIO_ON) {
>  		detect_prio(conf, pp);
>  		if (prio_selected(p)) {
> -			origin = "(setting: storage device autodetected)";
> +			origin = autodetect_origin;
>  			goto out;
>  		}
>  	}
>  	mpe = find_mpe(conf->mptable, pp->wwid);
> -	set_prio(conf->multipath_dir, mpe, "(setting: multipath.conf multipaths section)");
> -	set_prio(conf->multipath_dir, conf->overrides, "(setting: multipath.conf overrides section)");
> -	set_prio(conf->multipath_dir, pp->hwe, "(setting: storage device configuration)");
> -	set_prio(conf->multipath_dir, conf, "(setting: multipath.conf defaults/devices section)");
> +	set_prio(conf->multipath_dir, mpe, multipaths_origin);
> +	set_prio(conf->multipath_dir, conf->overrides, overrides_origin);
> +	set_prio(conf->multipath_dir, pp->hwe, hwe_origin);
> +	set_prio(conf->multipath_dir, conf, conf_origin);
>  	prio_get(conf->multipath_dir, p, DEFAULT_PRIO, DEFAULT_PRIO_ARGS);
> -	origin = "(setting: multipath internal)";
> +	origin = default_origin;
>  out:
>  	/*
>  	 * fetch tpgs mode for alua, if its not already obtained
> @@ -568,7 +584,7 @@ out:
>  
>  int select_no_path_retry(struct config *conf, struct multipath *mp)
>  {
> -	char *origin = NULL;
> +	const char *origin = NULL;
>  	char buff[12];
>  
>  	if (mp->disable_queueing) {
> @@ -586,20 +602,20 @@ out:
>  		condlog(3, "%s: no_path_retry = %s %s", mp->alias, buff,
>  			origin);
>  	else
> -		condlog(3, "%s: no_path_retry = undef (setting: multipath internal)",
> -			mp->alias);
> +		condlog(3, "%s: no_path_retry = undef %s",
> +			mp->alias, default_origin);
>  	return 0;
>  }
>  
>  int
>  select_minio_rq (struct config *conf, struct multipath * mp)
>  {
> -	char *origin;
> +	const char *origin;
>  
> -	do_set(minio_rq, mp->mpe, mp->minio, "(setting: multipath.conf multipaths section)");
> -	do_set(minio_rq, conf->overrides, mp->minio, "(setting: multipath.conf overrides section)");
> -	do_set(minio_rq, mp->hwe, mp->minio, "(setting: storage device configuration)");
> -	do_set(minio_rq, conf, mp->minio, "(setting: multipath.conf defaults/devices section)");
> +	do_set(minio_rq, mp->mpe, mp->minio, multipaths_origin);
> +	do_set(minio_rq, conf->overrides, mp->minio, overrides_origin);
> +	do_set(minio_rq, mp->hwe, mp->minio, hwe_origin);
> +	do_set(minio_rq, conf, mp->minio, conf_origin);
>  	do_default(mp->minio, DEFAULT_MINIO_RQ);
>  out:
>  	condlog(3, "%s: minio = %i %s", mp->alias, mp->minio, origin);
> @@ -609,7 +625,7 @@ out:
>  int
>  select_minio_bio (struct config *conf, struct multipath * mp)
>  {
> -	char *origin;
> +	const char *origin;
>  
>  	mp_set_mpe(minio);
>  	mp_set_ovr(minio);
> @@ -633,7 +649,8 @@ int select_minio(struct config *conf, struct multipath *mp)
>  
>  int select_fast_io_fail(struct config *conf, struct multipath *mp)
>  {
> -	char *origin, buff[12];
> +	const char *origin;
> +	char buff[12];
>  
>  	mp_set_ovr(fast_io_fail);
>  	mp_set_hwe(fast_io_fail);
> @@ -647,7 +664,8 @@ out:
>  
>  int select_dev_loss(struct config *conf, struct multipath *mp)
>  {
> -	char *origin, buff[12];
> +	const char *origin;
> +	char buff[12];
>  
>  	mp_set_ovr(dev_loss);
>  	mp_set_hwe(dev_loss);
> @@ -662,7 +680,7 @@ out:
>  
>  int select_flush_on_last_del(struct config *conf, struct multipath *mp)
>  {
> -	char *origin;
> +	const char *origin;
>  
>  	mp_set_mpe(flush_on_last_del);
>  	mp_set_ovr(flush_on_last_del);
> @@ -677,12 +695,13 @@ out:
>  
>  int select_reservation_key(struct config *conf, struct multipath *mp)
>  {
> -	char *origin, buff[PRKEY_SIZE];
> +	const char *origin;
> +	char buff[PRKEY_SIZE];
>  	char *from_file = "";
>  	uint64_t prkey = 0;
>  
> -	do_prkey_set(mp->mpe, "(setting: multipath.conf multipaths section)");
> -	do_prkey_set(conf, "(setting: multipath.conf defaults/devices section)");
> +	do_prkey_set(mp->mpe, multipaths_origin);
> +	do_prkey_set(conf, conf_origin);
>  	put_be64(mp->reservation_key, 0);
>  	mp->prkey_source = PRKEY_SOURCE_NONE;
>  	return 0;
> @@ -703,7 +722,7 @@ out:
>  
>  int select_retain_hwhandler(struct config *conf, struct multipath *mp)
>  {
> -	char *origin;
> +	const char *origin;
>  	unsigned int minv_dm_retain[3] = {1, 5, 0};
>  
>  	if (!VERSION_GE(conf->version, minv_dm_retain)) {
> @@ -729,7 +748,7 @@ out:
>  
>  int select_detect_prio(struct config *conf, struct path *pp)
>  {
> -	char *origin;
> +	const char *origin;
>  
>  	pp_set_ovr(detect_prio);
>  	pp_set_hwe(detect_prio);
> @@ -743,7 +762,7 @@ out:
>  
>  int select_detect_checker(struct config *conf, struct path *pp)
>  {
> -	char *origin;
> +	const char *origin;
>  
>  	pp_set_ovr(detect_checker);
>  	pp_set_hwe(detect_checker);
> @@ -758,7 +777,7 @@ out:
>  
>  int select_deferred_remove(struct config *conf, struct multipath *mp)
>  {
> -	char *origin;
> +	const char *origin;
>  
>  #ifndef LIBDM_API_DEFERRED
>  	mp->deferred_remove = DEFERRED_REMOVE_OFF;
> @@ -783,7 +802,8 @@ out:
>  
>  int select_delay_watch_checks(struct config *conf, struct multipath *mp)
>  {
> -	char *origin, buff[12];
> +	const char *origin;
> +	char buff[12];
>  
>  	mp_set_mpe(delay_watch_checks);
>  	mp_set_ovr(delay_watch_checks);
> @@ -798,7 +818,8 @@ out:
>  
>  int select_delay_wait_checks(struct config *conf, struct multipath *mp)
>  {
> -	char *origin, buff[12];
> +	const char *origin;
> +	char buff[12];
>  
>  	mp_set_mpe(delay_wait_checks);
>  	mp_set_ovr(delay_wait_checks);
> @@ -814,7 +835,8 @@ out:
>  
>  int select_marginal_path_err_sample_time(struct config *conf, struct multipath *mp)
>  {
> -	char *origin, buff[12];
> +	const char *origin;
> +	char buff[12];
>  
>  	mp_set_mpe(marginal_path_err_sample_time);
>  	mp_set_ovr(marginal_path_err_sample_time);
> @@ -830,7 +852,8 @@ out:
>  
>  int select_marginal_path_err_rate_threshold(struct config *conf, struct multipath *mp)
>  {
> -	char *origin, buff[12];
> +	const char *origin;
> +	char buff[12];
>  
>  	mp_set_mpe(marginal_path_err_rate_threshold);
>  	mp_set_ovr(marginal_path_err_rate_threshold);
> @@ -846,7 +869,8 @@ out:
>  
>  int select_marginal_path_err_recheck_gap_time(struct config *conf, struct multipath *mp)
>  {
> -	char *origin, buff[12];
> +	const char *origin;
> +	char buff[12];
>  
>  	mp_set_mpe(marginal_path_err_recheck_gap_time);
>  	mp_set_ovr(marginal_path_err_recheck_gap_time);
> @@ -862,7 +886,8 @@ out:
>  
>  int select_marginal_path_double_failed_time(struct config *conf, struct multipath *mp)
>  {
> -	char *origin, buff[12];
> +	const char *origin;
> +	char buff[12];
>  
>  	mp_set_mpe(marginal_path_double_failed_time);
>  	mp_set_ovr(marginal_path_double_failed_time);
> @@ -878,7 +903,7 @@ out:
>  
>  int select_skip_kpartx (struct config *conf, struct multipath * mp)
>  {
> -	char *origin;
> +	const char *origin;
>  
>  	mp_set_mpe(skip_kpartx);
>  	mp_set_ovr(skip_kpartx);
> @@ -894,7 +919,7 @@ out:
>  
>  int select_max_sectors_kb(struct config *conf, struct multipath * mp)
>  {
> -	char *origin;
> +	const char *origin;
>  
>  	mp_set_mpe(max_sectors_kb);
>  	mp_set_ovr(max_sectors_kb);
> @@ -915,7 +940,8 @@ out:
>  
>  int select_ghost_delay (struct config *conf, struct multipath * mp)
>  {
> -	char *origin, buff[12];
> +	const char *origin;
> +	char buff[12];
>  
>  	mp_set_mpe(ghost_delay);
>  	mp_set_ovr(ghost_delay);
> -- 
> 2.16.1