Re: [dm-devel] [PATCH 1/2] multipath.conf: add "enable_foreign" parameter

2019-08-20 Thread Benjamin Marzinski
On Tue, Aug 20, 2019 at 04:24:58PM +, Martin Wilck wrote:
> On Mon, 2019-08-19 at 20:32 +, Martin Wilck wrote:
> > On Fri, 2019-08-16 at 15:12 -0500, Benjamin Marzinski wrote:
> > > On Thu, Aug 15, 2019 at 02:46:54PM +, Martin Wilck wrote:
> > > > From: Martin Wilck 
> > > > 
> > > > This new configuration parameter can be used to selectively
> > > > enable foreign libraries. The value is a regular expression,
> > > > against which foreign library names such as "nvme" are matched.
> > > > By setting this to a value that matches no foreign library
> > > > (e.g. "^$" or "NONE"), foreign code is completely disabled.
> > > > By default, all available foreign libraries are loaded.
> > > > 
> > > 
> > > This will stop the foreign libraries from even claiming devices.
> > > Won't
> > > this mean that pathinfo() will now treat these devices as
> > > multipathable
> > > paths, since is_claimed_by_foreign() will return false? 
> > 
> > IMO this won't happen, because we ignore the native multipath path
> > devices anyway as they're "hidden", and we ignore native NVMe
> > multipath
> > maps because they have "nvme-subsystem" subsystem rather than "nvme"
> > (commit b18ed66). But I'll double-check again.
> 
> Confirmed. With nvme_core.multipath=Y and the nvme library disabled,no
> NVMe-related output is printed.
> 
> Ben, with that in mind, would you ACK this patch?

Sure. Sorry about the confusion

Reviewed-by: Benjamin Marzinksi 
 
> Martin
> 

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


Re: [dm-devel] [PATCH 1/2] multipath.conf: add "enable_foreign" parameter

2019-08-20 Thread Martin Wilck
On Mon, 2019-08-19 at 20:32 +, Martin Wilck wrote:
> On Fri, 2019-08-16 at 15:12 -0500, Benjamin Marzinski wrote:
> > On Thu, Aug 15, 2019 at 02:46:54PM +, Martin Wilck wrote:
> > > From: Martin Wilck 
> > > 
> > > This new configuration parameter can be used to selectively
> > > enable foreign libraries. The value is a regular expression,
> > > against which foreign library names such as "nvme" are matched.
> > > By setting this to a value that matches no foreign library
> > > (e.g. "^$" or "NONE"), foreign code is completely disabled.
> > > By default, all available foreign libraries are loaded.
> > > 
> > 
> > This will stop the foreign libraries from even claiming devices.
> > Won't
> > this mean that pathinfo() will now treat these devices as
> > multipathable
> > paths, since is_claimed_by_foreign() will return false? 
> 
> IMO this won't happen, because we ignore the native multipath path
> devices anyway as they're "hidden", and we ignore native NVMe
> multipath
> maps because they have "nvme-subsystem" subsystem rather than "nvme"
> (commit b18ed66). But I'll double-check again.

Confirmed. With nvme_core.multipath=Y and the nvme library disabled,no
NVMe-related output is printed.

Ben, with that in mind, would you ACK this patch?

Martin


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


Re: [dm-devel] [PATCH 1/2] multipath.conf: add "enable_foreign" parameter

2019-08-19 Thread Martin Wilck
On Fri, 2019-08-16 at 15:12 -0500, Benjamin Marzinski wrote:
> On Thu, Aug 15, 2019 at 02:46:54PM +, Martin Wilck wrote:
> > From: Martin Wilck 
> > 
> > This new configuration parameter can be used to selectively
> > enable foreign libraries. The value is a regular expression,
> > against which foreign library names such as "nvme" are matched.
> > By setting this to a value that matches no foreign library
> > (e.g. "^$" or "NONE"), foreign code is completely disabled.
> > By default, all available foreign libraries are loaded.
> > 
> 
> This will stop the foreign libraries from even claiming devices.
> Won't
> this mean that pathinfo() will now treat these devices as
> multipathable
> paths, since is_claimed_by_foreign() will return false? 

IMO this won't happen, because we ignore the native multipath path
devices anyway as they're "hidden", and we ignore native NVMe multipath
maps because they have "nvme-subsystem" subsystem rather than "nvme"
(commit b18ed66). But I'll double-check again.

Martin


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


Re: [dm-devel] [PATCH 1/2] multipath.conf: add "enable_foreign" parameter

2019-08-16 Thread Benjamin Marzinski
On Thu, Aug 15, 2019 at 02:46:54PM +, Martin Wilck wrote:
> From: Martin Wilck 
> 
> This new configuration parameter can be used to selectively
> enable foreign libraries. The value is a regular expression,
> against which foreign library names such as "nvme" are matched.
> By setting this to a value that matches no foreign library
> (e.g. "^$" or "NONE"), foreign code is completely disabled.
> By default, all available foreign libraries are loaded.
> 

This will stop the foreign libraries from even claiming devices. Won't
this mean that pathinfo() will now treat these devices as multipathable
paths, since is_claimed_by_foreign() will return false? While I do see
the value in disabling foreign libraries completely, that's not the
intent of my patch.  The goal of my patch is simply to not print the
devices have been claimed by the foreign libraries.

-Ben

> Signed-off-by: Martin Wilck 
> ---
>  libmultipath/config.h   |  1 +
>  libmultipath/defaults.h |  2 ++
>  libmultipath/dict.c |  6 +
>  libmultipath/foreign.c  | 53 +
>  libmultipath/foreign.h  |  3 ++-
>  multipath/main.c|  2 +-
>  multipathd/main.c   |  2 +-
>  7 files changed, 62 insertions(+), 7 deletions(-)
> 
> diff --git a/libmultipath/config.h b/libmultipath/config.h
> index ff2b4e86..36e99196 100644
> --- a/libmultipath/config.h
> +++ b/libmultipath/config.h
> @@ -224,6 +224,7 @@ struct config {
>   vector elist_device;
>   vector elist_property;
>   vector elist_protocol;
> + char *enable_foreign;
>  };
>  
>  extern struct udev * udev;
> diff --git a/libmultipath/defaults.h b/libmultipath/defaults.h
> index decc9335..4dfe007c 100644
> --- a/libmultipath/defaults.h
> +++ b/libmultipath/defaults.h
> @@ -48,6 +48,8 @@
>  #define DEFAULT_FIND_MULTIPATHS_TIMEOUT -10
>  #define DEFAULT_UNKNOWN_FIND_MULTIPATHS_TIMEOUT 1
>  #define DEFAULT_ALL_TG_PT ALL_TG_PT_OFF
> +/* Enable all foreign libraries by default */
> +#define DEFAULT_ENABLE_FOREIGN ""
>  
>  #define CHECKINT_UNDEF   (~0U)
>  #define DEFAULT_CHECKINT 5
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index c6eba0f6..9f282c3f 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -610,6 +610,10 @@ declare_def_handler(find_multipaths_timeout, set_int)
>  declare_def_snprint_defint(find_multipaths_timeout, print_int,
>  DEFAULT_FIND_MULTIPATHS_TIMEOUT)
>  
> +declare_def_handler(enable_foreign, set_str)
> +declare_def_snprint_defstr(enable_foreign, print_str,
> +DEFAULT_ENABLE_FOREIGN)
> +
>  static int
>  def_config_dir_handler(struct config *conf, vector strvec)
>  {
> @@ -1710,6 +1714,8 @@ init_keywords(vector keywords)
>   install_keyword("find_multipaths_timeout",
>   &def_find_multipaths_timeout_handler,
>   &snprint_def_find_multipaths_timeout);
> + install_keyword("enable_foreign", &def_enable_foreign_handler,
> + &snprint_def_enable_foreign);
>   __deprecated install_keyword("default_selector", &def_selector_handler, 
> NULL);
>   __deprecated install_keyword("default_path_grouping_policy", 
> &def_pgpolicy_handler, NULL);
>   __deprecated install_keyword("default_uid_attribute", 
> &def_uid_attribute_handler, NULL);
> diff --git a/libmultipath/foreign.c b/libmultipath/foreign.c
> index 48e8d247..4b34e141 100644
> --- a/libmultipath/foreign.c
> +++ b/libmultipath/foreign.c
> @@ -16,6 +16,7 @@
>  */
>  
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -25,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include "vector.h"
>  #include "debug.h"
>  #include "util.h"
> @@ -111,17 +113,45 @@ static int select_foreign_libs(const struct dirent *di)
>   return fnmatch(foreign_pattern, di->d_name, FNM_FILE_NAME) == 0;
>  }
>  
> -static int _init_foreign(const char *multipath_dir)
> +static void free_pre(void *arg)
> +{
> + regex_t **pre = arg;
> +
> + if (pre != NULL && *pre != NULL) {
> + regfree(*pre);
> + free(*pre);
> + *pre = NULL;
> + }
> +}
> +
> +static int _init_foreign(const char *multipath_dir, const char *enable)
>  {
>   char pathbuf[PATH_MAX];
>   struct dirent **di;
>   struct scandir_result sr;
>   int r, i;
> + regex_t *enable_re = NULL;
>  
>   foreigns = vector_alloc();
>   if (foreigns == NULL)
>   return -ENOMEM;
>  
> + pthread_cleanup_push(free_pre, &enable_re);
> + enable_re = calloc(1, sizeof(*enable_re));
> + if (enable_re) {
> + const char *str = enable ? enable : DEFAULT_ENABLE_FOREIGN;
> +
> + r = regcomp(enable_re, str, REG_EXTENDED|REG_NOSUB);
> + if (r != 0) {
> + char errbuf[64];
> +
> + (void)regerror(r, enable_re, errbuf, sizeof(errbuf));
> + condlog (2, "%s: error compiling en