| Message ID | 20180608102041.22904-16-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
On Fri, Jun 08, 2018 at 12:20:28PM +0200, Martin Wilck wrote: > Merging hwentries with identical vendor/product/revision is still useful, > although it's not strictly necessary any more. But such identical entries > should always be merged, not only if they appear in different configuration > files. This requires changing the logic of factorize_hwtable. Sorry for my slowness in reviewing these. This one looks wrong to me > By setting -DCHECK_BUILTIN_HWTABLE at compile time, the built-in hwtable > can be checked against duplicates as well. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > --- > libmultipath/config.c | 43 +++++++++++++++++++++---------------------- > 1 file changed, 21 insertions(+), 22 deletions(-) > > diff --git a/libmultipath/config.c b/libmultipath/config.c > index 713ac7f3..fb41d620 100644 > --- a/libmultipath/config.c > +++ b/libmultipath/config.c > @@ -452,7 +452,7 @@ out: > } > > static void > -factorize_hwtable (vector hw, int n) > +factorize_hwtable (vector hw, int n, const char *table_desc) > { > struct hwentry *hwe1, *hwe2; > int i, j; > @@ -462,22 +462,26 @@ restart: > if (i == n) > break; > j = n; > + /* drop invalid device configs */ > + if (i >= n && (!hwe1->vendor || !hwe1->product)) { How can i >= n? vector_foreach_slot() starts i at 0, and then next lines are if (i == n) break; > + condlog(0, "device config in %s missing vendor or product parameter", > + table_desc); > + vector_del_slot(hw, i--); shouldn't we also decrement n here. I realize that doesn't work well for the CHECK_BUILTIN_HWTABLE check, where n is 0, but as I said above, I'm pretty sure that will never get here. > + free_hwe(hwe1); > + continue; > + } > + j = n > i + 1 ? n : i + 1; again, n must always be at least i + 1 to get here, so j is always n. > vector_foreach_slot_after(hw, hwe2, j) { > - /* drop invalid device configs */ > - if (!hwe2->vendor || !hwe2->product) { > - condlog(0, "device config missing vendor or product parameter"); > - vector_del_slot(hw, j--); > - free_hwe(hwe2); > - continue; > - } > if (hwe_strmatch(hwe2, hwe1) == 0) { > - condlog(4, "%s: removing hwentry %s:%s:%s", > + condlog(i >= n ? 1 : 3, this check also looks wrong > + "%s: duplicate device section for %s:%s:%s in %s", > __func__, hwe1->vendor, hwe1->product, > - hwe1->revision); > + hwe1->revision, table_desc); > vector_del_slot(hw, i); > merge_hwe(hwe2, hwe1); > free_hwe(hwe1); > - n -= 1; > + if (i < n) and this one looks unnecessary. > + n -= 1; > /* > * Play safe here; we have modified > * the original vector so the outer > @@ -605,9 +609,8 @@ process_config_dir(struct config *conf, vector keywords, char *dir) > snprintf(path, LINE_MAX, "%s/%s", dir, namelist[i]->d_name); > path[LINE_MAX-1] = '\0'; > process_file(conf, path); > - if (VECTOR_SIZE(conf->hwtable) > old_hwtable_size) > - factorize_hwtable(conf->hwtable, old_hwtable_size); > - > + factorize_hwtable(conf->hwtable, old_hwtable_size, > + namelist[i]->d_name); > } > pthread_cleanup_pop(1); > } > @@ -655,6 +658,9 @@ load_config (char * file) > if (setup_default_hwtable(conf->hwtable)) > goto out; > > +#ifdef CHECK_BUILTIN_HWTABLE > + factorize_hwtable(conf->hwtable, 0, "builtin"); > +#endif > /* > * read the config file > */ > @@ -668,14 +674,7 @@ load_config (char * file) > condlog(0, "error parsing config file"); > goto out; > } > - if (VECTOR_SIZE(conf->hwtable) > builtin_hwtable_size) { > - /* > - * remove duplica in hwtable. config file > - * takes precedence over build-in hwtable > - */ > - factorize_hwtable(conf->hwtable, builtin_hwtable_size); > - } > - > + factorize_hwtable(conf->hwtable, builtin_hwtable_size, file); > } > > conf->processed_main_config = 1; > -- > 2.17.0 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Hi Ben, On Fri, 2018-06-15 at 13:03 -0500, Benjamin Marzinski wrote: > On Fri, Jun 08, 2018 at 12:20:28PM +0200, Martin Wilck wrote: > > Merging hwentries with identical vendor/product/revision is still > > useful, > > although it's not strictly necessary any more. But such identical > > entries > > should always be merged, not only if they appear in different > > configuration > > files. This requires changing the logic of factorize_hwtable. > > Sorry for my slowness in reviewing these. This one looks wrong to me Thanks for the thorough review. Given the size of the series, I don't think this was slow. > > By setting -DCHECK_BUILTIN_HWTABLE at compile time, the built-in > > hwtable > > can be checked against duplicates as well. > > > > Signed-off-by: Martin Wilck <mwilck@suse.com> > > --- > > libmultipath/config.c | 43 +++++++++++++++++++++---------------- > > ------ > > 1 file changed, 21 insertions(+), 22 deletions(-) > > > > diff --git a/libmultipath/config.c b/libmultipath/config.c > > index 713ac7f3..fb41d620 100644 > > --- a/libmultipath/config.c > > +++ b/libmultipath/config.c > > @@ -452,7 +452,7 @@ out: > > } > > > > static void > > -factorize_hwtable (vector hw, int n) > > +factorize_hwtable (vector hw, int n, const char *table_desc) > > { > > struct hwentry *hwe1, *hwe2; > > int i, j; > > @@ -462,22 +462,26 @@ restart: > > if (i == n) > > break; > > j = n; > > + /* drop invalid device configs */ > > + if (i >= n && (!hwe1->vendor || !hwe1->product)) { > > How can i >= n? vector_foreach_slot() starts i at 0, and then next > lines are > > if (i == n) > break; The above two lines, and the line "j = n" after that, were meant to be deleted. That slipped through in my rebasing work, sorry about that. When these lines are gone, I believe your issues are resolved, see below. n denotes the number of hwentries present before the current config file was parsed. Before my patch series, "factorize_hwtable" would only check for duplicates between the "old" (i < n) and "new" (i >= n) sections, and would not remove the dups inside either section. Now duplicates are also merged if they occur in the same section (except for the built-in hwtable, for which the "inside" check is only done with -DCHECK_BUILTIN_HWTABLE). Well - that's what I intended to do, but by by forgetting to delete the above lines, I failed :-( In practice, this error only breaks the "broken hwentry" and "dup removal inside a section" features - application of "good" hwentries to paths and multipaths isn't affected, and thus the test cases pass despite the bug. I'll post a fix, plus new test cases covering it. > > + condlog(0, "device config in %s missing > > vendor or product parameter", > > + table_desc); > > + vector_del_slot(hw, i--); > > shouldn't we also decrement n here. I realize that doesn't work well > for > the CHECK_BUILTIN_HWTABLE check, where n is 0, but as I said above, > I'm > pretty sure that will never get here. No, see above. The check for broken entries is only done for "new" ones (i > n), as we assume the entries up to n to be checked already. So when we reach this condition, is is really always above n. > > > + free_hwe(hwe1); > > + continue; > > + } > > + j = n > i + 1 ? n : i + 1; > > again, n must always be at least i + 1 to get here, so j is always n. See above. > > > vector_foreach_slot_after(hw, hwe2, j) { > > - /* drop invalid device configs */ > > - if (!hwe2->vendor || !hwe2->product) { > > - condlog(0, "device config missing > > vendor or product parameter"); > > - vector_del_slot(hw, j--); > > - free_hwe(hwe2); > > - continue; > > - } > > if (hwe_strmatch(hwe2, hwe1) == 0) { > > - condlog(4, "%s: removing hwentry > > %s:%s:%s", > > + condlog(i >= n ? 1 : 3, > > this check also looks wrong The intention is not to log at prio 1 if a new section (read in most cases: multipath.conf) contains a duplicate to a previous section (read in most cases: built-in hwtable). Such use is perfectly valid, whereas two hwentries with the same vendor and product in one config file are suspicious and should be logged. > > > + "%s: duplicate device > > section for %s:%s:%s in %s", > > __func__, hwe1->vendor, > > hwe1->product, > > - hwe1->revision); > > + hwe1->revision, > > table_desc); > > vector_del_slot(hw, i); > > merge_hwe(hwe2, hwe1); > > free_hwe(hwe1); > > - n -= 1; > > + if (i < n) > > and this one looks unnecessary. See above. I order to avoid spamming dm-devel with the whole series again, I'll send patches on top of it. If you prefer that I send the full rebased series, I'll do of course, but I'll wait for more reviews. Thanks, Martin
diff --git a/libmultipath/config.c b/libmultipath/config.c index 713ac7f3..fb41d620 100644 --- a/libmultipath/config.c +++ b/libmultipath/config.c @@ -452,7 +452,7 @@ out: } static void -factorize_hwtable (vector hw, int n) +factorize_hwtable (vector hw, int n, const char *table_desc) { struct hwentry *hwe1, *hwe2; int i, j; @@ -462,22 +462,26 @@ restart: if (i == n) break; j = n; + /* drop invalid device configs */ + if (i >= n && (!hwe1->vendor || !hwe1->product)) { + condlog(0, "device config in %s missing vendor or product parameter", + table_desc); + vector_del_slot(hw, i--); + free_hwe(hwe1); + continue; + } + j = n > i + 1 ? n : i + 1; vector_foreach_slot_after(hw, hwe2, j) { - /* drop invalid device configs */ - if (!hwe2->vendor || !hwe2->product) { - condlog(0, "device config missing vendor or product parameter"); - vector_del_slot(hw, j--); - free_hwe(hwe2); - continue; - } if (hwe_strmatch(hwe2, hwe1) == 0) { - condlog(4, "%s: removing hwentry %s:%s:%s", + condlog(i >= n ? 1 : 3, + "%s: duplicate device section for %s:%s:%s in %s", __func__, hwe1->vendor, hwe1->product, - hwe1->revision); + hwe1->revision, table_desc); vector_del_slot(hw, i); merge_hwe(hwe2, hwe1); free_hwe(hwe1); - n -= 1; + if (i < n) + n -= 1; /* * Play safe here; we have modified * the original vector so the outer @@ -605,9 +609,8 @@ process_config_dir(struct config *conf, vector keywords, char *dir) snprintf(path, LINE_MAX, "%s/%s", dir, namelist[i]->d_name); path[LINE_MAX-1] = '\0'; process_file(conf, path); - if (VECTOR_SIZE(conf->hwtable) > old_hwtable_size) - factorize_hwtable(conf->hwtable, old_hwtable_size); - + factorize_hwtable(conf->hwtable, old_hwtable_size, + namelist[i]->d_name); } pthread_cleanup_pop(1); } @@ -655,6 +658,9 @@ load_config (char * file) if (setup_default_hwtable(conf->hwtable)) goto out; +#ifdef CHECK_BUILTIN_HWTABLE + factorize_hwtable(conf->hwtable, 0, "builtin"); +#endif /* * read the config file */ @@ -668,14 +674,7 @@ load_config (char * file) condlog(0, "error parsing config file"); goto out; } - if (VECTOR_SIZE(conf->hwtable) > builtin_hwtable_size) { - /* - * remove duplica in hwtable. config file - * takes precedence over build-in hwtable - */ - factorize_hwtable(conf->hwtable, builtin_hwtable_size); - } - + factorize_hwtable(conf->hwtable, builtin_hwtable_size, file); } conf->processed_main_config = 1;
Merging hwentries with identical vendor/product/revision is still useful, although it's not strictly necessary any more. But such identical entries should always be merged, not only if they appear in different configuration files. This requires changing the logic of factorize_hwtable. By setting -DCHECK_BUILTIN_HWTABLE at compile time, the built-in hwtable can be checked against duplicates as well. Signed-off-by: Martin Wilck <mwilck@suse.com> --- libmultipath/config.c | 43 +++++++++++++++++++++---------------------- 1 file changed, 21 insertions(+), 22 deletions(-)