Re: [PATCH 15/28] libmultipath: merge hwentries inside a conf file

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxx>
> > ---
> >  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 <mwilck@xxxxxxxx>, 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@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux