sys/sys/bitstring.h
Fix a rounding calculation that could undersize a bitstring on
32-bit platforms.
include/bitstring.h
Add sys/param.h for the roundup2 macro
Details
Details
- Reviewers
gibbs - Commits
- rS302180: Fix bitstring allocation on 32-bit platforms
Existing ATF test cases
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 4336 Build 4381: arc lint + arc unit
Event Timeline
Comment Actions
I see how this can over allocate on platforms with long > 8bits, but I'm missing how 32bit long platforms are special and can get too little space allocated. Can you provide an example in the checkin comment?
Comment Actions
Restore bitstr_size to its originally intended purpose of calculating the
actual number of bytes used by a bitstring, and fix bit_decl to round up
appropriately.
Comment Actions
If I want to dynamically allocate a bitstr, I use bitstr_size() to tell me how much to allocate. Without the roundup, I'll potentially allocate too little memory.
| sys/sys/bitstring.h | ||
|---|---|---|
| 114–115 | #define bitstr_size(_nbits) (roundup2(_nbits, _BITSTR_BITS) / 8) | |
| 134 | No roundup is required here. Is it clearer to use howmany()? I'm not sure. | |
Comment Actions
Should we inline roundup2? Prior to this change, at least in user space, bitstring.h only relied on functionality provided in the C standard. It might be nice to keep that.
Comment Actions
I would say that you should use bit_alloc. But plenty of callers are doing it themselves. I'll do as you suggest, which will basically revert the 2nd diff of this review.
| sys/sys/bitstring.h | ||
|---|---|---|
| 134 | Roundup most certainly is required here, unless the roundup is implemented in bitstr_size instead. howmany() is basically just division rounding up. | |
| sys/sys/bitstring.h | ||
|---|---|---|
| 134 | That's what I meant. If bitstr_size() has to roundup anyway, there's no need to roundup here too. | |
Comment Actions
Are you happy with this @gibbs? Due to my vacation, I must commit today or it won't get into 11.0.
Comment Actions
I'm just not sure about the new dependency on sys/param.h. I leave that for you to decide.
| sys/sys/bitstring.h | ||
|---|---|---|
| 114–116 | We don't want to prevent a future declaration of roundup2 in the user's program either. So something like this? #ifdef roundup2 #define _bit_roundup2 roundup2 #else #define _bit_roundup2(x, y) (((x)+((y)-1))&(~((y)-1))) /* if y is powers of two */ #endif Move to private section and update call sites to use the private macro name. | |
Comment Actions
Please fix the extra whitespace and test both with and without a roundup2 defined prior to including the header before landing.
| sys/sys/bitstring.h | ||
|---|---|---|
| 78 | Extra newline. | |