You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Vlad Georgescu <vg...@gmail.com> on 2006/08/08 22:01:41 UTC
Re: [PATCH] Client-side Cyrus SASL support
I modified this patch to use svn_atomic_init_once, and also made a
couple of other changes.
[[[
Add client-side support for Cyrus SASL.
* configure.in: Define SVN_HAVE_SASL.
* subversion/libsvn_ra_svn/client.c
(svn_ra_svn__init): Call svn_ra_svn__sasl_init.
* subversion/libsvn_ra_svn/ra_svn_sasl.h: New file.
* subversion/libsvn_ra_svn/sasl_auth.c: New file.
* subversion/libsvn_ra_svn/simple_auth.c:
Enclose content within an #ifndef SVN_HAVE_SASL .. #endif pair.
* subversion/libsvn_ra_svn/ra_svn.h
(SVN_RA_SVN__READBUF_SIZE,
SVN_RA_SVN__WRITEBUF_SIZE): New defines.
(svn_ra_svn_conn_st): Use them here.
(svn_ra_svn__sasl_init): New declaration.
]]]
--
Vlad
Re: [PATCH] Client-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/17/06, Vlad Georgescu <vg...@gmail.com> wrote:
> On 8/17/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> > On 8/14/06, Vlad Georgescu <vg...@gmail.com> wrote:
> > > I made a couple of changes to this patch to make it compile
> > > successfully under win32. On Unix, we include <sasl/sasl.h>, but this
> > > doesn't work on windows, we need to include <sasl.h> because we use
> > > Cyrus SASL's include directory directly. We also need to define
> > > STRUCT_IOVEC_DEFINED, so that sasl.h doesn't try to redefine iovec
> > > (this was initially in D.J. Heap's patch, I moved it here to keep the
> > > solution close to the problem). See also:
> > > http://svn.haxx.se/dev/archive-2006-08/0411.shtml
> >
> > It looks like the only thing left to resolve is Philip's question
> > about use of atexit versus pool cleanups for the sasl_done stuff.
> >
> > I don't personally have a strong opinion on it, although I lean
> > towards agreeing with Philip since he's usually right.
> >
>
> I changed the patch to use the pool cleanup in all cases. I'm still
> not 100% convinced that this is the right thing to do, but it's only a
> minor issue, there's no point in debating it any further.
Looks good to me. Committed in r21109.
Thanks for being so great about incorporating feedback on this patch Vlad,
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Client-side Cyrus SASL support
Posted by Vlad Georgescu <vg...@gmail.com>.
On 8/17/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> On 8/14/06, Vlad Georgescu <vg...@gmail.com> wrote:
> > I made a couple of changes to this patch to make it compile
> > successfully under win32. On Unix, we include <sasl/sasl.h>, but this
> > doesn't work on windows, we need to include <sasl.h> because we use
> > Cyrus SASL's include directory directly. We also need to define
> > STRUCT_IOVEC_DEFINED, so that sasl.h doesn't try to redefine iovec
> > (this was initially in D.J. Heap's patch, I moved it here to keep the
> > solution close to the problem). See also:
> > http://svn.haxx.se/dev/archive-2006-08/0411.shtml
>
> It looks like the only thing left to resolve is Philip's question
> about use of atexit versus pool cleanups for the sasl_done stuff.
>
> I don't personally have a strong opinion on it, although I lean
> towards agreeing with Philip since he's usually right.
>
I changed the patch to use the pool cleanup in all cases. I'm still
not 100% convinced that this is the right thing to do, but it's only a
minor issue, there's no point in debating it any further.
--
Vlad
Re: [PATCH] Client-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/14/06, Vlad Georgescu <vg...@gmail.com> wrote:
> I made a couple of changes to this patch to make it compile
> successfully under win32. On Unix, we include <sasl/sasl.h>, but this
> doesn't work on windows, we need to include <sasl.h> because we use
> Cyrus SASL's include directory directly. We also need to define
> STRUCT_IOVEC_DEFINED, so that sasl.h doesn't try to redefine iovec
> (this was initially in D.J. Heap's patch, I moved it here to keep the
> solution close to the problem). See also:
> http://svn.haxx.se/dev/archive-2006-08/0411.shtml
It looks like the only thing left to resolve is Philip's question
about use of atexit versus pool cleanups for the sasl_done stuff.
I don't personally have a strong opinion on it, although I lean
towards agreeing with Philip since he's usually right.
On the other hand, this patch has been kicking around for quite some
time, so I'm starting to see some arguement in favor of just
committing what we've got now, and incrementally improving it once
it's in trunk.
Thoughts?
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Client-side Cyrus SASL support
Posted by Vlad Georgescu <vg...@gmail.com>.
I made a couple of changes to this patch to make it compile
successfully under win32. On Unix, we include <sasl/sasl.h>, but this
doesn't work on windows, we need to include <sasl.h> because we use
Cyrus SASL's include directory directly. We also need to define
STRUCT_IOVEC_DEFINED, so that sasl.h doesn't try to redefine iovec
(this was initially in D.J. Heap's patch, I moved it here to keep the
solution close to the problem). See also:
http://svn.haxx.se/dev/archive-2006-08/0411.shtml
--
Vlad
Re: [PATCH] Client-side Cyrus SASL support
Posted by Vlad Georgescu <vg...@gmail.com>.
On 8/10/06, Philip Martin <ph...@codematters.co.uk> wrote:
> "Vlad Georgescu" <vg...@gmail.com> writes:
>
> > +apr_status_t svn_ra_svn__sasl_common_init(void)
> > +{
> > + 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);
> > +#endif /* APR_HAS_THREADS */
> > + return apr_err;
> > +}
>
> I still think it's odd for the library to use atexit, and for the
> threaded/non-threaded code to be so different. Why doesn't the
> non-threaded code just use the threaded code but without the mutexes?
Sure, you could use the global pool and the pool cleanup in the
single-threaded case, but it doesn't really make sense. That pool is
only necessary for the mutex allocations, so you'd be creating a pool
just to register a cleanup for it.
--
Vlad
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Client-side Cyrus SASL support
Posted by Philip Martin <ph...@codematters.co.uk>.
"Vlad Georgescu" <vg...@gmail.com> writes:
> +apr_status_t svn_ra_svn__sasl_common_init(void)
> +{
> + 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);
> +#endif /* APR_HAS_THREADS */
> + return apr_err;
> +}
I still think it's odd for the library to use atexit, and for the
threaded/non-threaded code to be so different. Why doesn't the
non-threaded code just use the threaded code but without the mutexes?
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Client-side Cyrus SASL support
Posted by Vlad Georgescu <vg...@gmail.com>.
On 8/9/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> > > +static void *sasl_mutex_alloc_cb(void)
> > > +{
> > > + apr_thread_mutex_t *mutex;
> > > + apr_status_t apr_err;
> > > + if (apr_is_empty_array(free_mutexes))
> > > + {
> > > + apr_err = apr_thread_mutex_create(&mutex,
> > > + APR_THREAD_MUTEX_DEFAULT,
> > > + sasl_pool);
> > > + if (apr_err != APR_SUCCESS)
> > > + return NULL;
> > > + }
> > > + else
> > > + {
> > > + apr_err = apr_thread_mutex_lock(array_mutex);
> > > + if (apr_err != APR_SUCCESS)
> > > + return NULL;
> > > + mutex = *((apr_thread_mutex_t**)apr_array_pop(free_mutexes));
> > > + apr_err = apr_thread_mutex_unlock(array_mutex);
> > > + if (apr_err != APR_SUCCESS)
> > > + return NULL;
> > > + }
> > > + return mutex;
> > > +}
> > >
> > > You can't access the array without locking the mutex. The lock needs
> > > to be pulled up outside the if statement, before you call
> > > apr_is_empty_array, otherwise none of it is thread safe. Basically
> > > you need to lock the mutex before ANY access to that array.
> >
> > Oops. Thanks for pointing that out.
>
> Still not quite right:
>
> +static void *sasl_mutex_alloc_cb(void)
> +{
> + apr_thread_mutex_t *mutex;
> + apr_status_t apr_err;
> +
> + apr_err = apr_thread_mutex_lock(array_mutex);
> + if (apr_err != APR_SUCCESS)
> + return NULL;
> +
> + if (apr_is_empty_array(free_mutexes))
> + {
> + apr_err = apr_thread_mutex_create(&mutex,
> + APR_THREAD_MUTEX_DEFAULT,
> + sasl_pool);
> + if (apr_err != APR_SUCCESS)
> + return NULL;
>
> If you return here you'll leave the mutex locked, you've gotta unlock
> it before returning.
>
> -garrett
>
I can't believe I missed that. See the new patched patch.
--
Vlad
Re: [PATCH] Client-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/9/06, Vlad Georgescu <vg...@gmail.com> wrote:
> On 8/9/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> > A few more problems:
> >
> > +/* Define sane defaults for a sasl_security_properties_t structure.
> > + See sasl.h for details. SASL needs to know our read buffer's size
> > + when negotiating a security layer. */
> > +#define SVN_RA_SVN__DEFAULT_SECPROPS {0, 256, SVN_RA_SVN__READBUF_SIZE, \
> > + 0, NULL, NULL}
> >
> > Please explain what 256 means in this context. I think it's been
> > discussed on this list, but it really should make it into the comment.
>
> Done.
Thanks!
> > +static void *sasl_mutex_alloc_cb(void)
> > +{
> > + apr_thread_mutex_t *mutex;
> > + apr_status_t apr_err;
> > + if (apr_is_empty_array(free_mutexes))
> > + {
> > + apr_err = apr_thread_mutex_create(&mutex,
> > + APR_THREAD_MUTEX_DEFAULT,
> > + sasl_pool);
> > + if (apr_err != APR_SUCCESS)
> > + return NULL;
> > + }
> > + else
> > + {
> > + apr_err = apr_thread_mutex_lock(array_mutex);
> > + if (apr_err != APR_SUCCESS)
> > + return NULL;
> > + mutex = *((apr_thread_mutex_t**)apr_array_pop(free_mutexes));
> > + apr_err = apr_thread_mutex_unlock(array_mutex);
> > + if (apr_err != APR_SUCCESS)
> > + return NULL;
> > + }
> > + return mutex;
> > +}
> >
> > You can't access the array without locking the mutex. The lock needs
> > to be pulled up outside the if statement, before you call
> > apr_is_empty_array, otherwise none of it is thread safe. Basically
> > you need to lock the mutex before ANY access to that array.
>
> Oops. Thanks for pointing that out.
Still not quite right:
+static void *sasl_mutex_alloc_cb(void)
+{
+ apr_thread_mutex_t *mutex;
+ apr_status_t apr_err;
+
+ apr_err = apr_thread_mutex_lock(array_mutex);
+ if (apr_err != APR_SUCCESS)
+ return NULL;
+
+ if (apr_is_empty_array(free_mutexes))
+ {
+ apr_err = apr_thread_mutex_create(&mutex,
+ APR_THREAD_MUTEX_DEFAULT,
+ sasl_pool);
+ if (apr_err != APR_SUCCESS)
+ return NULL;
If you return here you'll leave the mutex locked, you've gotta unlock
it before returning.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Client-side Cyrus SASL support
Posted by Vlad Georgescu <vg...@gmail.com>.
On 8/9/06, Garrett Rooney <ro...@electricjellyfish.net> wrote:
> A few more problems:
>
> +/* Define sane defaults for a sasl_security_properties_t structure.
> + See sasl.h for details. SASL needs to know our read buffer's size
> + when negotiating a security layer. */
> +#define SVN_RA_SVN__DEFAULT_SECPROPS {0, 256, SVN_RA_SVN__READBUF_SIZE, \
> + 0, NULL, NULL}
>
> Please explain what 256 means in this context. I think it's been
> discussed on this list, but it really should make it into the comment.
Done.
> +static void *sasl_mutex_alloc_cb(void)
> +{
> + apr_thread_mutex_t *mutex;
> + apr_status_t apr_err;
> + if (apr_is_empty_array(free_mutexes))
> + {
> + apr_err = apr_thread_mutex_create(&mutex,
> + APR_THREAD_MUTEX_DEFAULT,
> + sasl_pool);
> + if (apr_err != APR_SUCCESS)
> + return NULL;
> + }
> + else
> + {
> + apr_err = apr_thread_mutex_lock(array_mutex);
> + if (apr_err != APR_SUCCESS)
> + return NULL;
> + mutex = *((apr_thread_mutex_t**)apr_array_pop(free_mutexes));
> + apr_err = apr_thread_mutex_unlock(array_mutex);
> + if (apr_err != APR_SUCCESS)
> + return NULL;
> + }
> + return mutex;
> +}
>
> You can't access the array without locking the mutex. The lock needs
> to be pulled up outside the if statement, before you call
> apr_is_empty_array, otherwise none of it is thread safe. Basically
> you need to lock the mutex before ANY access to that array.
Oops. Thanks for pointing that out.
I attached the new patch.
--
Vlad
Re: [PATCH] Client-side Cyrus SASL support
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 8/8/06, Vlad Georgescu <vg...@gmail.com> wrote:
> I modified this patch to use svn_atomic_init_once, and also made a
> couple of other changes.
>
> [[[
> Add client-side support for Cyrus SASL.
>
> * configure.in: Define SVN_HAVE_SASL.
>
> * subversion/libsvn_ra_svn/client.c
> (svn_ra_svn__init): Call svn_ra_svn__sasl_init.
>
> * subversion/libsvn_ra_svn/ra_svn_sasl.h: New file.
>
> * subversion/libsvn_ra_svn/sasl_auth.c: New file.
>
> * subversion/libsvn_ra_svn/simple_auth.c:
> Enclose content within an #ifndef SVN_HAVE_SASL .. #endif pair.
>
> * subversion/libsvn_ra_svn/ra_svn.h
> (SVN_RA_SVN__READBUF_SIZE,
> SVN_RA_SVN__WRITEBUF_SIZE): New defines.
> (svn_ra_svn_conn_st): Use them here.
> (svn_ra_svn__sasl_init): New declaration.
> ]]]
A few more problems:
+/* Define sane defaults for a sasl_security_properties_t structure.
+ See sasl.h for details. SASL needs to know our read buffer's size
+ when negotiating a security layer. */
+#define SVN_RA_SVN__DEFAULT_SECPROPS {0, 256, SVN_RA_SVN__READBUF_SIZE, \
+ 0, NULL, NULL}
Please explain what 256 means in this context. I think it's been
discussed on this list, but it really should make it into the comment.
+static void *sasl_mutex_alloc_cb(void)
+{
+ apr_thread_mutex_t *mutex;
+ apr_status_t apr_err;
+ if (apr_is_empty_array(free_mutexes))
+ {
+ apr_err = apr_thread_mutex_create(&mutex,
+ APR_THREAD_MUTEX_DEFAULT,
+ sasl_pool);
+ if (apr_err != APR_SUCCESS)
+ return NULL;
+ }
+ else
+ {
+ apr_err = apr_thread_mutex_lock(array_mutex);
+ if (apr_err != APR_SUCCESS)
+ return NULL;
+ mutex = *((apr_thread_mutex_t**)apr_array_pop(free_mutexes));
+ apr_err = apr_thread_mutex_unlock(array_mutex);
+ if (apr_err != APR_SUCCESS)
+ return NULL;
+ }
+ return mutex;
+}
You can't access the array without locking the mutex. The lock needs
to be pulled up outside the if statement, before you call
apr_is_empty_array, otherwise none of it is thread safe. Basically
you need to lock the mutex before ANY access to that array.
Other than that it seems fine to me.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org