[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Daniel Shahaf <danielsh_at_elego.de>
Date: Tue, 5 Jul 2011 01:21:13 +0300

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,<stefan2_at_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, &params);
> }
>
> 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);
}
Received on 2011-07-05 00:22:35 CEST

This is an archived mail posted to the Subversion Dev mailing list.