Re: [PATCH 08/35] libmultipath: create bitfield abstraction

[Martin Wilck <mwilck@suse.com>]

On Tue, 2020-08-04 at 17:04 +0200, Martin Wilck wrote:
> On Thu, 2020-07-16 at 16:17 -0500, Benjamin Marzinski wrote:
> > On Thu, Jul 09, 2020 at 12:15:53PM +0200, mwilck@suse.com wrote:
> > > From: Martin Wilck <mwilck@suse.com>
> > > 
> > > In e32d521d ("libmultipath: coalesce_paths: fix size mismatch
> > > handling"),
> > > we introduced simple bitmap handling functions. We can do better.
> > > This
> > > patch introduces a bitfield type with overflow detection and a
> > > find_first_set() method.
> > > 
> > > Use this in coalesce_paths(), and adapt the unit tests. Also, add
> > > unit tests for "odd" bitfield sizes; so far we tested only
> > > multiples
> > > of 64.
> > > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  libmultipath/configure.c |   9 +-
> > >  libmultipath/util.c      |  35 ++++++
> > >  libmultipath/util.h      |  57 ++++++++-
> > >  tests/util.c             | 263
> > > +++++++++++++++++++++++++++++++++++----
> > >  4 files changed, 327 insertions(+), 37 deletions(-)
> > > 
> > > ...
> > > diff --git a/libmultipath/util.c b/libmultipath/util.c
> > > index 3c43f28..46cacd4 100644
> > > --- a/libmultipath/util.c
> > > +++ b/libmultipath/util.c
> > > @@ -404,3 +404,38 @@ void close_fd(void *arg)
> > >  {
> > >  	close((long)arg);
> > >  }
> > > +
> > > +struct bitfield *alloc_bitfield(unsigned int maxbit)
> > > +{
> > > +	unsigned int n;
> > > +	struct bitfield *bf;
> > > +
> > > +	n = maxbit > 0 ? (maxbit - 1) / bits_per_slot + 1 : 0;
> > 
> > What's the point in accepting 0? That's an empty bitmap.
> > 
> Thanks for spotting these, I will fix them.

Thinking about it once more, I believe that accepting 0 as the bitfield
length is actually the right thing. A bitfield of length 0 makes not
much less sense than one of length 1. The code makes sure that the bit
operations on the 0-length bitfield behave correctly (see
test_bitmask_len_0()). Thus callers can use bitfields without bothering
for extra NULL checks. That was the intention. Like we support 0-length 
vectors.

Regards,
Martin