On 7/30/06, Philip Martin <philip@codematters.co.uk> wrote:
> >> > +static void sasl_mutex_free_cb(void *mutex)
> >> > +{
> >> > + apr_thread_mutex_lock(array_mutex);
> >>
> >> Check for errors?
> >
> > It wouldn't do us any good here. That callback has a void return value.
> >
> >>
> >> > + *((apr_thread_mutex_t**)apr_array_push(free_mutexes)) = mutex;
> >> > + apr_thread_mutex_unlock(array_mutex);
> >> > +}
>
> Is it sensible to modify the array if the mutex lock failed? Is it
> sensible to unlock the mutex if the lock failed?
Ok, I'll fix this.
> >> > +apr_status_t svn_ra_svn__sasl_common_init()
> >> > +{
> >> > + apr_status_t apr_err = APR_SUCCESS;
> >> > +
> >> > + atexit(sasl_done);
> >>
> >> If the client calls this function more than once then it will register
> >> more than one atexit handler. Is it safe to call sasl_done more than
> >> once?
> >
> > svn_ra_svn__sasl_common_init() is an internal function, guaranteed to
> > be called only once. See svn_ra_svn__sasl_init().
>
> If Subversion is built with --enable-dso then I suspect it might get
> called more than once, certainly static variable within the library
> must get initialised each time the DSO is loaded.
Hmm, I'm not sure I understand this. Why would the DSO be loaded more than once?
> > +#include <sasl/sasl.h>
> > +
> > +/* Define sane defaults for a sasl_security_properties_t structure.
> > + See sasl.h for details. 4096 is the size of our read buffer (SASL needs
> > + to know this value when negotiating a security layer). */
> > +#define DEFAULT_SECPROPS {0, 256, 4096, 0, NULL, NULL}
>
> Does 256 correspond to "baseline AES"?
I chose 256 as the default maximum encryption strength because,
AFAICT, there are no mechanisms that provide encryption stronger than
256 bits.
We could set this to an arbitrarily high value.
> Is 4096 a number that can be changed or does it have to match some
> buffer size within the Subversion code?
It should match the size of the read_buf array in svn_ra_svn_conn_t.
The SASL docs say:
"set maxbufsize in your security properties to be the buffer size
that you pass to the read() system call—that is, the amount of
data you're prepared to read at any one time."
> DEFAULT_SECPROPS is only used once, why is it in a header file?
Because I also plan to use it on the server side.
> > +
> > +apr_status_t svn_ra_svn__sasl_common_init()
> > +{
> > + apr_status_t apr_err = APR_SUCCESS;
> > +
> > +#ifdef APR_HAS_THREADS
> > + sasl_set_mutex(sasl_mutex_alloc_cb,
> > + sasl_mutex_lock_cb,
> > + sasl_mutex_unlock_cb,
> > + sasl_mutex_free_cb);
> > + sasl_pool = svn_pool_create(NULL);
> > + apr_pool_cleanup_register(sasl_pool, NULL, sasl_done_cb,
> > + apr_pool_cleanup_null);
> > + free_mutexes = apr_array_make(sasl_pool, 0, sizeof(apr_thread_mutex_t *));
> > + apr_err = apr_thread_mutex_create(&array_mutex,
> > + APR_THREAD_MUTEX_DEFAULT,
> > + sasl_pool);
> > +#else
> > + atexit(sasl_done);
>
> atexit still looks wrong, can this use the pool mechanism used by the
> threaded case?
Well, in the single threaded case, there is no global pool, nor are
there any issues associated with mutexes or pool cleanup ordering, so
it should be safe to just call sasl_done() when the application
terminates. If I overlooked something, please elaborate.
> > +svn_error_t *svn_ra_svn__sasl_init()
> > +{
> > + /* We have to initialize the cache exactly once. Because APR
> > + doesn't have statically-initialized mutexes, we implement a poor
> > + man's spinlock using svn__atomic_cas. */
>
> This still looks like code duplication, and it's tricky code at that.
>
> How about some sort of svn_atomic_init_once(), give it a pointer to an
> svn_atomic_t and some function callbacks and all the thread/cas/sleep
> stuff would be hidden from the caller.
OK, i'd like to fix this before I resubmit this patch, but I'm not
sure where I should declare and implement this new function. Any
suggestions?
Thanks,
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Jul 31 13:56:32 2006