On Fri, Sep 10, 2021 at 5:49 PM Yann Ylavic <ylavic....@gmail.com> wrote:
>
> 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?
Should we be consistent here then, and always use the passed in reqpool?
I.e. something like the below (on the current code):
Index: buckets/apr_buckets_file.c
===================================================================
--- buckets/apr_buckets_file.c (revision 1896509)
+++ buckets/apr_buckets_file.c (working copy)
@@ -239,7 +239,6 @@ static apr_status_t file_bucket_setaside(apr_bucke
new = apr_bucket_alloc(sizeof(*new), b->list);
memcpy(new, a, sizeof(*new));
new->refcount.refcount = 1;
- new->readpool = reqpool;
a->refcount.refcount--;
a = b->data = new;
@@ -246,11 +245,9 @@ static apr_status_t file_bucket_setaside(apr_bucke
}
else {
apr_file_setaside(&fd, f, reqpool);
- if (!apr_pool_is_ancestor(a->readpool, reqpool)) {
- a->readpool = reqpool;
- }
}
a->fd = fd;
+ a->readpool = reqpool;
return APR_SUCCESS;
}
>
> Regards;
> Yann.
