On Mon, Jul 4, 2011 at 17:55, Stefan Fuhrmann <eqfox_at_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
Received on 2011-07-05 22:50:13 CEST