Re: svn commit: r1552324 - Introduce svn_ra_session_dup()

Bert Huijben wrote:

> svn_ra_session_open4 already uses some variables from the config without
> duplicating them to the internal pool, so in that way it is not a first.

(It's svn_ra_open4.) That's undocumented. That sounds like a bug. Since it's already released, we should update the doc string to say so, but going forward do we want to continue that practice? I would have thought it better (less surprising) not to do so.

> svn_ra_dup_session() documents that it will re-use the original arguments
> of the first Ra session, which includes references to things like batons,
> that can't be duplicated.

That's not how I interpret what it currently says:

/**
 * Open a new ra session @a *new_session to the same repository as an existing
 * ra session @a old_session, copying the callbacks, auth baton, etc. from the
 * old session.

Please make that doc string more explicit if the intent is that the original 'config' and other pointers must still be valid.

Also svn_ra_open4() should document the lifetime requirements of each of its arguments, and for the compound arguments ('callbacks', 'callback_baton', 'config') whether the structures themselves or only (specified) items within them need to live for the duration of the session.

> (And Ra serf already did some internal session duplication before this
> patch)
>
> No, it is not the cleanest way to implement this, but I don't see any other
> option of creating helper Ra sessions from the Ra layer for the compact shims.
>
> The way we use it now from libsvn_client is safe as we always construct these
> sessions based on the state in svn_client_ctx_t, which must outlive both Ra
> sessions.
>
> Bert

p.s. Please avoid top-posting and HTML on this list.

Thanks.
- Julian

>From: Julian Foad
>> URL: http://svn.apache.org/r1552324
>[...]
>> @@ -494,6 +494,7 @@ svn_ra_serf__open(svn_ra_session_t *sess
>>
>>    serf_sess = apr_pcalloc(pool, sizeof(*serf_sess));
>>    serf_sess->pool = svn_pool_create(pool);
>> +  serf_sess->config = config;
>
> Is this safe -- storing the old pointer to 'config' ...
> ... and then using it later? I don't see any lifetime guarantee.
Received on 2013-12-20 16:03:32 CET