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,<stef...@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);
}
