Re: named_atomic review

On Mon, Nov 19, 2012 at 6:24 PM, Daniel Shahaf <d.s_at_daniel.shahaf.name>wrote:

> A few questions about named_atomic.c:
>
> Index: subversion/libsvn_subr/named_atomic.c
> ===================================================================
> --- subversion/libsvn_subr/named_atomic.c (revision 1411269)
> +++ subversion/libsvn_subr/named_atomic.c (working copy)
> @@ -309,6 +309,7 @@ delete_lock_file(void *arg)
>
> /* locks have already been cleaned up. Simply close the file */
> apr_file_close(mutex->lock_file);
> + /* ### check the return value? */
>

Yup. Fixed in r1413420.

> /* Remove the file from disk. This will fail if there ares still other
> * users of this lock file, i.e. namespace. */
> @@ -324,6 +325,7 @@ delete_lock_file(void *arg)
> static svn_error_t *
> validate(svn_named_atomic__t *atomic)
> {
> + /* ### What is the purpose of this validation? What does it do? */
>

Tries to catch callers of the named atomics API that did not check
the results of a failed call toe.g. svn_named_atomic__get. Basically
a paranoia check for NULL before dereferencing.

> return atomic && atomic->data && atomic->mutex
> ? SVN_NO_ERROR
> : svn_error_create(SVN_ERR_BAD_ATOMIC, 0, _("Not a valid atomic"));
> @@ -394,6 +396,7 @@ svn_atomic_namespace__create(svn_atomic_namespace_
> const char *shm_name, *lock_name;
> apr_finfo_t finfo;
>
> + /* ### Use a scratch_pool provided by caller? */
>

Hm, maybe. But the only caller doesn't have such a thing handy,
so changing this API would potentially ripple through much code
without giving an actual benefit.

> apr_pool_t *subpool = svn_pool_create(result_pool);
>
> /* allocate the namespace data structure
> @@ -421,10 +424,15 @@ svn_atomic_namespace__create(svn_atomic_namespace_
> apr_pool_cleanup_register(result_pool, &new_ns->mutex,
> delete_lock_file,
> apr_pool_cleanup_null);
> + /* ### If svn_atomic_namespace__create() is called twice, the cleanup
> + ### handler will be installed twice - so the lock will be removed
> + ### as soon as _either_ if them is fired. Problem? */
>

According to the APR docs on apr_file_remove, this should not
be a problem. Now documented in the code.

   /* Prevent concurrent initialization.
> */
> SVN_ERR(lock(&new_ns->mutex));
> + /* ### What if two functions reach this line of code concurrently?
> Should
> + ### the rest of the function be an svn_atomic__init_once() callback?
> */
>

No. It is permitted by design that the same process may open
the same process multiple time (holding independent references /
handles to it). The lock only prevents concurrent / racy initializations
of the shared data.

> /* First, make sure that the underlying file exists. If it doesn't
> * exist, create one and initialize its content.
> Index: subversion/include/private/svn_named_atomic.h
> ===================================================================
> --- subversion/include/private/svn_named_atomic.h (revision 1411269)
> +++ subversion/include/private/svn_named_atomic.h (working copy)
> @@ -104,8 +104,10 @@ svn_atomic_namespace__cleanup(const char *name,
> * characters and an error will be returned if the specified name is
> longer
> * than supported.
> *
> - * @note The lifetime of the atomic is bound to the lifetime
> + * @note The lifetime of the atomic object is bound to the lifetime
> * of the @a ns object, i.e. the pool the latter was created in.
> + * The namespace persists as long as at least one process holds
> + * an #svn_atomic_namespace__t object corresponding to it.
> */
>

Committed with some additional clarification.

> svn_error_t *
> svn_named_atomic__get(svn_named_atomic__t **atomic,
>
> I've skipped some details, but those that I haven't appear to be
> straightforward.
>

Perfectly fine review. Thanks!

-- Stefan^2.

-- 
Certified & Supported Apache Subversion Downloads:
*
http://www.wandisco.com/subversion/download
*