On 1/7/22 3:02 PM, Yann Ylavic wrote:
> 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.
But if we we setaside on a subpool of readpool where the fd pool is also an
ancestor of the subpool
like readpool == fd->pool, then file_bucket_setaside is a noop, because of the
first if. Hence I think
it is not made to setaside in subpools, but only to pools which may have an
unconnected lifeccyle.
>> 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):
Maybe a good idea.
Regards
Rüdiger
