[RFC PATCH 0/2] multipath: change default devnode blacklist

[RFC PATCH 0/2] multipath: change default devnode blacklist

[Benjamin Marzinski]

I recently got a request to add the Oracle ASM filer driver devices to
multipath's builtin devnode blacklist.  However, instead of having to do
always this for each device type individually, I decided to make
multipath blacklist all non scsi, nvme, and dasd devnodes by default.
This is what the multipath udev rules already do. If people don't like
this solution, the alternative is to add another line to the default
devnode blacklist like "^(asm/|oracleafd/|ofsctl)".

Benjamin Marzinski (2):
  libmultipath: change filter_devnode arguments
  libmultipath: change how default devnode blacklist works

 libmultipath/blacklist.c   | 21 +++++++---------
 libmultipath/blacklist.h   |  2 +-
 libmultipath/config.c      |  5 ++++
 libmultipath/config.h      |  1 +
 libmultipath/discovery.c   |  4 +--
 libmultipath/print.c       | 12 ++++++---
 libmultipath/uevent.c      |  3 +--
 multipath/main.c           |  3 +--
 multipath/multipath.conf.5 | 11 +++++---
 multipathd/cli_handlers.c  |  3 +--
 tests/blacklist.c          | 51 ++++++++++++++++++++++++++++++++------
 11 files changed, 80 insertions(+), 36 deletions(-)

-- 
2.17.2

[RFC PATCH 1/2] libmultipath: change filter_devnode arguments

[Benjamin Marzinski]

Instead of taking the blist and elist devnode vectors, filter device
now takes a config struct. This change is necessary to enable future
commits. It makes no functional changes to the code.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/blacklist.c  |  8 ++++----
 libmultipath/blacklist.h  |  2 +-
 libmultipath/discovery.c  |  4 +---
 libmultipath/print.c      |  3 +--
 libmultipath/uevent.c     |  3 +--
 multipath/main.c          |  3 +--
 multipathd/cli_handlers.c |  3 +--
 tests/blacklist.c         | 18 +++++++++++-------
 8 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
index 00e8dbdb..bc8e9e00 100644
--- a/libmultipath/blacklist.c
+++ b/libmultipath/blacklist.c
@@ -311,14 +311,14 @@ filter_device (vector blist, vector elist, char * vendor, char * product,
 }
 
 int
-filter_devnode (vector blist, vector elist, char * dev)
+filter_devnode (struct config *conf, char * dev)
 {
 	int r = MATCH_NOTHING;
 
 	if (dev) {
-		if (_blacklist_exceptions(elist, dev))
+		if (_blacklist_exceptions(conf->elist_devnode, dev))
 			r = MATCH_DEVNODE_BLIST_EXCEPT;
-		else if (_blacklist(blist, dev))
+		else if (_blacklist(conf->blist_devnode, dev))
 			r = MATCH_DEVNODE_BLIST;
 	}
 
@@ -369,7 +369,7 @@ filter_path (struct config * conf, struct path * pp)
 	r = filter_property(conf, pp->udev, 3, pp->uid_attribute);
 	if (r > 0)
 		return r;
-	r = filter_devnode(conf->blist_devnode, conf->elist_devnode, pp->dev);
+	r = filter_devnode(conf, pp->dev);
 	if (r > 0)
 		return r;
 	r = filter_device(conf->blist_device, conf->elist_device,
diff --git a/libmultipath/blacklist.h b/libmultipath/blacklist.h
index 2d721f60..938b9505 100644
--- a/libmultipath/blacklist.h
+++ b/libmultipath/blacklist.h
@@ -33,7 +33,7 @@ struct blentry_device {
 
 int setup_default_blist (struct config *);
 int alloc_ble_device (vector);
-int filter_devnode (vector, vector, char *);
+int filter_devnode (struct config *, char *);
 int filter_wwid (vector, vector, char *, char *);
 int filter_device (vector, vector, char *, char *, char *);
 int filter_path (struct config *, struct path *);
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index ffec5162..2d003359 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -2066,9 +2066,7 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 			return PATHINFO_SKIPPED;
 	}
 
-	if (filter_devnode(conf->blist_devnode,
-			   conf->elist_devnode,
-			   pp->dev) > 0)
+	if (filter_devnode(conf, pp->dev) > 0)
 		return PATHINFO_SKIPPED;
 
 	condlog(4, "%s: mask = 0x%x", pp->dev, mask);
diff --git a/libmultipath/print.c b/libmultipath/print.c
index 298b3764..ee79a9d0 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -2071,8 +2071,7 @@ int snprint_devices(struct config *conf, char * buff, int len,
 				blkdev->d_name);
 		pp = find_path_by_dev(vecs->pathvec, blkdev->d_name);
 		if (!pp) {
-			r = filter_devnode(conf->blist_devnode,
-					   conf->elist_devnode, blkdev->d_name);
+			r = filter_devnode(conf, blkdev->d_name);
 			if (r > 0)
 				fwd += snprintf(buff + fwd, len - fwd,
 						" devnode blacklisted, unmonitored");
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index e0d13b11..05937306 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -202,8 +202,7 @@ uevent_can_discard(struct uevent *uev)
 	 */
 	conf = get_multipath_config();
 	pthread_cleanup_push(put_multipath_config, conf);
-	if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
-			   uev->kernel) > 0)
+	if (filter_devnode(conf, uev->kernel) > 0)
 		invalid = 1;
 	pthread_cleanup_pop(1);
 
diff --git a/multipath/main.c b/multipath/main.c
index 953fab27..b235ad66 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -565,8 +565,7 @@ configure (struct config *conf, enum mpath_cmds cmd,
 	if (dev && (dev_type == DEV_DEVNODE ||
 		    dev_type == DEV_UEVENT) &&
 	    cmd != CMD_REMOVE_WWID &&
-	    (filter_devnode(conf->blist_devnode,
-			    conf->elist_devnode, dev) > 0)) {
+	    (filter_devnode(conf, dev) > 0)) {
 		goto out;
 	}
 
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 31c3d9fd..bda81184 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -705,8 +705,7 @@ cli_add_path (void * v, char ** reply, int * len, void * data)
 	condlog(2, "%s: add path (operator)", param);
 	conf = get_multipath_config();
 	pthread_cleanup_push(put_multipath_config, conf);
-	if (filter_devnode(conf->blist_devnode, conf->elist_devnode,
-			   param) > 0)
+	if (filter_devnode(conf, param) > 0)
 		invalid = 1;
 	pthread_cleanup_pop(1);
 	if (invalid)
diff --git a/tests/blacklist.c b/tests/blacklist.c
index 6e7c1864..0ae82592 100644
--- a/tests/blacklist.c
+++ b/tests/blacklist.c
@@ -138,24 +138,28 @@ static int reset_blists(void **state)
 
 static void test_devnode_blacklist(void **state)
 {
+	conf.blist_devnode = blist_devnode_sdb;
 	expect_condlog(3, "sdb: device node name blacklisted\n");
-	assert_int_equal(filter_devnode(blist_devnode_sdb, NULL, "sdb"),
+	assert_int_equal(filter_devnode(&conf, "sdb"),
 			 MATCH_DEVNODE_BLIST);
 }
 
 static void test_devnode_whitelist(void **state)
 {
+	conf.blist_devnode = blist_all;
+	conf.elist_devnode = blist_devnode_sdb;
 	expect_condlog(3, "sdb: device node name whitelisted\n");
-	assert_int_equal(filter_devnode(blist_all, blist_devnode_sdb, "sdb"),
+	assert_int_equal(filter_devnode(&conf, "sdb"),
 			 MATCH_DEVNODE_BLIST_EXCEPT);
 	expect_condlog(3, "sdc: device node name blacklisted\n");
-	assert_int_equal(filter_devnode(blist_all, blist_devnode_sdb, "sdc"),
+	assert_int_equal(filter_devnode(&conf, "sdc"),
 			 MATCH_DEVNODE_BLIST);
 }
 
 static void test_devnode_missing(void **state)
 {
-	assert_int_equal(filter_devnode(blist_devnode_sdb, NULL, "sdc"),
+	conf.blist_devnode = blist_devnode_sdb;
+	assert_int_equal(filter_devnode(&conf, "sdc"),
 			 MATCH_NOTHING);
 }
 
@@ -481,9 +485,9 @@ static void test_filter_path_whitelist_wwid(void **state)
 int test_blacklist(void)
 {
 	const struct CMUnitTest tests[] = {
-		cmocka_unit_test(test_devnode_blacklist),
-		cmocka_unit_test(test_devnode_whitelist),
-		cmocka_unit_test(test_devnode_missing),
+		test_and_reset(test_devnode_blacklist),
+		test_and_reset(test_devnode_whitelist),
+		test_and_reset(test_devnode_missing),
 		cmocka_unit_test(test_device_blacklist),
 		cmocka_unit_test(test_device_whitelist),
 		cmocka_unit_test(test_device_missing),
-- 
2.17.2

[RFC PATCH 2/2] libmultipath: change how default devnode blacklist works

[Benjamin Marzinski]

The number of devices that multipath needs to blacklist keeps growing,
and the udev rules already have

KERNEL!="sd*|dasd*|nvme*", GOTO="end_mpath"

so they only work correctly with these device types.  Instead of
individually blacklisting every type of device that can't be
multipathed, multipath's default blacklist should work like the udev
rule, and blacklist all devices that aren't scsi, dasd, or nvme.
Unfortunately, the c regex library doesn't support negative lookahead.
Instead, multipath needs a special regex list that it can check.  If the
denode doesn't match any rule in the list, it will be blacklisted. This
is checked after the devnode exceptions, so it is still possible to add
other devices via the exceptions.

It is not possible (or necessary) to directly set the special regex list
in /etc/multipath.conf. Because of this, there is no good way to show it
with "multipathd show config" or "multipath -t", since those are
expected to print a valid condig. It is displayed in the "multipathd
show blacklist" output, however.

The blacklist unit tests have also been updated to deal with the
changes.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/blacklist.c   | 13 +++++--------
 libmultipath/config.c      |  5 +++++
 libmultipath/config.h      |  1 +
 libmultipath/print.c       |  9 ++++++++-
 multipath/multipath.conf.5 | 11 +++++++----
 tests/blacklist.c          | 33 +++++++++++++++++++++++++++++++++
 6 files changed, 59 insertions(+), 13 deletions(-)

diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
index bc8e9e00..ae74b6b7 100644
--- a/libmultipath/blacklist.c
+++ b/libmultipath/blacklist.c
@@ -192,16 +192,10 @@ setup_default_blist (struct config * conf)
 	char * str;
 	int i;
 
-	str = STRDUP("^(ram|zram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-9]");
+	str = STRDUP("^(sd[a-z]|dasd[a-z]|nvme[0-9])");
 	if (!str)
 		return 1;
-	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
-		return 1;
-
-	str = STRDUP("^(td|hd|vd)[a-z]");
-	if (!str)
-		return 1;
-	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
+	if (store_ble(conf->elist_devnode_default, str, ORIGIN_DEFAULT))
 		return 1;
 
 	str = STRDUP("(SCSI_IDENT_|ID_WWN)");
@@ -318,6 +312,9 @@ filter_devnode (struct config *conf, char * dev)
 	if (dev) {
 		if (_blacklist_exceptions(conf->elist_devnode, dev))
 			r = MATCH_DEVNODE_BLIST_EXCEPT;
+		else if (!_blacklist_exceptions(conf->elist_devnode_default,
+						dev))
+			r = MATCH_DEVNODE_BLIST;
 		else if (_blacklist(conf->blist_devnode, dev))
 			r = MATCH_DEVNODE_BLIST;
 	}
diff --git a/libmultipath/config.c b/libmultipath/config.c
index b4d87689..b695f01b 100644
--- a/libmultipath/config.c
+++ b/libmultipath/config.c
@@ -632,6 +632,7 @@ free_config (struct config * conf)
 	if (conf->config_dir)
 		FREE(conf->config_dir);
 
+	free_blacklist(conf->elist_devnode_default);
 	free_blacklist(conf->blist_devnode);
 	free_blacklist(conf->blist_wwid);
 	free_blacklist(conf->blist_property);
@@ -803,6 +804,10 @@ load_config (char * file)
 	condlog(3, "polling interval: %d, max: %d",
 		conf->checkint, conf->max_checkint);
 
+	conf->elist_devnode_default = vector_alloc();
+	if (!conf->elist_devnode_default)
+		goto out;
+
 	if (conf->blist_devnode == NULL) {
 		conf->blist_devnode = vector_alloc();
 
diff --git a/libmultipath/config.h b/libmultipath/config.h
index ceecff2d..49fae19f 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -223,6 +223,7 @@ struct config {
 	vector blist_device;
 	vector blist_property;
 	vector blist_protocol;
+	vector elist_devnode_default;
 	vector elist_devnode;
 	vector elist_wwid;
 	vector elist_device;
diff --git a/libmultipath/print.c b/libmultipath/print.c
index ee79a9d0..c003ee40 100644
--- a/libmultipath/print.c
+++ b/libmultipath/print.c
@@ -1702,7 +1702,14 @@ int snprint_blacklist_report(struct config *conf, char *buff, int len)
 	if ((len - fwd - threshold) <= 0)
 		return len;
 	fwd += snprintf(buff + fwd, len - fwd, "device node rules:\n"
-					       "- blacklist:\n");
+					       "- allowed:\n");
+	if (!snprint_blacklist_group(buff, len, &fwd,
+				     &conf->elist_devnode_default))
+		return len;
+
+	if ((len - fwd - threshold) <= 0)
+		return len;
+	fwd += snprintf(buff + fwd, len - fwd, "- blacklist:\n");
 	if (!snprint_blacklist_group(buff, len, &fwd, &conf->blist_devnode))
 		return len;
 
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 05a5e8ff..d0b8c5c0 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -1264,10 +1264,13 @@ unless explicitly stated.
 Regular expression matching the device nodes to be excluded/included.
 .RS
 .PP
-The default \fIblacklist\fR consists of the regular expressions
-"^(ram|zram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-9]" and
-"^(td|hd|vd)[a-z]". This causes virtual devices, non-disk devices, and some other
-device types to be excluded from multipath handling by default.
+By default, multipath excludes devnodes that do not match the regular
+expression "^(sd[a-z]|dasd[a-z]|nvme[0-9])". This works differently than how
+the user defined expressions work in either the \fIblacklist\fR section (which
+excludes devnodes that match the expression) or the \fIblacklist_exceptions\fR
+section (which includes devnodes that match the regular expression). For
+multipath to include devices that are not scsi, dasd, or nvme, the devnodes
+must be added to the \fIblacklist_exceptions\fR section.
 .RE
 .TP
 .B wwid
diff --git a/tests/blacklist.c b/tests/blacklist.c
index 0ae82592..8db3d394 100644
--- a/tests/blacklist.c
+++ b/tests/blacklist.c
@@ -60,6 +60,8 @@ __wrap_udev_list_entry_get_name(struct udev_list_entry *list_entry)
 	return *(const char **)list_entry;
 }
 
+vector elist_devnode_default;
+vector elist_property_default;
 vector blist_devnode_sdb;
 vector blist_all;
 vector blist_device_foo_bar;
@@ -70,6 +72,20 @@ vector blist_property_wwn;
 
 static int setup(void **state)
 {
+	struct config conf;
+
+	memset(&conf, 0, sizeof(conf));
+	conf.elist_devnode_default = vector_alloc();
+	if (!conf.elist_devnode_default)
+		return -1;
+	conf.elist_property = vector_alloc();
+	if (!conf.elist_property)
+		return -1;
+	if (setup_default_blist(&conf) != 0)
+		return -1;
+	elist_property_default = conf.elist_property;
+	elist_devnode_default = conf.elist_devnode_default;
+
 	blist_devnode_sdb = vector_alloc();
 	if (!blist_devnode_sdb ||
 	    store_ble(blist_devnode_sdb, strdup("sdb"), ORIGIN_CONFIG))
@@ -111,6 +127,8 @@ static int setup(void **state)
 
 static int teardown(void **state)
 {
+	free_blacklist(elist_devnode_default);
+	free_blacklist(elist_property_default);
 	free_blacklist(blist_devnode_sdb);
 	free_blacklist(blist_all);
 	free_blacklist_device(blist_device_foo_bar);
@@ -123,6 +141,7 @@ static int teardown(void **state)
 
 static int reset_blists(void **state)
 {
+	conf.elist_devnode_default = NULL;
 	conf.blist_devnode = NULL;
 	conf.blist_wwid = NULL;
 	conf.blist_property = NULL;
@@ -159,8 +178,16 @@ static void test_devnode_whitelist(void **state)
 static void test_devnode_missing(void **state)
 {
 	conf.blist_devnode = blist_devnode_sdb;
+	conf.elist_devnode_default = elist_devnode_default;
 	assert_int_equal(filter_devnode(&conf, "sdc"),
 			 MATCH_NOTHING);
+	assert_int_equal(filter_devnode(&conf, "nvme0n1"),
+			 MATCH_NOTHING);
+	assert_int_equal(filter_devnode(&conf, "dasda"),
+			 MATCH_NOTHING);
+	expect_condlog(3, "hda: device node name blacklisted\n");
+	assert_int_equal(filter_devnode(&conf, "hda"),
+			 MATCH_DEVNODE_BLIST);
 }
 
 static void test_device_blacklist(void **state)
@@ -302,6 +329,7 @@ static void test_filter_path_devnode(void **state)
 	/* always must include property elist, to avoid "missing property"
 	 * blacklisting */
 	conf.elist_property = blist_property_wwn;
+	conf.elist_devnode_default = elist_devnode_default;
 	conf.blist_devnode = blist_devnode_sdb;
 	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
 	expect_condlog(3, "sdb: device node name blacklisted\n");
@@ -313,6 +341,7 @@ static void test_filter_path_device(void **state)
 	/* always must include property elist, to avoid "missing property"
 	 * blacklisting */
 	conf.elist_property = blist_property_wwn;
+	conf.elist_devnode_default = elist_devnode_default;
 	conf.blist_device = blist_device_foo_bar;
 	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
 	expect_condlog(3, "sdb: (foo:bar) vendor/product blacklisted\n");
@@ -322,6 +351,7 @@ static void test_filter_path_device(void **state)
 static void test_filter_path_protocol(void **state)
 {
 	conf.elist_property = blist_property_wwn;
+	conf.elist_devnode_default = elist_devnode_default;
 	conf.blist_protocol = blist_protocol_fcp;
 	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
 	expect_condlog(3, "sdb: protocol scsi:fcp blacklisted\n");
@@ -331,6 +361,7 @@ static void test_filter_path_protocol(void **state)
 static void test_filter_path_wwid(void **state)
 {
 	conf.elist_property = blist_property_wwn;
+	conf.elist_devnode_default = elist_devnode_default;
 	conf.blist_wwid = blist_wwid_xyzzy;
 	expect_condlog(3, "sdb: udev property ID_WWN whitelisted\n");
 	expect_condlog(3, "sdb: wwid xyzzy blacklisted\n");
@@ -377,6 +408,7 @@ static void test_filter_path_missing1(void **state)
 static void test_filter_path_missing2(void **state)
 {
 	conf.elist_property = blist_property_wwn;
+	conf.elist_devnode_default = elist_devnode_default;
 	conf.blist_devnode = blist_devnode_sdb;
 	conf.blist_device = blist_device_foo_bar;
 	conf.blist_protocol = blist_protocol_fcp;
@@ -391,6 +423,7 @@ static void test_filter_path_missing2(void **state)
 static void test_filter_path_missing3(void **state)
 {
 	conf.blist_property = blist_property_wwn;
+	conf.elist_devnode_default = elist_devnode_default;
 	conf.blist_devnode = blist_devnode_sdb;
 	conf.blist_device = blist_device_foo_bar;
 	conf.blist_protocol = blist_protocol_fcp;
-- 
2.17.2

Re: [RFC PATCH 0/2] multipath: change default devnode blacklist

[Martin Wilck]

Hi Ben,

On Thu, 2020-06-04 at 19:30 -0500, Benjamin Marzinski wrote:
> I recently got a request to add the Oracle ASM filer driver devices
> to
> multipath's builtin devnode blacklist.  However, instead of having to
> do
> always this for each device type individually, I decided to make
> multipath blacklist all non scsi, nvme, and dasd devnodes by default.
> This is what the multipath udev rules already do. If people don't
> like
> this solution, the alternative is to add another line to the default
> devnode blacklist like "^(asm/|oracleafd/|ofsctl)".

Thanks, this looks ok. But I'd like to propose an alternative idea:
Extend the RE syntax in our config file to allow negated regular 
expressions. Like this:

blacklist {
    devnode "!(^(sd[a-z]|dasd[a-z]|nvme[0-9]))"
}

The "!(${RE})" construct would mean "everything that does not match ${RE}".
This logic would only be applied to an entire regex.
If a user needs a RE matching with "!(" and ending with ")", she can escape 
the exclamation mark "\!(like this)".

AFAICS this could be implemented quite easily (by adding a "bool negate" field
in struct blentry and some simple parser logic), and could be applied to other 
REs in the config file as well. We could print this with "multipath -t", and 
we wouldn't need to document an exception.

It's also pretty much backwards-compatible, I don't think many people use
regexes starting with "!(" for multipath these days.

Regards,
Martin

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

Re: [RFC PATCH 0/2] multipath: change default devnode blacklist

[Benjamin Marzinski]

On Fri, Jun 05, 2020 at 07:20:26PM +0000, Martin Wilck wrote:
> Hi Ben,
> 
> On Thu, 2020-06-04 at 19:30 -0500, Benjamin Marzinski wrote:
> > I recently got a request to add the Oracle ASM filer driver devices
> > to
> > multipath's builtin devnode blacklist.  However, instead of having to
> > do
> > always this for each device type individually, I decided to make
> > multipath blacklist all non scsi, nvme, and dasd devnodes by default.
> > This is what the multipath udev rules already do. If people don't
> > like
> > this solution, the alternative is to add another line to the default
> > devnode blacklist like "^(asm/|oracleafd/|ofsctl)".
> 
> Thanks, this looks ok. But I'd like to propose an alternative idea:
> Extend the RE syntax in our config file to allow negated regular 
> expressions. Like this:
> 
> blacklist {
>     devnode "!(^(sd[a-z]|dasd[a-z]|nvme[0-9]))"
> }
> 
> The "!(${RE})" construct would mean "everything that does not match ${RE}".
> This logic would only be applied to an entire regex.
> If a user needs a RE matching with "!(" and ending with ")", she can escape 
> the exclamation mark "\!(like this)".
> 
> AFAICS this could be implemented quite easily (by adding a "bool negate" field
> in struct blentry and some simple parser logic), and could be applied to other 
> REs in the config file as well. We could print this with "multipath -t", and 
> we wouldn't need to document an exception.
> 
> It's also pretty much backwards-compatible, I don't think many people use
> regexes starting with "!(" for multipath these days.

Sure, but since we can only really support negating the whole regular
expression, and c regular expressions don't treat '!' as a special
character, why do we need the prentheses around the regular expression?
It seems like we can just treat regular expressions starting with '!' as
negated, and ones starting with "\!" as starting with a literal '!'.

Do you think that there is much chance that users have blacklist strings
that start with '!'? There are no devnodes, udev properties, or
protocols that start with that.  I don't know of a UUID format that has
an exclamation point, and while it's possible that a product string
starts with one, it seems really unlikely.

-Ben

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

Re: [RFC PATCH 0/2] multipath: change default devnode blacklist

[Martin Wilck]

On Fri, 2020-06-05 at 17:27 -0500, Benjamin Marzinski wrote:
> On Fri, Jun 05, 2020 at 07:20:26PM +0000, Martin Wilck wrote:
> > Hi Ben,
> > 
> > On Thu, 2020-06-04 at 19:30 -0500, Benjamin Marzinski wrote:
> > > I recently got a request to add the Oracle ASM filer driver
> > > devices
> > > to
> > > multipath's builtin devnode blacklist.  However, instead of
> > > having to
> > > do
> > > always this for each device type individually, I decided to make
> > > multipath blacklist all non scsi, nvme, and dasd devnodes by
> > > default.
> > > This is what the multipath udev rules already do. If people don't
> > > like
> > > this solution, the alternative is to add another line to the
> > > default
> > > devnode blacklist like "^(asm/|oracleafd/|ofsctl)".
> > 
> > Thanks, this looks ok. But I'd like to propose an alternative idea:
> > Extend the RE syntax in our config file to allow negated regular 
> > expressions. Like this:
> > 
> > blacklist {
> >     devnode "!(^(sd[a-z]|dasd[a-z]|nvme[0-9]))"
> > }
> > 
> > The "!(${RE})" construct would mean "everything that does not match
> > ${RE}".
> > This logic would only be applied to an entire regex.
> > If a user needs a RE matching with "!(" and ending with ")", she
> > can escape 
> > the exclamation mark "\!(like this)".
> > 
> > AFAICS this could be implemented quite easily (by adding a "bool
> > negate" field
> > in struct blentry and some simple parser logic), and could be
> > applied to other 
> > REs in the config file as well. We could print this with "multipath
> > -t", and 
> > we wouldn't need to document an exception.
> > 
> > It's also pretty much backwards-compatible, I don't think many
> > people use
> > regexes starting with "!(" for multipath these days.
> 
> Sure, but since we can only really support negating the whole regular
> expression, and c regular expressions don't treat '!' as a special
> character, why do we need the prentheses around the regular
> expression?
> It seems like we can just treat regular expressions starting with '!'
> as
> negated, and ones starting with "\!" as starting with a literal '!'.
> 
> Do you think that there is much chance that users have blacklist
> strings
> that start with '!'? There are no devnodes, udev properties, or
> protocols that start with that.  I don't know of a UUID format that
> has
> an exclamation point, and while it's possible that a product string
> starts with one, it seems really unlikely.
> 

Sure, just using "!" would be fine, too, and simpler. Does this mean
you agree with my proposal in general?

Martin

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

Re: [RFC PATCH 0/2] multipath: change default devnode blacklist

[Benjamin Marzinski]

On Sat, Jun 06, 2020 at 07:23:28PM +0000, Martin Wilck wrote:
> On Fri, 2020-06-05 at 17:27 -0500, Benjamin Marzinski wrote:
> > On Fri, Jun 05, 2020 at 07:20:26PM +0000, Martin Wilck wrote:
> > > Hi Ben,
> > > 
> > > On Thu, 2020-06-04 at 19:30 -0500, Benjamin Marzinski wrote:
> > > > I recently got a request to add the Oracle ASM filer driver
> > > > devices
> > > > to
> > > > multipath's builtin devnode blacklist.  However, instead of
> > > > having to
> > > > do
> > > > always this for each device type individually, I decided to make
> > > > multipath blacklist all non scsi, nvme, and dasd devnodes by
> > > > default.
> > > > This is what the multipath udev rules already do. If people don't
> > > > like
> > > > this solution, the alternative is to add another line to the
> > > > default
> > > > devnode blacklist like "^(asm/|oracleafd/|ofsctl)".
> > > 
> > > Thanks, this looks ok. But I'd like to propose an alternative idea:
> > > Extend the RE syntax in our config file to allow negated regular 
> > > expressions. Like this:
> > > 
> > > blacklist {
> > >     devnode "!(^(sd[a-z]|dasd[a-z]|nvme[0-9]))"
> > > }
> > > 
> > > The "!(${RE})" construct would mean "everything that does not match
> > > ${RE}".
> > > This logic would only be applied to an entire regex.
> > > If a user needs a RE matching with "!(" and ending with ")", she
> > > can escape 
> > > the exclamation mark "\!(like this)".
> > > 
> > > AFAICS this could be implemented quite easily (by adding a "bool
> > > negate" field
> > > in struct blentry and some simple parser logic), and could be
> > > applied to other 
> > > REs in the config file as well. We could print this with "multipath
> > > -t", and 
> > > we wouldn't need to document an exception.
> > > 
> > > It's also pretty much backwards-compatible, I don't think many
> > > people use
> > > regexes starting with "!(" for multipath these days.
> > 
> > Sure, but since we can only really support negating the whole regular
> > expression, and c regular expressions don't treat '!' as a special
> > character, why do we need the prentheses around the regular
> > expression?
> > It seems like we can just treat regular expressions starting with '!'
> > as
> > negated, and ones starting with "\!" as starting with a literal '!'.
> > 
> > Do you think that there is much chance that users have blacklist
> > strings
> > that start with '!'? There are no devnodes, udev properties, or
> > protocols that start with that.  I don't know of a UUID format that
> > has
> > an exclamation point, and while it's possible that a product string
> > starts with one, it seems really unlikely.
> > 
> 
> Sure, just using "!" would be fine, too, and simpler. Does this mean
> you agree with my proposal in general?

yeah. I'll be posting a v2 shortly.

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