You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by st...@apache.org on 2011/07/01 21:04:30 UTC
svn commit: r1142026 - in /subversion/branches/svn_mutex/subversion:
include/private/svn_mutex.h libsvn_subr/svn_mutex.c
Author: stefan2
Date: Fri Jul 1 19:04:29 2011
New Revision: 1142026
URL: http://svn.apache.org/viewvc?rev=1142026&view=rev
Log:
Introduce the svn_mutex API and implement it
* subversion/include/private/svn_mutex.h
new private API header
* subversion/libsvn_subr/svn_mutex.c
implement the new API
Added:
subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h
subversion/branches/svn_mutex/subversion/libsvn_subr/svn_mutex.c
Added: subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h
URL: http://svn.apache.org/viewvc/subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h?rev=1142026&view=auto
==============================================================================
--- subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h (added)
+++ subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h Fri Jul 1 19:04:29 2011
@@ -0,0 +1,82 @@
+/**
+ * @copyright
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ * @endcopyright
+ *
+ * @file svn_mutex.h
+ * @brief Strutures and functions for mutual exclusion
+ */
+
+#ifndef SVN_MUTEX_H
+#define SVN_MUTEX_H
+
+#include <apr_thread_mutex.h>
+#include "svn_error.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif /* __cplusplus */
+
+/**
+ * This is a simple wrapper around @c apr_thread_mutex_t and will be a
+ * valid identifier even if APR does not support threading.
+ */
+typedef struct svn_mutex__t
+{
+#if APR_HAS_THREADS
+
+ /** A mutex for synchronization between threads. It may be NULL, in
+ * which case no synchronization will take place. The latter is useful,
+ * if implement some functionality where synchronization is optional.
+ */
+ apr_thread_mutex_t *mutex;
+
+#endif
+} svn_mutex__t;
+
+/** Initialize the @a *mutex with a lifetime defined by @a pool, if
+ * @a enable_mutex is TRUE and with @c NULL otherwise. If @a enable_mutex
+ * is set but threading is not supported by APR, this function returns an
+ * @c APR_ENOTIMPL error.
+ */
+svn_error_t *
+svn_mutex__init(svn_mutex__t *mutex,
+ svn_boolean_t enable_mutex,
+ apr_pool_t *pool);
+
+/** Acquire the @a mutex, if that has been enabled in @ref svn_mutex__init.
+ * Make sure to call @ref svn_mutex__unlock some time later in the same
+ * thread to release the mutex again. Recursive locking are not supported.
+ */
+svn_error_t *
+svn_mutex__lock(svn_mutex__t mutex);
+
+/** Release the @a mutex, previously acquired using @ref svn_mutex__lock
+ * that has been enabled in @ref svn_mutex__init.
+ */
+svn_error_t *
+svn_mutex__unlock(svn_mutex__t mutex,
+ svn_error_t *err);
+
+#ifdef __cplusplus
+}
+#endif /* __cplusplus */
+
+#endif /* SVN_MUTEX_H */
\ No newline at end of file
Added: subversion/branches/svn_mutex/subversion/libsvn_subr/svn_mutex.c
URL: http://svn.apache.org/viewvc/subversion/branches/svn_mutex/subversion/libsvn_subr/svn_mutex.c?rev=1142026&view=auto
==============================================================================
--- subversion/branches/svn_mutex/subversion/libsvn_subr/svn_mutex.c (added)
+++ subversion/branches/svn_mutex/subversion/libsvn_subr/svn_mutex.c Fri Jul 1 19:04:29 2011
@@ -0,0 +1,80 @@
+/*
+ * svn_mutex.c: in-memory caching for Subversion
+ *
+ * ====================================================================
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ * ====================================================================
+ */
+
+#include "svn_private_config.h"
+#include "private/svn_mutex.h"
+
+svn_error_t *
+svn_mutex__init(svn_mutex__t *mutex,
+ svn_boolean_t enable_mutex,
+ apr_pool_t *pool)
+{
+#if APR_HAS_THREADS
+ mutex->mutex = NULL;
+ if (enable_mutex)
+ {
+ apr_status_t status =
+ apr_thread_mutex_create(&mutex->mutex,
+ APR_THREAD_MUTEX_DEFAULT,
+ pool);
+ if (status)
+ return svn_error_wrap_apr(status, _("Can't create mutex"));
+ }
+#else
+ if (enable_mutex)
+ return svn_error_wrap_apr(APR_ENOTIMPL, _("APR doesn't support threads"));
+#endif
+
+ return SVN_NO_ERROR;
+}
+
+svn_error_t *
+svn_mutex__lock(svn_mutex__t mutex)
+{
+#if APR_HAS_THREADS
+ if (mutex.mutex)
+ {
+ apr_status_t status = apr_thread_mutex_lock(mutex.mutex);
+ if (status)
+ return svn_error_wrap_apr(status, _("Can't lock mutex"));
+ }
+#endif
+
+ return SVN_NO_ERROR;
+}
+
+svn_error_t *
+svn_mutex__unlock(svn_mutex__t mutex,
+ svn_error_t *err)
+{
+#if APR_HAS_THREADS
+ if (mutex.mutex)
+ {
+ apr_status_t status = apr_thread_mutex_unlock(mutex.mutex);
+ if (status && !err)
+ return svn_error_wrap_apr(status, _("Can't unlock mutex"));
+ }
+#endif
+
+ return err;
+}
on coding with callbacks and batons Re: svn commit: r1142026 - in
/subversion/branches/svn_mutex/subversion: include/private/svn_mutex.h
libsvn_subr/svn_mutex.c
Posted by Daniel Shahaf <da...@elego.de>.
Stefan Fuhrmann wrote on Mon, Jul 04, 2011 at 23:55:28 +0200:
> On 02.07.2011 04:24, Greg Stein wrote:
> >On Fri, Jul 1, 2011 at 15:04,<st...@apache.org> wrote:
> >>+/** Release the @a mutex, previously acquired using @ref svn_mutex__lock
> >>+ * that has been enabled in @ref svn_mutex__init.
> >>+ */
> >>+svn_error_t *
> >>+svn_mutex__unlock(svn_mutex__t mutex,
> >>+ svn_error_t *err);
> >>+
> >>+#ifdef __cplusplus
> >>+}
> >>+#endif /* __cplusplus */
> >I agree with Daniel's suggestion to add a "with_lock" function that
> >invokes a callback with the mutex held. That is probably the safest
> >way to use the mutexes, and it will always guarantee they are unlocked
> >(eg. in the face of an error). I see that you're accepting an ERR as a
> >parameter on the unlock, presumably to try and handle these error-exit
> >situations. My preference would be to completely omit the explicit
> >lock/unlock functions, and *only* have the with_lock concept. I think
> >it makes for better/safer code.
> >
> I understand what you guys are trying to achieve but
> there seems to be no way to do it in a meaningful way.
> r1142193 is an implementation for that and might be
> useful in some situations.
>
> In most cases, however, it will complicate things and
> make the code less dependable. My preferred locking
> style:
>
> static svn_error_t *
> bar()
> {
> ... prepare parameters 1 to 4 ...
> SVN_ERR(svn_mutex__lock(mutex));
> return svn_mutex__unlock(mutex, foo(param1, param2, param3, param4));
> }
>
> Now the same using *__with_lock:
>
> struct foo_params_t
> {
> T1 param1;
> T2 param2;
> T3 param3;
> T4 param4;
> };
>
> static svn_error_t *
> foo_void(void* baton)
> {
> struct foo_params_t * p = baton;
> return foo(p->param1, p->param2, p->param3, p->param4);
> }
>
> static svn_error_t *
> bar()
> {
> struct foo_params_t params;
>
> ... prepare parameters 1 to 4 ...
>
> params.param1 = param1;
> params.param2 = param2;
> params.param3 = param3;
> params.param4 = param4;
>
> return svn_mutex__with_lock(mutex, foo_void, ¶ms);
> }
>
> Despite the obvious coding overhead, we also lose
> type safety. So, I don't see an actual advantage in
> a widespread use of the *__with_lock API.
>
> Instead, we should strife to refactor functions such
> that they lock and unlock only in one place. Then we
> can use the initial pattern.
I see your point. (The example I have in mind is the
"svn_fs_fs__foo() { return svn_fs_fs__with_write_lock(foo_body, baton); }"
functions.)
I think it applies more generally: for example: the run_* functions in
workqueue.c generally do two things --- parse the arguments out of
a skel, and use those arguments. I think they would have been far
easier to read if each run_foo() was split into a 'parse_foo_skel()'
function (conforming to some library-private function signature) and
a 'do_foo' function (which takes explicitly in its signature the
parameters passed via the skel).
eg,
static svn_error_t *
run_file_install(svn_wc__db_t *db,
const svn_skel_t *work_item,
const char *wri_abspath,
svn_cancel_func_t cancel_func,
void *cancel_baton,
apr_pool_t *scratch_pool)
{
const char *local_relpath = work_item->children->next;
svn_boolean_t use_commit_times = work_item->children->next->next;
svn_boolean_t record_fileinfo = work_item->children->next->next->next;
const char *source_abspath = work_item->children->next->next->next->next;
return do_postupgrade(db, wri_abspath, cancel_func, cancel_baton, scratch_pool,
local_relpath, use_commit_times, record_fileinfo, NULL);
}
Re: svn commit: r1142026 - in /subversion/branches/svn_mutex/subversion:
include/private/svn_mutex.h libsvn_subr/svn_mutex.c
Posted by Greg Stein <gs...@gmail.com>.
On Mon, Jul 4, 2011 at 17:55, Stefan Fuhrmann <eq...@web.de> wrote:
>...
>> I agree with Daniel's suggestion to add a "with_lock" function that
>> invokes a callback with the mutex held. That is probably the safest
>> way to use the mutexes, and it will always guarantee they are unlocked
>> (eg. in the face of an error). I see that you're accepting an ERR as a
>> parameter on the unlock, presumably to try and handle these error-exit
>> situations. My preference would be to completely omit the explicit
>> lock/unlock functions, and *only* have the with_lock concept. I think
>> it makes for better/safer code.
>>
> I understand what you guys are trying to achieve but
> there seems to be no way to do it in a meaningful way.
> r1142193 is an implementation for that and might be
> useful in some situations.
>
> In most cases, however, it will complicate things and
> make the code less dependable. My preferred locking
> style:
>
> static svn_error_t *
> bar()
> {
> ... prepare parameters 1 to 4 ...
> SVN_ERR(svn_mutex__lock(mutex));
> return svn_mutex__unlock(mutex, foo(param1, param2, param3, param4));
> }
I understand your concern here. But having those two functions also
leads us into bad patterns. I've seen LOTS of code in svn over the
years where something gets lock, and then not-guaranteed to be
unlocked. That is why the _with_lock() pattern has been so important
lately: to clean up all that stuff. I don't want to see the pattern
continue.
How about if you say "don't use __lock and __unlock directly. use the
__with_lock *MACRO*".
You can define it something like this:
do {
svn_mutex_t *m = (mutex);
svn_error_t *e = svn_mutex_lock(m);
if (e) return svn_error_trace(e);
e = svn_mutex__unlock(m, expr);
if (e) return svn_error_trace(e);
} while (0);
Basically, a statement-level macro. Returns on any error.
Thoughts?
And obviously, the fine-grained calls could be used, but only in very
special circumstances that the code should insert comments describing
"why I'm not using SVN_MUTEX__WITH_LOCK(m, expr)"
Cheers,
-g
Re: svn commit: r1142026 - in /subversion/branches/svn_mutex/subversion:
include/private/svn_mutex.h libsvn_subr/svn_mutex.c
Posted by Stefan Fuhrmann <eq...@web.de>.
On 02.07.2011 04:24, Greg Stein wrote:
Hey Greg,
thanks for the review.
> On Fri, Jul 1, 2011 at 15:04,<st...@apache.org> wrote:
>> ...
>> +++ subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h Fri Jul 1 19:04:29 2011
>> ...
>> +#ifndef SVN_MUTEX_H
>> +#define SVN_MUTEX_H
>> +
>> +#include<apr_thread_mutex.h>
>> +#include "svn_error.h"
> Typical style is to put blank lines between the "natural groupings" of includes.
>
Fixed in r1142191.
>> +
>> +#ifdef __cplusplus
>> +extern "C" {
>> +#endif /* __cplusplus */
>> +
>> +/**
>> + * This is a simple wrapper around @c apr_thread_mutex_t and will be a
>> + * valid identifier even if APR does not support threading.
>> + */
> Actually, I don't think it will be valid: if !APR_HAS_THREADS, then
> you have an empty structure. I think some compiles will barf on that,
> so you probably want "int unused;" in that situation.
>
I got mislead with the whole struct thing. It is a simple
typedef now and much closer to APR (r1142191).
>> ...
>> +/** Initialize the @a *mutex with a lifetime defined by @a pool, if
>> + * @a enable_mutex is TRUE and with @c NULL otherwise. If @a enable_mutex
>> + * is set but threading is not supported by APR, this function returns an
>> + * @c APR_ENOTIMPL error.
>> + */
>> +svn_error_t *
>> +svn_mutex__init(svn_mutex__t *mutex,
>> + svn_boolean_t enable_mutex,
>> + apr_pool_t *pool);
>> +
>> +/** Acquire the @a mutex, if that has been enabled in @ref svn_mutex__init.
>> + * Make sure to call @ref svn_mutex__unlock some time later in the same
>> + * thread to release the mutex again. Recursive locking are not supported.
>> + */
>> +svn_error_t *
>> +svn_mutex__lock(svn_mutex__t mutex);
> In the Subversion source code, we generally do not pass structures as
> parameters. I'm not even sure if I can find an example. The code
> "always" passes a pointer-to-structure.
>
> This also has an impact on the signature for svn_mutex__init, and the
> unlock function, of course.
>
Same fix.
>> +/** Release the @a mutex, previously acquired using @ref svn_mutex__lock
>> + * that has been enabled in @ref svn_mutex__init.
>> + */
>> +svn_error_t *
>> +svn_mutex__unlock(svn_mutex__t mutex,
>> + svn_error_t *err);
>> +
>> +#ifdef __cplusplus
>> +}
>> +#endif /* __cplusplus */
> I agree with Daniel's suggestion to add a "with_lock" function that
> invokes a callback with the mutex held. That is probably the safest
> way to use the mutexes, and it will always guarantee they are unlocked
> (eg. in the face of an error). I see that you're accepting an ERR as a
> parameter on the unlock, presumably to try and handle these error-exit
> situations. My preference would be to completely omit the explicit
> lock/unlock functions, and *only* have the with_lock concept. I think
> it makes for better/safer code.
>
I understand what you guys are trying to achieve but
there seems to be no way to do it in a meaningful way.
r1142193 is an implementation for that and might be
useful in some situations.
In most cases, however, it will complicate things and
make the code less dependable. My preferred locking
style:
static svn_error_t *
bar()
{
... prepare parameters 1 to 4 ...
SVN_ERR(svn_mutex__lock(mutex));
return svn_mutex__unlock(mutex, foo(param1, param2, param3, param4));
}
Now the same using *__with_lock:
struct foo_params_t
{
T1 param1;
T2 param2;
T3 param3;
T4 param4;
};
static svn_error_t *
foo_void(void* baton)
{
struct foo_params_t * p = baton;
return foo(p->param1, p->param2, p->param3, p->param4);
}
static svn_error_t *
bar()
{
struct foo_params_t params;
... prepare parameters 1 to 4 ...
params.param1 = param1;
params.param2 = param2;
params.param3 = param3;
params.param4 = param4;
return svn_mutex__with_lock(mutex, foo_void, ¶ms);
}
Despite the obvious coding overhead, we also lose
type safety. So, I don't see an actual advantage in
a widespread use of the *__with_lock API.
Instead, we should strife to refactor functions such
that they lock and unlock only in one place. Then we
can use the initial pattern.
>> ...
>> +++ subversion/branches/svn_mutex/subversion/libsvn_subr/svn_mutex.c Fri Jul 1 19:04:29 2011
>> ...
>> +#else
>> + if (enable_mutex)
>> + return svn_error_wrap_apr(APR_ENOTIMPL, _("APR doesn't support threads"));
>> +#endif
> Euh. You can just return one of the SVN_ERR_ codes here. Wrapping an
> APR error doesn't make much sense :-P
>
> Maybe SVN_ERR_UNSUPPORTED_FEATURE ?
Done in r1142191 as well. IIRC, I took that line from
the original inprocess cache implementation.
-- Stefan^2.
Re: svn commit: r1142026 - in /subversion/branches/svn_mutex/subversion:
include/private/svn_mutex.h libsvn_subr/svn_mutex.c
Posted by Greg Stein <gs...@gmail.com>.
On Fri, Jul 1, 2011 at 15:04, <st...@apache.org> wrote:
>...
> +++ subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h Fri Jul 1 19:04:29 2011
>...
> +#ifndef SVN_MUTEX_H
> +#define SVN_MUTEX_H
> +
> +#include <apr_thread_mutex.h>
> +#include "svn_error.h"
Typical style is to put blank lines between the "natural groupings" of includes.
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +/**
> + * This is a simple wrapper around @c apr_thread_mutex_t and will be a
> + * valid identifier even if APR does not support threading.
> + */
Actually, I don't think it will be valid: if !APR_HAS_THREADS, then
you have an empty structure. I think some compiles will barf on that,
so you probably want "int unused;" in that situation.
>...
> +/** Initialize the @a *mutex with a lifetime defined by @a pool, if
> + * @a enable_mutex is TRUE and with @c NULL otherwise. If @a enable_mutex
> + * is set but threading is not supported by APR, this function returns an
> + * @c APR_ENOTIMPL error.
> + */
> +svn_error_t *
> +svn_mutex__init(svn_mutex__t *mutex,
> + svn_boolean_t enable_mutex,
> + apr_pool_t *pool);
> +
> +/** Acquire the @a mutex, if that has been enabled in @ref svn_mutex__init.
> + * Make sure to call @ref svn_mutex__unlock some time later in the same
> + * thread to release the mutex again. Recursive locking are not supported.
> + */
> +svn_error_t *
> +svn_mutex__lock(svn_mutex__t mutex);
In the Subversion source code, we generally do not pass structures as
parameters. I'm not even sure if I can find an example. The code
"always" passes a pointer-to-structure.
This also has an impact on the signature for svn_mutex__init, and the
unlock function, of course.
> +/** Release the @a mutex, previously acquired using @ref svn_mutex__lock
> + * that has been enabled in @ref svn_mutex__init.
> + */
> +svn_error_t *
> +svn_mutex__unlock(svn_mutex__t mutex,
> + svn_error_t *err);
> +
> +#ifdef __cplusplus
> +}
> +#endif /* __cplusplus */
I agree with Daniel's suggestion to add a "with_lock" function that
invokes a callback with the mutex held. That is probably the safest
way to use the mutexes, and it will always guarantee they are unlocked
(eg. in the face of an error). I see that you're accepting an ERR as a
parameter on the unlock, presumably to try and handle these error-exit
situations. My preference would be to completely omit the explicit
lock/unlock functions, and *only* have the with_lock concept. I think
it makes for better/safer code.
>...
> +++ subversion/branches/svn_mutex/subversion/libsvn_subr/svn_mutex.c Fri Jul 1 19:04:29 2011
>...
> +#else
> + if (enable_mutex)
> + return svn_error_wrap_apr(APR_ENOTIMPL, _("APR doesn't support threads"));
> +#endif
Euh. You can just return one of the SVN_ERR_ codes here. Wrapping an
APR error doesn't make much sense :-P
Maybe SVN_ERR_UNSUPPORTED_FEATURE ?
>...
Cheers,
-g
Re: svn commit: r1142026 - in /subversion/branches/svn_mutex/subversion: include/private/svn_mutex.h libsvn_subr/svn_mutex.c
Posted by Blair Zajac <bl...@orcaware.com>.
On Jul 1, 2011, at 12:04 PM, stefan2@apache.org wrote:
> Author: stefan2
> Date: Fri Jul 1 19:04:29 2011
> New Revision: 1142026
>
> URL: http://svn.apache.org/viewvc?rev=1142026&view=rev
> Log:
> Introduce the svn_mutex API and implement it
>
> * subversion/include/private/svn_mutex.h
> new private API header
> * subversion/libsvn_subr/svn_mutex.c
> implement the new API
>
> Added:
> subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h
> subversion/branches/svn_mutex/subversion/libsvn_subr/svn_mutex.c
>
> Added: subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h
> URL: http://svn.apache.org/viewvc/subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h?rev=1142026&view=auto
> ==============================================================================
> --- subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h (added)
> +++ subversion/branches/svn_mutex/subversion/include/private/svn_mutex.h Fri Jul 1 19:04:29 2011
> @@ -0,0 +1,82 @@
> +/**
> + * @copyright
> + * ====================================================================
> + * Licensed to the Apache Software Foundation (ASF) under one
> + * or more contributor license agreements. See the NOTICE file
> + * distributed with this work for additional information
> + * regarding copyright ownership. The ASF licenses this file
> + * to you under the Apache License, Version 2.0 (the
> + * "License"); you may not use this file except in compliance
> + * with the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing,
> + * software distributed under the License is distributed on an
> + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
> + * KIND, either express or implied. See the License for the
> + * specific language governing permissions and limitations
> + * under the License.
> + * ====================================================================
> + * @endcopyright
> + *
> + * @file svn_mutex.h
> + * @brief Strutures and functions for mutual exclusion
> + */
> +
> +#ifndef SVN_MUTEX_H
> +#define SVN_MUTEX_H
> +
> +#include <apr_thread_mutex.h>
> +#include "svn_error.h"
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif /* __cplusplus */
> +
> +/**
> + * This is a simple wrapper around @c apr_thread_mutex_t and will be a
> + * valid identifier even if APR does not support threading.
> + */
> +typedef struct svn_mutex__t
> +{
> +#if APR_HAS_THREADS
> +
> + /** A mutex for synchronization between threads. It may be NULL, in
> + * which case no synchronization will take place. The latter is useful,
> + * if implement some functionality where synchronization is optional.
> + */
> + apr_thread_mutex_t *mutex;
> +
> +#endif
> +} svn_mutex__t;
> +
> +/** Initialize the @a *mutex with a lifetime defined by @a pool, if
> + * @a enable_mutex is TRUE and with @c NULL otherwise. If @a enable_mutex
> + * is set but threading is not supported by APR, this function returns an
> + * @c APR_ENOTIMPL error.
The NULL part is a little confusing, it took a couple of reads to see the NULL was associated with *mutex. Suggest changing it so something like
/** If @a enable_mutex is TRUE, initialize the @a *mutex with a lifetime
* defined by @a pool, otherwise initialize it with @c NULL.
How would we use enable_mutex? Seems like a compile time constant, not a runtime one.
> + */
> +svn_error_t *
> +svn_mutex__init(svn_mutex__t *mutex,
> + svn_boolean_t enable_mutex,
> + apr_pool_t *pool);
> +
> +/** Acquire the @a mutex, if that has been enabled in @ref svn_mutex__init.
> + * Make sure to call @ref svn_mutex__unlock some time later in the same
> + * thread to release the mutex again. Recursive locking are not supported.
> + */
> +svn_error_t *
> +svn_mutex__lock(svn_mutex__t mutex);
> +
> +/** Release the @a mutex, previously acquired using @ref svn_mutex__lock
> + * that has been enabled in @ref svn_mutex__init.
> + */
> +svn_error_t *
> +svn_mutex__unlock(svn_mutex__t mutex,
> + svn_error_t *err);
What is the input err for?
Blair