"Vlad Georgescu" <vgeorgescu@gmail.com> writes:
> On 7/30/06, Philip Martin <philip@codematters.co.uk> wrote:
>>
>> 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?
If the pool used to load the DSO is cleared/destroyed then the DSO
gets unloaded. As far as I recall even the command line client can
load the RA libraries multiple times, while a long running GUI might
load them many times.
>
>> > +#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.
I don't care which value you choose since we can always change it
later, but you need to explain how you made the choice so that anyone
considering changing it knows why the current value was chosen. You
are not required to do detailed analysis, a "suck it and see" approach
is fine, so long as you document the choice.
>> 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."
So are there two literal 4096's in the code? Should we use a #define?
>
>> 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.
If the DSO gets loaded twice then sasl_done will get called twice,
does that work? If the DSO gets loaded more than ATEXIT_MAX times it
will fill up all the atexit slots.
If the DSO is loaded twice then sasl_client_init will get called twice
without an intervening sasl_done since sasl_done is postponed until
the application exits, does that work? The threaded code might have
the same problem, the use of a global pool means that sasl_done_cb
will only get called when apr is shut down, not when the DSO is
unloaded.
In fact the threaded code might have a problem even if the DSO is only
loaded once, since sasl_done_cb is part of the DSO and so the callback
code will disappear before when the DSO is unloaded. If that happens
before the global pool is destroyed, and with --enable-dso that's
likely, then when the callback is finally called it will cause a
SIGBUS or SIGSEGV. The fs code currently has exactly that problem at
present.
>
>> > +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?
I suppose include/svn_atomic.h and libsvn_subr/atomic.c.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Tue Aug 1 00:47:09 2006