philip@tigris.org writes:
> New Revision: 2452
>
> Log:
> First step for issue 749. Introduce an access baton and open/close
> functions. Replace lock/unlock calls with open/close calls and start
> passing an access baton around. Check for a write lock when creating
> adm things.
Wow. +1, Philip, and also +1 on doing it incrementally. Thanks for
taking this on!
> Modified: trunk/subversion/include/svn_wc.h
> ==============================================================================
> +/* ### Should this type be opaque in the public interface? */
> +typedef struct svn_wc_adm_access_t
> +{
Yes, I think so. Callers shouldn't know nor care what's in these
access batons, the same way an editor driver doesn't know what's in
editor batons.
> + enum svn_wc_adm_access_type {
So likewise gets an internal name.
> + svn_wc_adm_access_unlocked,
> + svn_wc_adm_access_read_lock,
And same with these.
> +/* Return an access baton in ADM_ACCESS for the working copy administrative
> + area associated with the directory PATH. If WRITE_LOCK is set the baton
> + will include a write lock, otherwise the baton can only be used for read
> + access. POOL will be used to allocate the baton and any subsequently
> + cached items. */
> +svn_error_t *svn_wc_adm_open (svn_wc_adm_access_t **adm_access,
> + const char *path,
> + svn_boolean_t write_lock,
> + apr_pool_t *pool);
So it returns an error if PATH is not a directory?
> +/* Give up the access baton ADM_ACCESS, and its lock if any */
> +svn_error_t *svn_wc_adm_close (svn_wc_adm_access_t *adm_access);
>
> +/* Ensure ADM_ACCESS has a write lock, and that the lock file still
> + exists. Returns SVN_ERR_WC_NOT_LOCKED if this is not the case. */
> +svn_error_t *svn_wc_adm_write_check (svn_wc_adm_access_t *adm_access);
Nothing wrong with talking about locking in the public interfaces, but
maybe not in such an implementation-specific way? If we one day use a
mechanism other than lock files, that wouldn't change the semantics of
this public function, yet the doc string would become inaccurate.
However, I have a more general observation, which I'd like to run by
you as a sanity check:
The svn_wc_adm_open() and svn_wc_adm_close() functions wouldn't
necessarily be public if it weren't for planned features that have
nothing to do with locking -- such as caching entries lists. Locking,
after all, is an internal integrity check that could conceivably be
done away with (were libsvn_wc written differently). So while it's
fine that these doc string all talk about locking right now, we should
keep in mind that locking is just one of several things that will be
provided by the access batons, and maybe not even the thing most of
interest to potential callers.
Thus I'm wondering if these doc strings shouldn't talk more
generically about the semantics of the access batons, and then
relegate discussion private implementation details to a secondary
"notes" section in the doc string. I'm not saying we should hide
what's going on, just make a clear distinction between the enforced
semantics and the backend requirements motivating those semantics.
> /* Set *LOCKED to non-zero if PATH is locked, else set it to zero. */
> svn_error_t *svn_wc_locked (svn_boolean_t *locked,
This will eventually leave the public interface, right?
> + /* ### Need an adm access baton to create the killme file. This
> + ### is temporary, the adm access baton should be part of the
> + ### log_runner structure. A write access baton will be
> + ### required to create the log file and will get passed to
> + ### svn_wc__run_log, at which point this call will fail and
> + ### can be removed */
> + SVN_ERR (svn_wc_adm_open (&adm_access, loggy->path, TRUE, pool));
> +
Okay, understood. Nice comment!
> Modified: trunk/subversion/libsvn_wc/lock.c
> ==============================================================================
> --- trunk/subversion/libsvn_wc/lock.c (original)
> +++ trunk/subversion/libsvn_wc/lock.c Wed Jul 10 07:05:20 2002
> @@ -24,37 +24,136 @@
> #include "wc.h"
> #include "adm_files.h"
>
> -
>
> -svn_error_t *
> -svn_wc_lock (const char *path, int wait_for, apr_pool_t *pool)
> +static svn_error_t *
> +svn_wc__lock (svn_wc_adm_access_t *adm_access, int wait_for, apr_pool_t *pool)
Minor nit: if it became static, it doesn't even need the "svn_wc__"
prefix. Just "wc_lock" would be more consistent with the way we
handle this situation elsewhere.
> +static svn_error_t *
> +svn_wc__unlock (const char *path, apr_pool_t *pool)
Same.
> +svn_error_t *
> +svn_wc_adm_write_check (svn_wc_adm_access_t *adm_access)
> +{
> + if (adm_access->type == svn_wc_adm_access_write_lock)
> + {
> + if (adm_access->lock_exists)
> + {
> + svn_boolean_t locked;
> +
> + /* Check physical lock still exists and hasn't been stolen */
> + SVN_ERR (svn_wc_locked (&locked, adm_access->path, adm_access->pool));
> + if (! locked)
> + return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, 0, NULL,
> + adm_access->pool,
> + "write-lock stolen in: %s",
> + adm_access->path);
> + }
> + }
Yeah. We still have a race condition here, but the window is pretty
small. This is the best we can do, given the existence of
svn_wc_cleanup(), I guess.
> + else
> + {
> + /* ### Could try to upgrade the read lock to a write lock here? */
> + return svn_error_createf (SVN_ERR_WC_NOT_LOCKED, 0, NULL,
> + adm_access->pool,
> + "no write-lock in: %s", adm_access->path);
Yah, don't see any reason not to...
> Modified: trunk/subversion/libsvn_wc/update_editor.c
> ==============================================================================
> --- trunk/subversion/libsvn_wc/update_editor.c (original)
> +++ trunk/subversion/libsvn_wc/update_editor.c Wed Jul 10 07:05:22 2002
> @@ -489,9 +489,10 @@
> apr_status_t apr_err;
> apr_file_t *log_fp = NULL;
> const char *base_name;
> + svn_wc_adm_access_t *adm_access;
> svn_stringbuf_t *log_item = svn_stringbuf_create ("", pool);
>
> - SVN_ERR (svn_wc_lock (pb->path, 0, pool));
> + SVN_ERR (svn_wc_adm_open (&adm_access, pb->path, TRUE, pool));
Looking at this code gives me a thought:
Perhaps svn_wc_adm_open() shouldn't allocate adm_access, but instead
take a pointer (not a double pointer) and assume it points to an
allocated structure? Then we could allocate the structure on the
stack when appropriate... such as here :-).
After all, currently, we have *two* mechanisms to control the lifetime
of an access baton: the pool it was allocated in, versus an explicit
call to close(). Of course, the latter method is the only one we
should use. So if we make the pool allocation a detail of
circumstance, instead of something that must happen every time we get
a baton, we're not really losing any control over its lifetime,
because this is one case where the lifetime is controlled by a
mechanism other than pools.
Thoughts?
-K
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Jul 10 18:23:45 2002