httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Plüm, Rüdiger, Vodafone Group <ruediger.pl...@vodafone.com>
Subject RE: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/ modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/ server/mpm/simple/
Date Tue, 06 Oct 2015 07:37:56 GMT


> -----Original Message-----
> From: Yann Ylavic [mailto:ylavic.dev@gmail.com]
> Sent: Dienstag, 6. Oktober 2015 01:40
> To: dev@httpd.apache.org
> Subject: Re: svn commit: r1706669 - in /httpd/httpd/trunk: ./ include/
> modules/http/ modules/ssl/ server/ server/mpm/event/ server/mpm/motorz/
> server/mpm/simple/
> 
> > Modified: httpd/httpd/trunk/server/mpm/event/event.c
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?re
> v=1706669&r1=1706668&r2=1706669&view=diff
> >
> ==========================================================================
> ====
> > --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> > +++ httpd/httpd/trunk/server/mpm/event/event.c Sun Oct  4 10:10:51 2015
> > @@ -1146,19 +1146,38 @@ read_request:
> >      }
> >
> []
> > +
> > +        rindex = apr_hash_first(NULL, c->filters);
> > +        while (rindex) {
> > +            ap_filter_t *f = apr_hash_this_val(rindex);
> > +
> > +            if (!APR_BRIGADE_EMPTY(f->bb)) {
> > +
> > +                rv = ap_pass_brigade(f, c->empty);
> > +                apr_brigade_cleanup(c->empty);
> > +                if (APR_SUCCESS != rv) {
> > +                    ap_log_cerror(
> > +                            APLOG_MARK, APLOG_DEBUG, rv, c,
> APLOGNO(00470)
> > +                            "write failure in '%s' output filter", f-
> >frec->name);
> > +                    break;
> > +                }
> > +
> > +                if (ap_filter_should_yield(f)) {
> > +                    data_in_output_filters = 1;
> > +                }
> > +            }
> > +
> > +            rindex = apr_hash_next(rindex);
> >          }
> 
> This pattern looks shared by all (relevant) MPMs, couldn't we make it
> a function? Maybe ap_reinstate_conn()?

Sounds sensible.

> 
> Also it seems that we leak the hash iterator here (on c->pool).

Why do we leak here? apr_hash_first(NULL, is used. So no new iterator is allocated.

> Couldn't we use a (single) linked list for c->filters since
> ap_filter_t already has a 'next' field?
> 
> >
> > Modified: httpd/httpd/trunk/server/util_filter.c
> > URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util_filter.c?rev=17
> 06669&r1=1706668&r2=1706669&view=diff
> >
> ==========================================================================
> ====
> > --- httpd/httpd/trunk/server/util_filter.c (original)
> > +++ httpd/httpd/trunk/server/util_filter.c Sun Oct  4 10:10:51 2015
> []
> > @@ -635,7 +653,8 @@ AP_DECLARE(apr_status_t) ap_save_brigade
> >      apr_status_t rv, srv = APR_SUCCESS;
> >
> >      /* If have never stored any data in the filter, then we had better
> > -     * create an empty bucket brigade so that we can concat.
> > +     * create an empty bucket brigade so that we can concat. Register
> > +     * a cleanup to zero out the pointer if the pool is cleared.
> 
> Maybe this comment belongs in ap_filter_setaside_brigade()?
> 
> >       */
> >      if (!(*saveto)) {
> >          *saveto = apr_brigade_create(p, f->c->bucket_alloc);
> > @@ -673,6 +692,248 @@ AP_DECLARE(apr_status_t) ap_save_brigade
> >      return srv;
> >  }
> >
> > +static apr_status_t filters_cleanup(void *data)
> > +{
> > +    ap_filter_t **key = data;
> > +
> > +    apr_hash_set((*key)->c->filters, key, sizeof(ap_filter_t **),
> NULL);
> > +
> > +    return APR_SUCCESS;
> > +}
> 
> Shouldn't we call filters_cleanup() from ap_remove_output_filter() too?

I think so.

> > +
> > +            f->bb = apr_brigade_create(pool, f->c->bucket_alloc);
> > +
> > +            apr_pool_pre_cleanup_register(pool, key, filters_cleanup);
> > +
> > +        }
> > +
> > +        /* decide what pool we setaside to, request pool or deferred
> pool? */
> > +        if (f->r) {
> > +            pool = f->r->pool;
> > +            APR_BRIGADE_CONCAT(f->bb, bb);
> > +        }
> > +        else {
> > +            if (!f->deferred_pool) {
> > +                apr_pool_create(&f->deferred_pool, f->c->pool);
> > +                apr_pool_tag(f->deferred_pool, "deferred_pool");
> > +            }
> > +            pool = f->deferred_pool;
> > +            return ap_save_brigade(f, &f->bb, &bb, pool);
> > +        }
> 
> Shouldn't ap_save_brigade() be moved below the "else"?

Looks like it should, plus removing APR_BRIGADE_CONCAT(f->bb, bb); above.
I think we need to set aside the buckets of bb even if we have f->r as bb may
contain transient buckets.

Regards

Rüdiger
Mime
View raw message