The BRIGADE_NORMALIZE macro in server/core.c is a
terrible coding example of brigade use.
It took me a little while to wrap my head around brigades
because I had assumed this code to be correct. It is not.
BRIGADE_NORMALIZE checks e before checking e != sentinel on first time
through the loop. In core_input_filter(), the brigade is known not to
be empty when this macro is called, but the macro has no such knowledge.
This macro would be better integrated into core_input_filter() and not
abstracted as a macro with undocumented assumptions.
BRIGADE_NORMALIZE skips the bucket after a 0-length bucket.
In doing so, it might skip a 0-length bucket following it. If the last
bucket is 0-length, it will skip the sentinel and loop through the
brigade twice, thereby catching the 0-length buckets that were skipped
the first time through, unless there had been 4 zero-length buckets in
a row. Repeat loop again if last bucket is a zero-length bucket ...
(Quick fix is to change line with 'd' to: d = APR_BUCKET_PREV(e); )
With better coding, it is redundant to check
(!APR_BRIGADE_EMPTY(b) && e != APR_BRIGADE_SENTINEL(b))
If BRIGADE_NORMALIZE was not skipping buckets improperly, simply checking
e != APR_BRIGADE_SENTINEL(b)
would be sufficient.
#if 0
/*** BROKEN ***/ /* this is from server/core.c and is BROKEN */
/**
* Remove all zero length buckets from the brigade.
*/
#define BRIGADE_NORMALIZE(b) \
do { \
apr_bucket *e = APR_BRIGADE_FIRST(b); \
do { \
if (e->length == 0 && !APR_BUCKET_IS_METADATA(e)) { \
apr_bucket *d; \
d = APR_BUCKET_NEXT(e); \
apr_bucket_delete(e); \
e = d; \
} \
e = APR_BUCKET_NEXT(e); \
} while (!APR_BRIGADE_EMPTY(b) && (e != APR_BRIGADE_SENTINEL(b))); \
} while (0)
#endif
/*** CORRECT ***/
/**
* Remove all zero-length buckets from a brigade.
* (IMPL: In tight loops, there is some benefit to storing sentinel in constant)
* (IMPL: 'restrict' keyword is part of C99 standard
* Compile w/ "gcc -std=c99" (or "gcc -Drestrict= -Wall ..." to disable))
*/
#define BRIGADE_NORMALIZE(bb) \
do { \
register apr_bucket * restrict ap__b; \
apr_bucket *ap__btmp = APR_BRIGADE_FIRST(bb); \
apr_bucket * const apr__sentinel = APR_BRIGADE_SENTINEL(bb); \
while ((ap__b = ap__btmp) != apr__sentinel) { \
ap__btmp = APR_BUCKET_NEXT(ap__b); \
if (ap__b->length == 0 && !APR_BUCKET_IS_METADATA(ap__b)) { \
apr_bucket_delete(ap__b); \
} \
} \
} while (0)
Cheers,
Glenn
