You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Glenn Strauss <gs...@gluelogic.com> on 2004/08/12 23:03:10 UTC
BRIGADE_NORMALIZE is bad coding example
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
Re: BRIGADE_NORMALIZE is bad coding example
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Thursday, August 12, 2004 6:24 PM -0400 Glenn Strauss
<gs...@gluelogic.com> wrote:
> Right. I didn't say it was a problem in practice.
> I did say that it was a terrible piece of code, and since this list
> often refers people to "look at the code", it should be fixed, IMHO.
> It is a _bad_ and _broken_ example of how to loop through a brigade.
Yah, but we don't publicize it: which is why I think your subject clearly
overreaches. We even have a comment that says this is bad. (But, we're also
not perfect and it should be corrected.) I'm guessing we never scrutinized
NORMALIZE_BRIGADE when we tossed APR_BRIGADE_FOREACH. *shrug*
So, I don't think most people would latch on to that code as our canonical
example of iterating through a brigade.
> As for C99 extensions, I understand that it is not available on all
> platforms, but why can't new code checked in include the 'restrict'
> keyword? Just like there is an APR_INLINE macro, why isn't there
> an APR_RESTRICT macro indirection? Would a patch implementing such
> in APR be accepted for APR 1.0?
APR 1.0 is already frozen for its 1.0 release. While APR_INLINE has a
legitimate purpose for us, I just don't see the same for APR_RESTRICT. It
seems to be another case of the C99 folks adding needless bloat. -- justin
Re: BRIGADE_NORMALIZE is bad coding example
Posted by Glenn Strauss <gs...@gluelogic.com>.
On Thu, Aug 12, 2004 at 02:27:03PM -0700, Justin Erenkrantz wrote:
> --On Thursday, August 12, 2004 5:03 PM -0400 Glenn Strauss
> <gs...@gluelogic.com> wrote:
>
> > 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
>
> IMHO, the cleanest correct fix is to just add an else clause. At the cost
> of more variables, you could always use d and forgo the else clause, but I
> think that'll just confuse readers of the macro even more. We also can't
> accept C99 extensions. *shrug*
>
> We probably never hit this odd corner case in practice though because
> core_input_filter only has one of the following: one empty bucket, one
> socket bucket, one heap bucket, or a heap and a socket bucket: two empty
> buckets won't happen due to the design. FWIW, BRIGADE_NORMALIZE was thrown
> out of apr-util because it was really bad to begin with: hence the comment
> ("This is bad.") and the localization of it to only server/core.c. It was
> never meant to be an example - yet, this is a minor bug more than anything
> serious. -- justin
Right. I didn't say it was a problem in practice.
I did say that it was a terrible piece of code, and since this list
often refers people to "look at the code", it should be fixed, IMHO.
It is a _bad_ and _broken_ example of how to loop through a brigade.
As for C99 extensions, I understand that it is not available on all
platforms, but why can't new code checked in include the 'restrict'
keyword? Just like there is an APR_INLINE macro, why isn't there
an APR_RESTRICT macro indirection? Would a patch implementing such
in APR be accepted for APR 1.0?
Cheers,
Glenn
Re: BRIGADE_NORMALIZE is bad coding example
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Thursday, August 12, 2004 5:03 PM -0400 Glenn Strauss
<gs...@gluelogic.com> wrote:
> 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
IMHO, the cleanest correct fix is to just add an else clause. At the cost of
more variables, you could always use d and forgo the else clause, but I think
that'll just confuse readers of the macro even more. We also can't accept C99
extensions. *shrug*
We probably never hit this odd corner case in practice though because
core_input_filter only has one of the following: one empty bucket, one socket
bucket, one heap bucket, or a heap and a socket bucket: two empty buckets
won't happen due to the design. FWIW, BRIGADE_NORMALIZE was thrown out of
apr-util because it was really bad to begin with: hence the comment ("This is
bad.") and the localization of it to only server/core.c. It was never meant
to be an example - yet, this is a minor bug more than anything serious. --
justin