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

[Martin Wilck]

Hi Ben,

On Mon, 2020-08-10 at 13:05 -0500, Benjamin Marzinski wrote:
> 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:
> > > 
> > > 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.

It took me a while, but ...

The current typical usage e.g. in group_by_match() looks like this:

	bitmap = alloc_bitfield(VECTOR_SIZE(paths));

	if (!bitmap)
		goto out;

	for (i = 0; i < VECTOR_SIZE(paths); i++) {
	        ....

Here, VECTOR_SIZE(paths) == 0 is a valid value that just causes nothing
to happen in the function. The other callers aren't much different. I
think we rather don't want to print an error message in these cases.

Every caller needs to check the return value of alloc_bitfield()
anyway, to guard against OOM. The code above would actually be slightly
optimized by returning NULL from alloc_bitfield() for maxbit == 0, by
returning immediately. Besides, we eliminate an (albeit minimal) risk
of hitting OOM by avoiding one needless memory allocation. 

The fact that all our callers work like this convinces me. I'll change
the return value to NULL like you suggested.

Cheers,
Martin