libsvn_wc (status.c) result_pool/scratch_pool

[Martin Hauner]

Hi,

there is one place in moving libsvn_wc/status.c to result_pool/scratch_pool
where I'm not sure what to do.

status.c has the following function:

static svn_error_t *
close_directory(void *dir_baton,
                 apr_pool_t *scratch_pool)


All pool usages are fine with scratch_pool but a single line looks like a
result_pool:

(currently line 1865)

               eb->anchor_status->ood_last_cmt_author =
                 apr_pstrdup(pool, db->ood_last_cmt_author);


So close_directory would need both pool parameters. But it is an implementation of:

include/svn_delta.h

   svn_error_t *(*close_directory)(void *dir_baton,
                                   apr_pool_t *pool);


I guess we don't want to change this.. ?

--
Martin

Subcommander 2.0.0 Beta 5 - http://subcommander.tigris.org
a Win32/Unix/MacOSX subversion GUI client & diff/merge tool.

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2400681

[Hyrum K. Wright-4]

On Sep 26, 2009, at 1:06 PM, Martin Hauner wrote:

> Hi,
>
> there is one place in moving libsvn_wc/status.c to result_pool/
> scratch_pool
> where I'm not sure what to do.
>
> status.c has the following function:
>
> static svn_error_t *
> close_directory(void *dir_baton,
>                 apr_pool_t *scratch_pool)
>
>
> All pool usages are fine with scratch_pool but a single line looks  
> like a
> result_pool:
>
> (currently line 1865)
>
>               eb->anchor_status->ood_last_cmt_author =
>                 apr_pstrdup(pool, db->ood_last_cmt_author);
>
>
> So close_directory would need both pool parameters. But it is an  
> implementation of:
>
> include/svn_delta.h
>
>   svn_error_t *(*close_directory)(void *dir_baton,
>                                   apr_pool_t *pool);
>
>
> I guess we don't want to change this.. ?

Correct.  We'll need to change the editor (which is also happening at  
some point).

-Hyrum

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2400683

[Julian Foad]

On Sat, 2009-09-26, Hyrum K. Wright wrote:

> On Sep 26, 2009, at 1:06 PM, Martin Hauner wrote:
> > there is one place in moving libsvn_wc/status.c to result_pool/
> > scratch_pool where I'm not sure what to do.
> >
> > status.c has the following function:
> >
> > static svn_error_t *
> > close_directory(void *dir_baton,
> >                 apr_pool_t *scratch_pool)
> >
> >
> > All pool usages are fine with scratch_pool but a single line looks  
> > like a
> > result_pool:
> >
> > (currently line 1865)
> >
> >               eb->anchor_status->ood_last_cmt_author =
> >                 apr_pstrdup(pool, db->ood_last_cmt_author);

If that allocation is being used to store results for use after the
function returns, then the pool is a "result pool". There is no harm in
a result pool also being used to hold some scratch data, so the solution
is to change the function's prototype to

static svn_error_t *
close_directory(void *dir_baton,
                apr_pool_t *result_pool)

(Or just leave it as "pool" and add a comment explaining why.)

- Julian

> >
> >
> > So close_directory would need both pool parameters. But it is an  
> > implementation of:
> >
> > include/svn_delta.h
> >
> >   svn_error_t *(*close_directory)(void *dir_baton,
> >                                   apr_pool_t *pool);
> >
> >
> > I guess we don't want to change this.. ?
>
> Correct.  We'll need to change the editor (which is also happening at  
> some point).
>
> -Hyrum
>
> ------------------------------------------------------
> http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2400683

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2401140

[Hyrum K. Wright-4]

On Sep 28, 2009, at 6:36 AM, Julian Foad wrote:

> On Sat, 2009-09-26, Hyrum K. Wright wrote:
>> On Sep 26, 2009, at 1:06 PM, Martin Hauner wrote:
>>> there is one place in moving libsvn_wc/status.c to result_pool/
>>> scratch_pool where I'm not sure what to do.
>>>
>>> status.c has the following function:
>>>
>>> static svn_error_t *
>>> close_directory(void *dir_baton,
>>>                apr_pool_t *scratch_pool)
>>>
>>>
>>> All pool usages are fine with scratch_pool but a single line looks
>>> like a
>>> result_pool:
>>>
>>> (currently line 1865)
>>>
>>>              eb->anchor_status->ood_last_cmt_author =
>>>                apr_pstrdup(pool, db->ood_last_cmt_author);
>
> If that allocation is being used to store results for use after the
> function returns, then the pool is a "result pool". There is no harm  
> in
> a result pool also being used to hold some scratch data, so the  
> solution
> is to change the function's prototype to
>
> static svn_error_t *
> close_directory(void *dir_baton,
>                apr_pool_t *result_pool)
>
> (Or just leave it as "pool" and add a comment explaining why.)

I haven't looked at the docs for this particular function, but our  
pattern for callback functions it to always provide a scratch_pool,  
and require providers of the callback to include any result_pool as  
part of their provided closure.  If close_directory() follows this  
pattern (and I see no reason why it should not), renaming the argument  
would be misleading at best.  (It also makes me wonder why we're  
dup'ing the value into a supposed scratch_pool in the first place.)

-Hyrum

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2401189

[Julian Foad]

On Mon, 2009-09-28 at 08:14 -0400, Hyrum K. Wright wrote:

> On Sep 28, 2009, at 6:36 AM, Julian Foad wrote:
>
> > On Sat, 2009-09-26, Hyrum K. Wright wrote:
> >> On Sep 26, 2009, at 1:06 PM, Martin Hauner wrote:
> >>> there is one place in moving libsvn_wc/status.c to result_pool/
> >>> scratch_pool where I'm not sure what to do.
> >>>
> >>> status.c has the following function:
> >>>
> >>> static svn_error_t *
> >>> close_directory(void *dir_baton,
> >>>                apr_pool_t *scratch_pool)
> >>>
> >>>
> >>> All pool usages are fine with scratch_pool but a single line looks
> >>> like a
> >>> result_pool:
> >>>
> >>> (currently line 1865)
> >>>
> >>>              eb->anchor_status->ood_last_cmt_author =
> >>>                apr_pstrdup(pool, db->ood_last_cmt_author);
> >
> > If that allocation is being used to store results for use after the
> > function returns, then the pool is a "result pool". There is no harm  
> > in
> > a result pool also being used to hold some scratch data, so the  
> > solution
> > is to change the function's prototype to
> >
> > static svn_error_t *
> > close_directory(void *dir_baton,
> >                apr_pool_t *result_pool)
> >
> > (Or just leave it as "pool" and add a comment explaining why.)
>
> I haven't looked at the docs for this particular function, but our  
> pattern for callback functions it to always provide a scratch_pool,  
> and require providers of the callback to include any result_pool as  
> part of their provided closure.

Ahh... yes, svn_delta_editor_t's doc string clearly implies what you
say. It looks like there was already a pool usage bug here, and that
data should instead be stored in a persistent pool.

Thanks, and sorry for the bad advice.

- Julian


>   If close_directory() follows this  
> pattern (and I see no reason why it should not), renaming the argument  
> would be misleading at best.  (It also makes me wonder why we're  
> dup'ing the value into a supposed scratch_pool in the first place.)
>
> -Hyrum

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2401212

[Daniel Rall]

On Mon, Sep 28, 2009 at 4:14 AM, Hyrum K. Wright <hyrum@...> wrote:

> On Sep 28, 2009, at 6:36 AM, Julian Foad wrote:
>
>> On Sat, 2009-09-26, Hyrum K. Wright wrote:
>>> On Sep 26, 2009, at 1:06 PM, Martin Hauner wrote:
>>>> there is one place in moving libsvn_wc/status.c to result_pool/
>>>> scratch_pool where I'm not sure what to do.
>>>>
>>>> status.c has the following function:
>>>>
>>>> static svn_error_t *
>>>> close_directory(void *dir_baton,
>>>>                apr_pool_t *scratch_pool)
>>>>
>>>>
>>>> All pool usages are fine with scratch_pool but a single line looks
>>>> like a
>>>> result_pool:
>>>>
>>>> (currently line 1865)
>>>>
>>>>              eb->anchor_status->ood_last_cmt_author =
>>>>                apr_pstrdup(pool, db->ood_last_cmt_author);
>>
>> If that allocation is being used to store results for use after the
>> function returns, then the pool is a "result pool". There is no harm
>> in
>> a result pool also being used to hold some scratch data, so the
>> solution
>> is to change the function's prototype to
>>
>> static svn_error_t *
>> close_directory(void *dir_baton,
>>                apr_pool_t *result_pool)
>>
>> (Or just leave it as "pool" and add a comment explaining why.)
>
> I haven't looked at the docs for this particular function, but our
> pattern for callback functions it to always provide a scratch_pool,
> and require providers of the callback to include any result_pool as
> part of their provided closure.  If close_directory() follows this
> pattern (and I see no reason why it should not), renaming the argument
> would be misleading at best.  (It also makes me wonder why we're
> dup'ing the value into a supposed scratch_pool in the first place.)

To avoid changing the svn_delta_editor_t's close_directory API, struct
edit_baton in libsvn_wc/status.c could grow a pool field that points
to the pool that it was allocated from (a la the field in struct
dir_baton). This way, the lifetime of
eb->anchor_status->ood_last_cmt_author will correspond to that of the
edit_baton itself.

------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2415261