You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@gmail.com> on 2009/08/01 13:01:55 UTC
Re: svn commit: r38518 - in trunk/subversion: include libsvn_wc
On Sat, Aug 1, 2009 at 07:47, Hyrum K. Wright<hy...@hyrumwright.org> wrote:
>...
> +++ trunk/subversion/libsvn_wc/revision_status.c Fri Jul 31 22:47:16 2009 (r38518)
>...
> @@ -95,9 +101,12 @@ svn_wc_revision_status(svn_wc_revision_s
> const svn_delta_editor_t *editor;
> void *edit_baton;
> svn_revnum_t edit_revision;
> - svn_wc_revision_status_t *result = apr_palloc(pool, sizeof(**result_p));
> + svn_wc_revision_status_t *result = apr_palloc(result_pool,
> + sizeof(**result_p));
> *result_p = result;
I realize you didn't into this, but the sizeof() above is very bad
form. It is the size of a *different* variable. Sure, it happens to be
the same size, but "happens" and "is definitely" are two different
things.
>...
> @@ -108,14 +117,14 @@ svn_wc_revision_status(svn_wc_revision_s
> /* initialize walking baton */
> sb.result = result;
> sb.committed = committed;
> - sb.wc_path = wc_path;
> + sb.local_abspath = local_abspath;
> sb.wc_url = NULL;
> - sb.pool = pool;
> + sb.pool = scratch_pool;
>
> SVN_ERR(svn_wc_adm_open_anchor(&anchor_access, &target_access, &target,
> - wc_path, FALSE, -1,
> + local_abspath, FALSE, -1,
> cancel_func, cancel_baton,
> - pool));
> + scratch_pool));
Access batons should not be opened with absolute paths. The
association stuff will not work properly.
I also suspect you want to use one of the new "open with DB" functions
that Bert added. And if you do, then using consistent paths will be
even MORE important.
Of course, better yet is to just remove access baton usage from this function.
>...
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377741
Re: svn commit: r38518 - in trunk/subversion: include libsvn_wc
Posted by "Hyrum K. Wright" <hy...@hyrumwright.org>.
On Aug 1, 2009, at 8:04 AM, Greg Stein wrote:
> On Sat, Aug 1, 2009 at 15:01, Greg Stein<gs...@gmail.com> wrote:
>> On Sat, Aug 1, 2009 at 07:47, Hyrum K.
>> Wright<hy...@hyrumwright.org> wrote:
>>> ...
>>> +++ trunk/subversion/libsvn_wc/revision_status.c Fri Jul 31
>>> 22:47:16 2009 (r38518)
>>> ...
>>> @@ -95,9 +101,12 @@ svn_wc_revision_status(svn_wc_revision_s
>>> const svn_delta_editor_t *editor;
>>> void *edit_baton;
>>> svn_revnum_t edit_revision;
>>> - svn_wc_revision_status_t *result = apr_palloc(pool,
>>> sizeof(**result_p));
>>> + svn_wc_revision_status_t *result = apr_palloc(result_pool,
>>> +
>>> sizeof(**result_p));
>>> *result_p = result;
>>
>> I realize you didn't into this, but the sizeof() above is very bad
>> form. It is the size of a *different* variable. Sure, it happens to
>> be
>> the same size, but "happens" and "is definitely" are two different
>> things.
r38520.
>>> ...
>>> @@ -108,14 +117,14 @@ svn_wc_revision_status(svn_wc_revision_s
>>> /* initialize walking baton */
>>> sb.result = result;
>>> sb.committed = committed;
>>> - sb.wc_path = wc_path;
>>> + sb.local_abspath = local_abspath;
>>> sb.wc_url = NULL;
>>> - sb.pool = pool;
>>> + sb.pool = scratch_pool;
>>>
>>> SVN_ERR(svn_wc_adm_open_anchor(&anchor_access, &target_access,
>>> &target,
>>> - wc_path, FALSE, -1,
>>> + local_abspath, FALSE, -1,
>>> cancel_func, cancel_baton,
>>> - pool));
>>> + scratch_pool));
>>
>> Access batons should not be opened with absolute paths. The
>> association stuff will not work properly.
Yikes! Is this documented anywhere?
>> I also suspect you want to use one of the new "open with DB"
>> functions
>> that Bert added. And if you do, then using consistent paths will be
>> even MORE important.
So, here's how I fell down this hole, and any suggestions for getting
out are appreciated.
Bert only wrote svn_wc__adm_open_in_context(), so I've got a local
patch for svn_wc__adm_probe_in_context(). When using this inside of
libsvn_client/merge.c, I get a segfault on pool cleanup. I've traced
this to a double close somewhere: a svn_wc__db_t * is being shared
somewhere, hand the object that is "borrowing" it is attempting to
close it after it has already been closed. Kaboom.
I *think* the errant usage is related to the use of
svn_wc_revision_status2() within the merge code, hence my efforts to
switch it over to use wc_ctx. But that turns out to be a mountain of
work because it uses svn_wc_status2() which returns an
svn_wc_status2_t struct which in turn contains an entry. We don't
have to rewrite the entire status stuff just yet but letting it use a
wc_ctx would mean svn_wc_revision_status2() could grab it's access
batons from the wc_ctx->db ... and hopefully that would solve the
double free problem. Convoluted, I know, and I'm fixin' to have a
stack overflow just thinking about it.
Writing this just now, I think I may have an idea of how to solve it.
I'll keep poking around.
>> Of course, better yet is to just remove access baton usage from
>> this function.
>
> Even better is to revamp the status stuff and entirely remove the guts
> of this function. The callback is a simple function. Using editors in
> here is an insane way to do a simple walk to find nodes to report
> status on.
Yeah, that does seem like overkill. Another function rewrite to put
on the ever-growing queue. :/
-Hyrum
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2378508
Re: svn commit: r38518 - in trunk/subversion: include libsvn_wc
Posted by Greg Stein <gs...@gmail.com>.
On Sat, Aug 1, 2009 at 15:01, Greg Stein<gs...@gmail.com> wrote:
> On Sat, Aug 1, 2009 at 07:47, Hyrum K. Wright<hy...@hyrumwright.org> wrote:
>>...
>> +++ trunk/subversion/libsvn_wc/revision_status.c Fri Jul 31 22:47:16 2009 (r38518)
>>...
>> @@ -95,9 +101,12 @@ svn_wc_revision_status(svn_wc_revision_s
>> const svn_delta_editor_t *editor;
>> void *edit_baton;
>> svn_revnum_t edit_revision;
>> - svn_wc_revision_status_t *result = apr_palloc(pool, sizeof(**result_p));
>> + svn_wc_revision_status_t *result = apr_palloc(result_pool,
>> + sizeof(**result_p));
>> *result_p = result;
>
> I realize you didn't into this, but the sizeof() above is very bad
> form. It is the size of a *different* variable. Sure, it happens to be
> the same size, but "happens" and "is definitely" are two different
> things.
>
>>...
>> @@ -108,14 +117,14 @@ svn_wc_revision_status(svn_wc_revision_s
>> /* initialize walking baton */
>> sb.result = result;
>> sb.committed = committed;
>> - sb.wc_path = wc_path;
>> + sb.local_abspath = local_abspath;
>> sb.wc_url = NULL;
>> - sb.pool = pool;
>> + sb.pool = scratch_pool;
>>
>> SVN_ERR(svn_wc_adm_open_anchor(&anchor_access, &target_access, &target,
>> - wc_path, FALSE, -1,
>> + local_abspath, FALSE, -1,
>> cancel_func, cancel_baton,
>> - pool));
>> + scratch_pool));
>
> Access batons should not be opened with absolute paths. The
> association stuff will not work properly.
>
> I also suspect you want to use one of the new "open with DB" functions
> that Bert added. And if you do, then using consistent paths will be
> even MORE important.
>
> Of course, better yet is to just remove access baton usage from this function.
Even better is to revamp the status stuff and entirely remove the guts
of this function. The callback is a simple function. Using editors in
here is an insane way to do a simple walk to find nodes to report
status on.
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2377742