[dm-devel] [PATCH 08/35] libmultipath: create bitfield abstraction

[Benjamin Marzinski]

On Tue, Aug 04, 2020 at 09:35:08PM +0200, Martin Wilck wrote:
> On Tue, 2020-08-04 at 11:26 -0500, Benjamin Marzinski wrote:
> > On Tue, Aug 04, 2020 at 05:18:18PM +0200, Martin Wilck wrote:
> > > 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 at suse.com
> > > > > wrote:
> > > > > > From: Martin Wilck <mwilck at suse.com>
> > > > > > +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.
> > 
> > But the calloc call itself can return NULL, so deferencing bf (as in
> > bf->len = maxbit), can crash.
> 
> Of course, I wasn't arguing about that. I'm going to resend with this
> fixed.
> 
> > I'm also still fuzzy on why we want to support zero length bitfields.
> > Since they can't be grown like vectors can, it seem like requesting a
> > zero length bitfield will always be a sign of a coding error. We
> > would get a more useful error by having the failure happen closer to
> > the error in the code.  Or is there actually a use for a zero length
> > bitfield that can't be grown?
> 
> The only "use" would be to be able to work with bitmaps in situations
> where zero elements are possible, without the need to catch another
> exception. 0-element bitfields are technically possible although
> logically they make no sense. That's robust design for a library
> function, IMO. I don't see why it'd be "better" to return NULL instead.
> If we are after errors in callers, we might want to print an error
> message and still not return NULL.

Fine. It's a pretty trivial thing either way.

-Ben

> 
> Martin
>