Kamesh Jayachandran wrote:
> C. Michael Pilato wrote:
>> Kamesh Jayachandran wrote:
>>
>>> C. Michael Pilato wrote:
>>>
>>>> Today, I was poking thru the BDB code, and noticed the new public FS
>>>> APIs
>>>> for merge tracking. As I dug into them a little bit deeper, though, I
>>>> saw
>>>> something that didn't seem quite right: base_txn_merge_info() doesn't
>>>> wrap
>>>> its read of the txn's proplist in a BDB transaction. Was that an
>>>> intentional deviation from the way svn_fs_base__txn_proplist() works
>>>> (even
>>>> though the two are super-similar in functionality)?
>>>>
>>>>
>>> Yes it is intentional.
>>> If we use 'svn_fs_base__retry_txn' instead of 'svn_fs_base__retry' it
>>> tries to open one more trail and hence aborts.
>>>
>>> 'begin_txn' in subversion/libsvn_fs_base/trail.c has the following
>>> comment,
>>> <snip>
>>> /* [*]
>>> If we're already inside a trail operation, abort() -- this is
>>> a coding problem (and will likely hang the repository
>>> anyway). */
>>>
>>> </snip>
>>>
>>
>> base_txn_merge_info() is a vtable implementation of a public function,
>> and
>> is not a trail-protected function. It should be able to call
>> svn_fs_base__retry_txn() just fine. If you're getting an abort() when
>> coded
>> that way, it's because you're calling into an FS vtable function from
>> some
>> other trail-protected function, which is both unnecessary and a
>> violation of
>> the (admittedly undocumented) libsvn_fs_base API usage rules.
>>
>> If the only intention here was avoiding an abort(), then the code is
>> just wrong.
>>
>>
> It happens because we call 'base_txn_mergeinfo' indirectly.
> Call chain upon commit in the order of call.
> txn_vtable's commit_txn hook(svn_fs_base__commit_txn)
> |
> V
> svn_fs_merge_info__update_index (available in libsvn_fs_util)
> |
> V
> txn_vtable's get_mergeinfo hook(base_txn_mergeinfo)
Okay, that's a violation of the libsvn_fs_base APIs. Here are the rules:
* Use transaction trails where you need them to implement public FS APIs
* Do *NOT* call a public API that uses transaction trails from inside
another transaction trails.
You're violating the first rule because you aren't transaction-protecting
your read of the proplist. But the naive fix will cause a violation of the
second rule.
But I think that latter situation is the real problem. Why in the world do
we have calls into an FS plugin from the FS_util library?! The util library
may be called *by* the FS plugins and FS loader, but shouldn't be calling
*into* either of those things.
--
C. Michael Pilato <cmpilato@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Received on Mon Apr 2 17:27:32 2007