* [PATCH v2] multipath: remove duplicates from multipath configuration
@ 2013-01-10 20:10 Benjamin Marzinski
2013-01-11 7:15 ` Hannes Reinecke
0 siblings, 1 reply; 3+ messages in thread
From: Benjamin Marzinski @ 2013-01-10 20:10 UTC (permalink / raw)
To: device-mapper development; +Cc: Christophe Varoqui
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 <bmarzins@redhat.com>
---
libmultipath/blacklist.c | 91 ++++++++++++++++++++++++-----------------------
libmultipath/config.c | 16 ++++++--
2 files changed, 60 insertions(+), 47 deletions(-)
Index: multipath-tools-120821/libmultipath/blacklist.c
===================================================================
--- multipath-tools-120821.orig/libmultipath/blacklist.c
+++ 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) \
Index: multipath-tools-120821/libmultipath/config.c
===================================================================
--- multipath-tools-120821.orig/libmultipath/config.c
+++ 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;
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] multipath: remove duplicates from multipath configuration 2013-01-10 20:10 [PATCH v2] multipath: remove duplicates from multipath configuration Benjamin Marzinski @ 2013-01-11 7:15 ` Hannes Reinecke 2013-01-11 17:52 ` Benjamin Marzinski 0 siblings, 1 reply; 3+ messages in thread From: Hannes Reinecke @ 2013-01-11 7:15 UTC (permalink / raw) To: dm-devel On 01/10/2013 09:10 PM, Benjamin Marzinski wrote: > 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 <bmarzins@redhat.com> > --- > libmultipath/blacklist.c | 91 ++++++++++++++++++++++++----------------------- > libmultipath/config.c | 16 ++++++-- > 2 files changed, 60 insertions(+), 47 deletions(-) > > Index: multipath-tools-120821/libmultipath/blacklist.c > =================================================================== > --- multipath-tools-120821.orig/libmultipath/blacklist.c > +++ 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) \ > Index: multipath-tools-120821/libmultipath/config.c > =================================================================== > --- multipath-tools-120821.orig/libmultipath/config.c > +++ 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; I hate the '!!' constructs. > @@ -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; > 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 -- Dr. Hannes Reinecke zSeries & Storage hare@suse.de +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) ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] multipath: remove duplicates from multipath configuration 2013-01-11 7:15 ` Hannes Reinecke @ 2013-01-11 17:52 ` Benjamin Marzinski 0 siblings, 0 replies; 3+ messages in thread From: Benjamin Marzinski @ 2013-01-11 17:52 UTC (permalink / raw) To: device-mapper development On Fri, Jan 11, 2013 at 08:15:35AM +0100, Hannes Reinecke wrote: > On 01/10/2013 09:10 PM, Benjamin Marzinski wrote: >> 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 <bmarzins@redhat.com> >> --- >> libmultipath/blacklist.c | 91 ++++++++++++++++++++++++----------------------- >> libmultipath/config.c | 16 ++++++-- >> 2 files changed, 60 insertions(+), 47 deletions(-) >> >> Index: multipath-tools-120821/libmultipath/blacklist.c >> =================================================================== >> --- multipath-tools-120821.orig/libmultipath/blacklist.c >> +++ 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) \ >> Index: multipath-tools-120821/libmultipath/config.c >> =================================================================== >> --- multipath-tools-120821.orig/libmultipath/config.c >> +++ 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; > 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. > >> @@ -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; >> > 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. > 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 > Cheers, > > Hannes > -- > Dr. Hannes Reinecke zSeries & Storage > hare@suse.de +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 > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-01-11 17:52 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-10 20:10 [PATCH v2] multipath: remove duplicates from multipath configuration Benjamin Marzinski 2013-01-11 7:15 ` Hannes Reinecke 2013-01-11 17:52 ` Benjamin Marzinski
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.