Philip Martin <philip@codematters.co.uk> writes:
> brane@tigris.org writes:
>> + const char **dir_name;
>> + for (dir_name = valid_dir_names; *dir_name; ++dir_name)
>> + if (0 == strcmp (name, *dir_name))
>> + {
>> + void *new_adm_dir_name = (void*) *dir_name;
>> + void *old_adm_dir_name = adm_dir_name;
>
> Perhaps 'const void *' and lose the cast?
>
>> + if (old_adm_dir_name != apr_atomic_casptr (&adm_dir_name,
>> + new_adm_dir_name,
>> + old_adm_dir_name))
>> + {
>> + /* Another thread won the race to change the name. */
>> + return svn_error_create
>> + (APR_EAGAIN, NULL,
>> + _("Could not set the administrative directory name"));
>> + }
This mail might be about to make me look stupid :)
Examining this a bit more closely I'm not sure what problem you are
trying to solve with that CAS.
With the CAS there is a race between the line:
old_adm_dir_name = adm_dir_name
and the "pseudo line":
adm_dir_name = new_adm_dir_name
and the CAS will detect that race if it occurs. However the function
doesn't really care about the original value of adm_dir_name, all it
wants to do is set the new value. So that race only exists because
you are using CAS, if the code simply did:
adm_dir_name = new_adm_dir_name
there would be no race.
Another reason for using CAS is that the adm_dir_name type might not
be atomic, and using CAS means that simultaneous writes won't leave
adm_dir_name in an invalid state. However, looking at the generic CAS
implementation in apr_atomic.c it appears that CAS won't help readers
while the write is in progress, since the readers don't lock the
mutex. I suppose I could use APR's --enable-nonportable-atomics to
avoid the generic implementation, is that recommended? What about
machines that have to use the generic implementation? Any machine
where void* is non-atomic will probably have to use the generic
implementation. I can't see from the documentation what guarantees
APR's CAS is meant to provide.
Another reason for using CAS would be to ensure that values written in
one thread become visible in another thread. However, if threads are
not otherwise syncronised but nevertheless expect svn_wc_set_adm_dir
to affect each other then that looks like an application design error.
Once the threads do synchronise then the data will become visible
without any need for the CAS.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sun Sep 25 22:14:29 2005