RE: svn commit: r1555350-/subversion/trunk/subversion/libsvn_client/mtcc.c

There are more users of the commit logic than just our command line tools and libsvn client.

The Ra API is a public API that can't just be broken for nicer logic in the Fs layer. Mucc is just another driver of the Ra commit logic.

If we have to revert something it would have to be the added pool requirements for 1.9. We can't change requirements for 1.0 APIs.

The mtcc code opens a commit editor and drives it to completion. There are no documented pool rules for that in 1.0-1.8. We need a time machine to change those rules or strict erata documenting what every API user should change. I see none of those for 1.9 yet?

Your patch doesn't even document what the real problem is that you fix. You are just shifting fsfs or fsx problems towards API users.

Without proper documentation this kind of requirement changes belong in a 2.0 release, not hidden without any documentation in a 1.9 or 1.10 release.

As is the cache logic is flawed if it has all these new pool usage requirements that we have to apply all through our code. (Not just here but svnserve and a few other applications were changed for similar things)

Bert

-----Original Message-----
From: "Stefan Fuhrmann" <stefan.fuhrmann_at_wandisco.com>
Sent: ‎4-‎1-‎2014 22:26
To: "Bert Huijben" <bert_at_qqmail.nl>
Cc: "dev_at_subversion.apache.org" <dev_at_subversion.apache.org>; "commits_at_subversion.apache.org" <commits_at_subversion.apache.org>
Subject: Re: svn commit: r1555350-/subversion/trunk/subversion/libsvn_client/mtcc.c

On Sat, Jan 4, 2014 at 9:47 PM, Bert Huijben <bert_at_qqmail.nl> wrote:

You only know this for the success path. The fs api should hide these details..

With all due respect, the fault is svn_client_mtcc_commit

destroying mtcc->pool before destroying mtcc itself (via

its owning pool). Hence, the caller has no control over pool
lifetimes and any allocation in sibling pools, e.g. scratch_pool,
is potentially invalid.

There is no way we can document these guarantees for public APIs and really assume that all existing and future users follow all these new requirements for a new Fs layer cache.

We had similar caches in the past, but hid the ugly details by installing cleanup handlers...

We should really look at this well before 1.9, as adding such requirements is a breaking change. And the commit API already existed since well before 1.0 without these pool requirements. Strict dual pools are only common since 1.7, but especially generated bindings (like swig) have strange pool handling much longer.

 
I leave it to the author of the mucc code to simply go back

to standard pool handling by destroying the mtcc with its
parent pool instead of partly destroying it in some API function.

-- Stefan^2.

From: stefan2_at_apache.org
Sent: 4-1-2014 15:05
To: commits_at_subversion.apache.org
Subject: svn commit: r1555350 -/subversion/trunk/subversion/libsvn_client/mtcc.c

Author: stefan2
Date: Sat Jan 4 14:04:36 2014
New Revision: 1555350

URL: http://svn.apache.org/r1555350
Log:
Fix segfault in svnmucc.

* subversion/libsvn_client/mtcc.c
  (svn_client_mtcc_commit): We explicitly destroy the MTCC pool and
                             have no knowledge about its relation to
                             SCRATCH_POOL. Thus, we can't use the
                             for anything non-trivial.

Modified:
    subversion/trunk/subversion/libsvn_client/mtcc.c

Modified: subversion/trunk/subversion/libsvn_client/mtcc.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_client/mtcc.c?rev=1555350&r1=1555349&r2=1555350&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_client/mtcc.c (original)
+++ subversion/trunk/subversion/libsvn_client/mtcc.c Sat Jan 4 14:04:36 2014
@@ -1337,12 +1337,15 @@ svn_client_mtcc_commit(apr_hash_t *revpr
                                "is not a directory"),
                              session_url);

+ /* Beware that the editor object must not live longer than the MTCC.
+ Otherwise, txn objects etc. in EDITOR may live longer than their
+ respective FS objects. So, we can't use SCRATCH_POOL here. */
   SVN_ERR(svn_ra_get_commit_editor3(mtcc->ra_session, &editor, &edit_baton,
                                     commit_revprops,
                                     commit_callback, commit_baton,
                                     NULL /* lock_tokens */,
                                     FALSE /* keep_locks */,
- scratch_pool));
+ mtcc->pool));

   err = editor->open_root(edit_baton, mtcc->base_revision, scratch_pool, &root_baton);
Received on 2014-01-04 23:53:23 CET