On Fri, Sep 10, 2021 at 4:44 PM Ruediger Pluem <rpl...@apache.org> wrote:
>
> On 9/10/21 4:04 PM, Yann Ylavic wrote:
> > Index: buckets/apr_buckets_file.c
> > ===================================================================
> > --- buckets/apr_buckets_file.c (revision 1893196)
> > +++ buckets/apr_buckets_file.c (working copy)
>
> > @@ -223,11 +223,33 @@ APR_DECLARE(apr_status_t) apr_bucket_file_set_buf_
> > return APR_SUCCESS;
> > }
> >
> > - if (!apr_pool_is_ancestor(a->readpool, reqpool)) {
> > - a->readpool = reqpool;
> > + /* If the file is shared/split accross multiple buckets, this bucket
> > can't
> > + * take exclusive ownership with apr_file_setaside() (thus
> > invalidating the
> > + * current/old a->fd), let's apr_file_dup() in this case instead.
> > + */
> > + if (a->refcount.refcount > 1) {
> > + apr_bucket_file *new;
> > + apr_status_t rv;
> > +
> > + rv = apr_file_dup(&fd, f, reqpool);
> > + if (rv != APR_SUCCESS) {
> > + return rv;
> > + }
> > +
> > + new = apr_bucket_alloc(sizeof(*new), b->list);
> > + memcpy(new, a, sizeof(*new));
> > + new->refcount.refcount = 1;
> > + new->readpool = reqpool;
>
> Why is the above no longer conditional on apr_pool_is_ancestor(a->readpool,
> reqpool) like in the else branch?
Good question..
Since we created a new apr_bucket_file and apr_file_t above with the
given reqpool's lifetime, I thought the reads would use it too just
like apr_bucket_file_make() uses the given pool.
But I don't really understand in the first place why we need to keep
the existing ->readpool if it's an ancestor of the given reqpool.
It's been so from the very beginning of the reqpool parameter
(r58312!), but if one wants to setaside on a subpool it may not be
thread-safe to keep using the parent pool.
While calling apr_file_setaside (or apr_file_dup now) makes sure that
the (new) file has the requested lifetime, why use the parent pool for
mmaping or !XTHREAD reopening the file?
Regards;
Yann.
