I'd suggest you did a better job in the commit log explaning this than in the doxygen where it really is needed.
Private declares don't belong in util_filter.h IMO. On Thu, Sep 6, 2018, 17:48 <yla...@apache.org> wrote: > Author: ylavic > Date: Thu Sep 6 22:48:28 2018 > New Revision: 1840265 > > URL: http://svn.apache.org/viewvc?rev=1840265&view=rev > Log: > Follow up to r1840149: core input filter pending data. > > Since r1840149 ap_core_input_filter() can't use use f->[priv->]bb > directly, so > ap_filter_input_pending() stopped accounting for its pending data. > > But ap_core_input_filter() can't (and doesn't need to) setaside its socket > bucket, so ap_filter_setaside_brigade() is not an option. This commit adds > ap_filter_adopt_brigade() which simply moves the given buckets (brigade) > into > f->priv->bb, and since this is not something to be done blindly (the > buckets > need to have c->pool/bucket_alloc lifetime, which is the case in the core > filter) the function is not AP_DECLAREd/exported thus can be used in core > only. > > With ap_filter_adopt_brigade() and ap_filter_reinstate_brigade(), the core > input is now ap_filter_input_pending() friendly. > > Also, ap_filter_recycle() is no more part of the API (AP_DECLARE removed > too), > there really is no point to call it outside core code. MAJOR bumped once > again > because of this. > > Modified: > httpd/httpd/trunk/include/ap_mmn.h > httpd/httpd/trunk/include/util_filter.h > httpd/httpd/trunk/modules/http/http_request.c > httpd/httpd/trunk/server/core_filters.c > httpd/httpd/trunk/server/util_filter.c > > Modified: httpd/httpd/trunk/include/ap_mmn.h > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/include/ap_mmn.h?rev=1840265&r1=1840264&r2=1840265&view=diff > > ============================================================================== > --- httpd/httpd/trunk/include/ap_mmn.h (original) > +++ httpd/httpd/trunk/include/ap_mmn.h Thu Sep 6 22:48:28 2018 > @@ -606,12 +606,13 @@ > * ap_acquire_brigade()/ap_release_brigade(), and > * in ap_filter_t replace pending/bb/deferred_pool > * fields by struct ap_filter_private *priv > + * 20180906.1 (2.5.1-dev) Don't export ap_filter_recycle() anymore > */ > > #define MODULE_MAGIC_COOKIE 0x41503235UL /* "AP25" */ > > #ifndef MODULE_MAGIC_NUMBER_MAJOR > -#define MODULE_MAGIC_NUMBER_MAJOR 20180905 > +#define MODULE_MAGIC_NUMBER_MAJOR 20180906 > #endif > #define MODULE_MAGIC_NUMBER_MINOR 1 /* 0...n */ > > > Modified: httpd/httpd/trunk/include/util_filter.h > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/include/util_filter.h?rev=1840265&r1=1840264&r2=1840265&view=diff > > ============================================================================== > --- httpd/httpd/trunk/include/util_filter.h (original) > +++ httpd/httpd/trunk/include/util_filter.h Thu Sep 6 22:48:28 2018 > @@ -596,6 +596,16 @@ AP_DECLARE(int) ap_filter_prepare_brigad > AP_DECLARE(apr_status_t) ap_filter_setaside_brigade(ap_filter_t *f, > apr_bucket_brigade > *bb); > > +/* > + * Adopt a bucket brigade as is (no setaside nor copy). > + * @param f The current filter > + * @param bb The bucket brigade adopted. This brigade is always empty > + * on return > + * @remark httpd internal, not exported, needed by > + * ap_core_input_filter > + */ > +void ap_filter_adopt_brigade(ap_filter_t *f, apr_bucket_brigade *bb); > + > /** > * Reinstate a brigade setaside earlier, and calculate the amount of data > we > * should write based on the presence of flush buckets, size limits on in > @@ -656,14 +666,17 @@ AP_DECLARE_NONSTD(int) ap_filter_output_ > */ > AP_DECLARE_NONSTD(int) ap_filter_input_pending(conn_rec *c); > > -/** > +/* > * Recycle removed request filters so that they can be reused for filters > * added later on the same connection. This typically should happen after > * each request handling. > * > * @param c The connection. > + * @remark httpd internal, not exported, needed by > + * ap_process_request_after_handler > + * > */ > -AP_DECLARE(void) ap_filter_recycle(conn_rec *c); > +void ap_filter_recycle(conn_rec *c); > > /** > * Flush function for apr_brigade_* calls. This calls ap_pass_brigade > > Modified: httpd/httpd/trunk/modules/http/http_request.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_request.c?rev=1840265&r1=1840264&r2=1840265&view=diff > > ============================================================================== > --- httpd/httpd/trunk/modules/http/http_request.c (original) > +++ httpd/httpd/trunk/modules/http/http_request.c Thu Sep 6 22:48:28 2018 > @@ -402,10 +402,7 @@ AP_DECLARE(void) ap_process_request_afte > (void)ap_check_pipeline(c, bb, DEFAULT_LIMIT_BLANK_LINES); > > ap_release_brigade(c, bb); > - > - if (!c->aborted) { > - ap_filter_recycle(c); > - } > + ap_filter_recycle(c); > > if (c->cs) { > if (c->aborted) { > > Modified: httpd/httpd/trunk/server/core_filters.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/server/core_filters.c?rev=1840265&r1=1840264&r2=1840265&view=diff > > ============================================================================== > --- httpd/httpd/trunk/server/core_filters.c (original) > +++ httpd/httpd/trunk/server/core_filters.c Thu Sep 6 22:48:28 2018 > @@ -94,7 +94,7 @@ apr_status_t ap_core_input_filter(ap_fil > ap_input_mode_t mode, apr_read_type_e > block, > apr_off_t readbytes) > { > - apr_status_t rv; > + apr_status_t rv = APR_SUCCESS; > core_net_rec *net = f->ctx; > core_ctx_t *ctx = net->in_ctx; > const char *str; > @@ -124,8 +124,11 @@ apr_status_t ap_core_input_filter(ap_fil > if (rv != APR_SUCCESS) > return rv; > } > - else if (APR_BRIGADE_EMPTY(ctx->bb)) { > - return APR_EOF; > + else { > + ap_filter_reinstate_brigade(f, ctx->bb, NULL); > + if (APR_BRIGADE_EMPTY(ctx->bb)) { > + return APR_EOF; > + } > } > > /* ### This is bad. */ > @@ -151,7 +154,7 @@ apr_status_t ap_core_input_filter(ap_fil > if (APR_STATUS_IS_EAGAIN(rv) && block == APR_NONBLOCK_READ) { > rv = APR_SUCCESS; > } > - return rv; > + goto cleanup; > } > > /* ### AP_MODE_PEEK is a horrific name for this mode because we also > @@ -171,15 +174,16 @@ apr_status_t ap_core_input_filter(ap_fil > * mean that there is another request, just a blank line. > */ > while (1) { > - if (APR_BRIGADE_EMPTY(ctx->bb)) > - return APR_EOF; > + if (APR_BRIGADE_EMPTY(ctx->bb)) { > + rv = APR_EOF; > + goto cleanup; > + } > > e = APR_BRIGADE_FIRST(ctx->bb); > - > rv = apr_bucket_read(e, &str, &len, APR_NONBLOCK_READ); > - > - if (rv != APR_SUCCESS) > - return rv; > + if (rv != APR_SUCCESS) { > + goto cleanup; > + } > > c = str; > while (c < str + len) { > @@ -188,7 +192,7 @@ apr_status_t ap_core_input_filter(ap_fil > else if (*c == APR_ASCII_CR && *(c + 1) == APR_ASCII_LF) > c += 2; > else > - return APR_SUCCESS; > + goto cleanup; > } > > /* If we reach here, we were a bucket just full of CRLFs, so > @@ -196,7 +200,9 @@ apr_status_t ap_core_input_filter(ap_fil > /* FIXME: Is this the right thing to do in the core? */ > apr_bucket_delete(e); > } > - return APR_SUCCESS; > + > + /* UNREACHABLE */ > + ap_assert(0); > } > > /* If mode is EXHAUSTIVE, we want to just read everything until the > end > @@ -222,7 +228,9 @@ apr_status_t ap_core_input_filter(ap_fil > * must be EOS. */ > e = apr_bucket_eos_create(f->c->bucket_alloc); > APR_BRIGADE_INSERT_TAIL(b, e); > - return APR_SUCCESS; > + > + rv = APR_SUCCESS; > + goto cleanup; > } > > /* read up to the amount they specified. */ > @@ -233,14 +241,13 @@ apr_status_t ap_core_input_filter(ap_fil > > e = APR_BRIGADE_FIRST(ctx->bb); > rv = apr_bucket_read(e, &str, &len, block); > - > - if (APR_STATUS_IS_EAGAIN(rv) && block == APR_NONBLOCK_READ) { > - /* getting EAGAIN for a blocking read is an error; for a > - * non-blocking read, return an empty brigade. */ > - return APR_SUCCESS; > - } > - else if (rv != APR_SUCCESS) { > - return rv; > + if (rv != APR_SUCCESS) { > + if (APR_STATUS_IS_EAGAIN(rv) && block == APR_NONBLOCK_READ) { > + /* getting EAGAIN for a blocking read is an error; not > for a > + * non-blocking read, return an empty brigade. */ > + rv = APR_SUCCESS; > + } > + goto cleanup; > } > else if (block == APR_BLOCK_READ && len == 0) { > /* We wanted to read some bytes in blocking mode. We read > @@ -257,14 +264,13 @@ apr_status_t ap_core_input_filter(ap_fil > e = apr_bucket_eos_create(f->c->bucket_alloc); > APR_BRIGADE_INSERT_TAIL(b, e); > } > - return APR_SUCCESS; > + goto cleanup; > } > > /* Have we read as much data as we wanted (be greedy)? */ > if (len < readbytes) { > apr_size_t bucket_len; > > - rv = APR_SUCCESS; > /* We already registered the data in e in len */ > e = APR_BUCKET_NEXT(e); > while ((len < readbytes) && (rv == APR_SUCCESS) > @@ -298,7 +304,7 @@ apr_status_t ap_core_input_filter(ap_fil > > rv = apr_brigade_partition(ctx->bb, readbytes, &e); > if (rv != APR_SUCCESS) { > - return rv; > + goto cleanup; > } > > /* Must do move before CONCAT */ > @@ -316,7 +322,7 @@ apr_status_t ap_core_input_filter(ap_fil > { > rv = apr_bucket_copy(e, ©_bucket); > if (rv != APR_SUCCESS) { > - return rv; > + goto cleanup; > } > APR_BRIGADE_INSERT_TAIL(b, copy_bucket); > } > @@ -325,7 +331,10 @@ apr_status_t ap_core_input_filter(ap_fil > /* Take what was originally there and place it back on ctx->bb */ > APR_BRIGADE_CONCAT(ctx->bb, ctx->tmpbb); > } > - return APR_SUCCESS; > + > +cleanup: > + ap_filter_adopt_brigade(f, ctx->bb); > + return rv; > } > > static apr_status_t send_brigade_nonblocking(apr_socket_t *s, > > Modified: httpd/httpd/trunk/server/util_filter.c > URL: > http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_filter.c?rev=1840265&r1=1840264&r2=1840265&view=diff > > ============================================================================== > --- httpd/httpd/trunk/server/util_filter.c (original) > +++ httpd/httpd/trunk/server/util_filter.c Thu Sep 6 22:48:28 2018 > @@ -321,7 +321,8 @@ static struct ap_filter_conn_ctx *get_co > return x; > } > > -static void make_spare_ring(struct spare_ring **ring, apr_pool_t *p) > +static APR_INLINE > +void make_spare_ring(struct spare_ring **ring, apr_pool_t *p) > { > if (!*ring) { > *ring = apr_palloc(p, sizeof(**ring)); > @@ -403,7 +404,7 @@ static apr_status_t request_filter_clean > return APR_SUCCESS; > } > > -AP_DECLARE(void) ap_filter_recycle(conn_rec *c) > +void ap_filter_recycle(conn_rec *c) > { > struct ap_filter_conn_ctx *x = c->filter_conn_ctx; > > @@ -983,6 +984,22 @@ AP_DECLARE(apr_status_t) ap_filter_setas > return APR_SUCCESS; > } > > +void ap_filter_adopt_brigade(ap_filter_t *f, apr_bucket_brigade *bb) > +{ > + struct ap_filter_private *fp = f->priv; > + > + ap_log_cerror(APLOG_MARK, APLOG_TRACE6, 0, f->c, > + "adopt %s brigade to %s brigade in '%s' output filter", > + APR_BRIGADE_EMPTY(bb) ? "empty" : "full", > + (!fp->bb || APR_BRIGADE_EMPTY(fp->bb)) ? "empty" : > "full", > + f->frec->name); > + > + if (!APR_BRIGADE_EMPTY(bb)) { > + ap_filter_prepare_brigade(f); > + APR_BRIGADE_CONCAT(fp->bb, bb); > + } > +} > + > AP_DECLARE(apr_status_t) ap_filter_reinstate_brigade(ap_filter_t *f, > apr_bucket_brigade > *bb, > apr_bucket > **flush_upto) > @@ -1308,6 +1325,7 @@ AP_DECLARE_NONSTD(apr_status_t) ap_fprin > va_end(args); > return rv; > } > + > AP_DECLARE(void) ap_filter_protocol(ap_filter_t *f, unsigned int flags) > { > f->frec->proto_flags = flags ; > > >
