Re: svn commit: r1142130 - in /subversion/branches/svn_mutex/subversion/libsvn_fs_fs: fs.c fs.h fs_fs.c

On Jul 1, 2011, at 5:27 PM, stefan2_at_apache.org wrote:

> Author: stefan2
> Date: Sat Jul 2 00:27:28 2011
> New Revision: 1142130
>
> URL: http://svn.apache.org/viewvc?rev=1142130&view=rev
> Log:
> Replace usage of plain APR thread mutex API with the new svn_mutex
> API in FSFS.

> -#if APR_HAS_THREADS
> - /* We also need a mutex for synchronising access to the active
> - transaction list and free transaction pointer. */
> - status = apr_thread_mutex_create(&ffsd->txn_list_lock,
> - APR_THREAD_MUTEX_DEFAULT,
> common_pool);
> - if (status)
> - return svn_error_wrap_apr(status,
> - _("Can't create FSFS txn list
> mutex"));
> -#endif
> -
> + SVN_ERR(svn_mutex__init(&ffsd->txn_list_lock, TRUE,
> common_pool));

I don't get why you have enable_mutex. On a platform without thread
support, svn_mutex__init with TRUE will always return APR_ENOTIMPL.
The proper fix is to wrap APR_HAS_THREAD around it:

#if APR_HAS_THREADS
SVN_ERR(svn_mutex__init(&ffsd->txn_list_lock, TRUE, common_pool));
#endif

but that seems like unnecessary work. I would rather see this

SVN_ERR(svn_mutex__init(&ffsd->txn_list_lock, common_pool));

and the mutex will behave with or without locking automatically.

Finally, looking at svn_mutex__init's implementation, I would state
that the wrapped pointer will always either be NULL or a valid
pointer, since you always set it to 0.

Blair
Received on 2011-07-02 04:35:38 CEST