On 02.07.2011 04:24, Greg Stein wrote:
Hey Greg,
thanks for the review.
> On Fri, Jul 1, 2011 at 15:04,<stefan2_at_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.
Received on 2011-07-04 23:56:01 CEST