| Message ID | 20130110201022.GR19059@ether.msp.redhat.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
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
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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
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;
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(-) -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel