multipath: remove duplicates from multipath configuration

PATCH In review Benjamin Marzinski 2 participants 3 messages Jan 10, 2013 – Jan 11, 2013 Device mapper

Share & tools

Benjamin Marzinski

author maintainer

2013-01-10 20:10 UTC #0 lore raw

PATCH v2 1/1 multipath: remove duplicates from multipath configuration

Added code to remove duplcate entries in the devices section, and the blacklist devices section of the builtin configuration table. The only change to setup_default_blist is the addition of _blacklist_device() to check if the device's bl_product entry already exists.

Signed-off-by Benjamin Marzinski <[email protected]>

---
 libmultipath/blacklist.c |   91 ++++++++++++++++++++++++-----------------------
 libmultipath/config.c    |   16 ++++++--
 2 files changed, 60 insertions(+), 47 deletions(-)

libmultipath/blacklist.c +47 ~~-44~~

Index: multipath-tools-120821/libmultipath/blacklist.c
===================================================================
@@ -96,50 +96,6 @@ set_ble_device (vector blist, char * ven
 }
 
 int
-setup_default_blist (struct config * conf)
-{
-	struct blentry * ble;
-	struct hwentry *hwe;
-	char * str;
-	int i;
-
-	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st)[0-9]*");
-	if (!str)
-		return 1;
-	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
-		return 1;
-
-	str = STRDUP("^hd[a-z]");
-	if (!str)
-		return 1;
-	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
-		return 1;
-
-	str = STRDUP("^dcssblk[0-9]*");
-	if (!str)
-		return 1;
-	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
-		return 1;
-
-	vector_foreach_slot (conf->hwtable, hwe, i) {
-		if (hwe->bl_product) {
-			if (alloc_ble_device(conf->blist_device))
-				return 1;
-			ble = VECTOR_SLOT(conf->blist_device,
-					  VECTOR_SIZE(conf->blist_device) -1);
-			if (set_ble_device(conf->blist_device,
-					   STRDUP(hwe->vendor),
-					   STRDUP(hwe->bl_product),
-					   ORIGIN_DEFAULT)) {
-				FREE(ble);
-				return 1;
-			}
-		}
-	}
-	return 0;
-}
-
-int
 _blacklist_exceptions (vector elist, char * str)
 {
 	int i;

@@ -192,6 +148,53 @@ _blacklist_device (vector blist, char *
 	}
 	return 0;
 }
+
+int
+setup_default_blist (struct config * conf)
+{
+	struct blentry * ble;
+	struct hwentry *hwe;
+	char * str;
+	int i;
+
+	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st)[0-9]*");
+	if (!str)
+		return 1;
+	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
+		return 1;
+
+	str = STRDUP("^hd[a-z]");
+	if (!str)
+		return 1;
+	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
+		return 1;
+
+	str = STRDUP("^dcssblk[0-9]*");
+	if (!str)
+		return 1;
+	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
+		return 1;
+
+	vector_foreach_slot (conf->hwtable, hwe, i) {
+		if (hwe->bl_product) {
+			if (_blacklist_device(conf->blist_device, hwe->vendor,
+					      hwe->bl_product))
+				continue;
+			if (alloc_ble_device(conf->blist_device))
+				return 1;
+			ble = VECTOR_SLOT(conf->blist_device,
+					  VECTOR_SIZE(conf->blist_device) -1);
+			if (set_ble_device(conf->blist_device,
+					   STRDUP(hwe->vendor),
+					   STRDUP(hwe->bl_product),
+					   ORIGIN_DEFAULT)) {
+				FREE(ble);
+				return 1;
+			}
+		}
+	}
+	return 0;
+}
 
 #define LOG_BLIST(M) \
 	if (vendor && product)						 \

libmultipath/config.c +13 -3

Index: multipath-tools-120821/libmultipath/config.c
===================================================================
@@ -25,13 +25,16 @@
 static int
 hwe_strmatch (struct hwentry *hwe1, struct hwentry *hwe2)
 {
-	if (hwe1->vendor && hwe2->vendor && strcmp(hwe1->vendor, hwe2->vendor))
+	if ((!!(hwe1->vendor) != !!(hwe2->vendor)) ||
+	    (hwe1->vendor && strcmp(hwe1->vendor, hwe2->vendor)))
 		return 1;
 
-	if (hwe1->product && hwe2->product && strcmp(hwe1->product, hwe2->product))
+	if ((!!(hwe1->product) != !!(hwe2->product)) ||
+	    (hwe1->product && strcmp(hwe1->product, hwe2->product)))
 		return 1;
 
-	if (hwe1->revision && hwe2->revision && strcmp(hwe1->revision, hwe2->revision))
+	if ((!!(hwe1->revision) != !!(hwe2->revision)) ||
+	    (hwe1->revision && strcmp(hwe1->revision, hwe2->revision)))
 		return 1;
 
 	return 0;

@@ -416,6 +419,13 @@ factorize_hwtable (vector hw, int n)
 				continue;
 			/* dup */
 			merge_hwe(hwe2, hwe1);
+			if (hwe_strmatch(hwe2, hwe1) == 0) {
+				vector_del_slot(hw, i);
+				free_hwe(hwe1);
+				n -= 1;
+				i -= 1;
+				j -= 1;
+			}
 		}
 	}
 	return 0;

Hannes Reinecke

2013-01-11 07:15 UTC #1 lore raw

Quoted 134 lines · Benjamin Marzinski

Signed-off-by Benjamin Marzinski <[email protected]>

---
  libmultipath/blacklist.c |   91 ++++++++++++++++++++++++-----------------------
  libmultipath/config.c    |   16 ++++++--
  2 files changed, 60 insertions(+), 47 deletions(-)

Index: multipath-tools-120821/libmultipath/blacklist.c
===================================================================

libmultipath/blacklist.c +47 ~~-44~~

@@ -96,50 +96,6 @@ set_ble_device (vector blist, char * ven
  }
 
  int
-setup_default_blist (struct config * conf)
-{
-	struct blentry * ble;
-	struct hwentry *hwe;
-	char * str;
-	int i;
-
-	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st)[0-9]*");
-	if (!str)
-		return 1;
-	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
-		return 1;
-
-	str = STRDUP("^hd[a-z]");
-	if (!str)
-		return 1;
-	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
-		return 1;
-
-	str = STRDUP("^dcssblk[0-9]*");
-	if (!str)
-		return 1;
-	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
-		return 1;
-
-	vector_foreach_slot (conf->hwtable, hwe, i) {
-		if (hwe->bl_product) {
-			if (alloc_ble_device(conf->blist_device))
-				return 1;
-			ble = VECTOR_SLOT(conf->blist_device,
-					  VECTOR_SIZE(conf->blist_device) -1);
-			if (set_ble_device(conf->blist_device,
-					   STRDUP(hwe->vendor),
-					   STRDUP(hwe->bl_product),
-					   ORIGIN_DEFAULT)) {
-				FREE(ble);
-				return 1;
-			}
-		}
-	}
-	return 0;
-}
-
-int
  _blacklist_exceptions (vector elist, char * str)
  {
  	int i;

@@ -192,6 +148,53 @@ _blacklist_device (vector blist, char *
  	}
  	return 0;
  }
+
+int
+setup_default_blist (struct config * conf)
+{
+	struct blentry * ble;
+	struct hwentry *hwe;
+	char * str;
+	int i;
+
+	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st)[0-9]*");
+	if (!str)
+		return 1;
+	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
+		return 1;
+
+	str = STRDUP("^hd[a-z]");
+	if (!str)
+		return 1;
+	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
+		return 1;
+
+	str = STRDUP("^dcssblk[0-9]*");
+	if (!str)
+		return 1;
+	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
+		return 1;
+
+	vector_foreach_slot (conf->hwtable, hwe, i) {
+		if (hwe->bl_product) {
+			if (_blacklist_device(conf->blist_device, hwe->vendor,
+					      hwe->bl_product))
+				continue;
+			if (alloc_ble_device(conf->blist_device))
+				return 1;
+			ble = VECTOR_SLOT(conf->blist_device,
+					  VECTOR_SIZE(conf->blist_device) -1);
+			if (set_ble_device(conf->blist_device,
+					   STRDUP(hwe->vendor),
+					   STRDUP(hwe->bl_product),
+					   ORIGIN_DEFAULT)) {
+				FREE(ble);
+				return 1;
+			}
+		}
+	}
+	return 0;
+}
 
  #define LOG_BLIST(M) \
  	if (vendor && product)						 \

Index: multipath-tools-120821/libmultipath/config.c
===================================================================

libmultipath/config.c +2 -2

@@ -25,13 +25,16 @@
  static int
  hwe_strmatch (struct hwentry *hwe1, struct hwentry *hwe2)
  {
-	if (hwe1->vendor && hwe2->vendor && strcmp(hwe1->vendor, hwe2->vendor))
+	if ((!!(hwe1->vendor) != !!(hwe2->vendor)) ||
+	    (hwe1->vendor && strcmp(hwe1->vendor, hwe2->vendor)))
  		return 1;
 
-	if (hwe1->product && hwe2->product && strcmp(hwe1->product, hwe2->product))

libmultipath/config.c line 40

Quoted 10 lines

libmultipath/config.c +4 -1

+	if ((!!(hwe1->product) != !!(hwe2->product)) ||
+	    (hwe1->product && strcmp(hwe1->product, hwe2->product)))
  		return 1;
 
-	if (hwe1->revision && hwe2->revision && strcmp(hwe1->revision, hwe2->revision))
+	if ((!!(hwe1->revision) != !!(hwe2->revision)) ||
+	    (hwe1->revision && strcmp(hwe1->revision, hwe2->revision)))
  		return 1;
 
  	return 0;

I hate the '!!' constructs.

Quoted 5 lines

libmultipath/config.c +0 -0

@@ -416,6 +419,13 @@ factorize_hwtable (vector hw, int n)
  				continue;
  			/* dup */
  			merge_hwe(hwe2, hwe1);

libmultipath/config.c factorize_hwtable (vector hw, int n) · line 431

Quoted 10 lines

libmultipath/config.c +7 -0

+			if (hwe_strmatch(hwe2, hwe1) == 0) {
+				vector_del_slot(hw, i);
+				free_hwe(hwe1);
+				n -= 1;
+				i -= 1;
+				j -= 1;
+			}
  		}
  	}
  	return 0;

Is the 'hwe_strmatch' required here?
Also I'm not sure if just decrementing the indices is the correct way of doing things; I had a version which just restarted the outer loop if we encountered a duplicate.

Cheers,

Hannes

Signature · 4 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Benjamin Marzinski

author maintainer

2013-01-11 17:52 UTC #2 lore raw

Quoted 139 lines · Hannes Reinecke

Quoted 138 lines · Benjamin Marzinski

Signed-off-by Benjamin Marzinski <[email protected]>

---
  libmultipath/blacklist.c |   91 ++++++++++++++++++++++++-----------------------
  libmultipath/config.c    |   16 ++++++--
  2 files changed, 60 insertions(+), 47 deletions(-)

Index: multipath-tools-120821/libmultipath/blacklist.c
===================================================================

libmultipath/blacklist.c +47 ~~-44~~

@@ -96,50 +96,6 @@ set_ble_device (vector blist, char * ven
  }
 
  int
-setup_default_blist (struct config * conf)
-{
-	struct blentry * ble;
-	struct hwentry *hwe;
-	char * str;
-	int i;
-
-	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st)[0-9]*");
-	if (!str)
-		return 1;
-	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
-		return 1;
-
-	str = STRDUP("^hd[a-z]");
-	if (!str)
-		return 1;
-	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
-		return 1;
-
-	str = STRDUP("^dcssblk[0-9]*");
-	if (!str)
-		return 1;
-	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
-		return 1;
-
-	vector_foreach_slot (conf->hwtable, hwe, i) {
-		if (hwe->bl_product) {
-			if (alloc_ble_device(conf->blist_device))
-				return 1;
-			ble = VECTOR_SLOT(conf->blist_device,
-					  VECTOR_SIZE(conf->blist_device) -1);
-			if (set_ble_device(conf->blist_device,
-					   STRDUP(hwe->vendor),
-					   STRDUP(hwe->bl_product),
-					   ORIGIN_DEFAULT)) {
-				FREE(ble);
-				return 1;
-			}
-		}
-	}
-	return 0;
-}
-
-int
  _blacklist_exceptions (vector elist, char * str)
  {
  	int i;

@@ -192,6 +148,53 @@ _blacklist_device (vector blist, char *
  	}
  	return 0;
  }
+
+int
+setup_default_blist (struct config * conf)
+{
+	struct blentry * ble;
+	struct hwentry *hwe;
+	char * str;
+	int i;
+
+	str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st)[0-9]*");
+	if (!str)
+		return 1;
+	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
+		return 1;
+
+	str = STRDUP("^hd[a-z]");
+	if (!str)
+		return 1;
+	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
+		return 1;
+
+	str = STRDUP("^dcssblk[0-9]*");
+	if (!str)
+		return 1;
+	if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
+		return 1;
+
+	vector_foreach_slot (conf->hwtable, hwe, i) {
+		if (hwe->bl_product) {
+			if (_blacklist_device(conf->blist_device, hwe->vendor,
+					      hwe->bl_product))
+				continue;
+			if (alloc_ble_device(conf->blist_device))
+				return 1;
+			ble = VECTOR_SLOT(conf->blist_device,
+					  VECTOR_SIZE(conf->blist_device) -1);
+			if (set_ble_device(conf->blist_device,
+					   STRDUP(hwe->vendor),
+					   STRDUP(hwe->bl_product),
+					   ORIGIN_DEFAULT)) {
+				FREE(ble);
+				return 1;
+			}
+		}
+	}
+	return 0;
+}
 
  #define LOG_BLIST(M) \
  	if (vendor && product)						 \

Index: multipath-tools-120821/libmultipath/config.c
===================================================================

libmultipath/config.c +4 -2

@@ -25,13 +25,16 @@
  static int
  hwe_strmatch (struct hwentry *hwe1, struct hwentry *hwe2)
  {
-	if (hwe1->vendor && hwe2->vendor && strcmp(hwe1->vendor, hwe2->vendor))
+	if ((!!(hwe1->vendor) != !!(hwe2->vendor)) ||
+	    (hwe1->vendor && strcmp(hwe1->vendor, hwe2->vendor)))
  		return 1;
 
-	if (hwe1->product && hwe2->product && strcmp(hwe1->product, hwe2->product))
+	if ((!!(hwe1->product) != !!(hwe2->product)) ||
+	    (hwe1->product && strcmp(hwe1->product, hwe2->product)))
  		return 1;

Quoted 7 lines

libmultipath/config.c line 40

Quoted 6 lines

libmultipath/config.c +2 -1

-	if (hwe1->revision && hwe2->revision && strcmp(hwe1->revision, hwe2->revision))
+	if ((!!(hwe1->revision) != !!(hwe2->revision)) ||
+	    (hwe1->revision && strcmp(hwe1->revision, hwe2->revision)))
  		return 1;
 
  	return 0;

I hate the '!!' constructs.

noted. If I respin this patch, I'll change it to something like

if ((hwe2->revision && !hwe1->revision) ||
    (hwe1->revision && (!hwe2->revision || strcmp(hwe1->revision, hwe2->revision))))

But there are many other instances of !! in the multipath tools code.

Quoted 17 lines

Quoted 6 lines
libmultipath/config.c +0 -0
@@ -416,6 +419,13 @@ factorize_hwtable (vector hw, int n)
  				continue;
  			/* dup */
  			merge_hwe(hwe2, hwe1);
libmultipath/config.c factorize_hwtable (vector hw, int n) · line 431
Quoted 10 lines
libmultipath/config.c +7 -0
+			if (hwe_strmatch(hwe2, hwe1) == 0) {
+				vector_del_slot(hw, i);
+				free_hwe(hwe1);
+				n -= 1;
+				i -= 1;
+				j -= 1;
+			}
  		}
  	}
  	return 0;
Is the 'hwe_strmatch' required here?

It should help clear up some user confusion. If the user adds a new config that only regex matches the built-in config, but doesn't string match it, you obviously need to keep the built-in config to correctly configure devices that match the built-in config but not the user config.

However if the user's config string matches the built-in, then the built-in config will never be used. Any device what would match the built-in config will match the user's merged config first. However, if the user dumps the config, and goes looking through it to make sure that everything got merged correctly, the first config they will see is the built-in config. Then they call support, who needs to explain to them that their config is further down in the output, and the last config in the output is what gets used.

It's easy to avoid all that by simply deleting the built-in config in the case where it string matches the user's config.

Quoted 3 lines

Also I'm not sure if just decrementing the indices is the correct way of doing things; I had a version which just restarted the outer loop if we encountered a duplicate.

Oops. You're correct. Looks like I am respinning the patch.

-Ben

Quoted 13 lines

Cheers,

Hannes

Signature · 9 lines

Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

--
dm-devel mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/dm-devel