On Aug 1, 2009, at 8:04 AM, Greg Stein wrote:
> On Sat, Aug 1, 2009 at 15:01, Greg Stein<gstein_at_gmail.com> wrote:
>> On Sat, Aug 1, 2009 at 07:47, Hyrum K.
>> Wright<hyrum_at_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
Received on 2009-08-01 23:13:21 CEST