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 <[email protected]> > --- > 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 [email protected] https://www.redhat.com/mailman/listinfo/dm-devel
