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 <mwi...@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
--
Dr. Martin Wilck <mwi...@suse.com>, Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
