httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Glenn Strauss <gs-apache-...@gluelogic.com>
Subject BRIGADE_NORMALIZE is bad coding example
Date Thu, 12 Aug 2004 21:03:10 GMT
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

Mime
View raw message