On 6/15/06, Vlad Georgescu <vgeorgescu@gmail.com> wrote:
> I created two new files, trivial_auth.c and sasl_auth.c, both of which
> export svn_ra_svn__auth. The contents of these files are surrounded
> with #ifdefs, so only one of them will be compiled, depending on
> whether or not SVN_HAVE_SASL is defined.
>
> I haven't done anything on the server side yet, so all this does right
> now is authenticate using the mechanisms that svnserve currently
> supports (ANONYMOUS, EXTERNAL, CRAM-MD5). In theory, all the other
> mechanisms should automagically work when I add full SASL support to
> svnserve.
>
> Please test/review this and report any problems.
First off, nice work. This looks like you're making some great
progress towards integrating SASL into our system.
A few comments based on a quick read of the diff:
svn_ra_svn__auth isn't a very meaningful name. Maybe svn_ra_svn__handle_auth?
Your initialization of sasl is scoped to a pool, but I'm not sure
that's correct. The sasl lib appears to use global data, and thus
should be initialized once at startup, and once at shutdown. There
should be a static variable that indicates if we've already
initialized sasl (assuming that it's not just ok to call its init
function repeatadly), and sasl_done() should be called from an atexit
handler, not a pool cleanup handler. Consider the case where multiple
threads are executing code that uses the auth implementation? You
don't want to shut down sasl while one is still running. Of course,
this assumes that sasl doesn't already take care of calling its init
and shutdown functions multiple times...
Your patch introduces numerous lines that are longer than 80 columns,
those should be broken at 80 columns, as specified in hacking.html.
There are also several places where your indenting is off:
+ do
+ {
+ result = sasl_client_start(sess->conn->sasl_conn,
+ mechstring,
+ &client_interact,
+ &out,
+ &outlen,
+ &mech);
+
+ /* Fill in username and password, if required */
+ if (result == SASL_INTERACT)
+ SVN_ERR(handle_interact(iterstate_p, sess, client_interact,
+ realmstring, pool));
+ }
+ while (result == SASL_INTERACT);
That should be:
do
{
/* Stuff here... */
}
while (result == SASL_INTERACT);
Similarly:
+ if (try_more_creds)
+ {
+ sasl_dispose(&sess->conn->sasl_conn);
+ /* unregister the cleanup callback */
+ apr_pool_cleanup_kill(pool, &sess->conn->sasl_conn,
+ &sasl_dispose_wrapper);
+ /* create a new context */
+ SVN_ERR(svn_ra_svn__sasl_new(sess, pool));
+ }
Should be:
if (try_more_creds)
{
/* Stuff here... */
}
Sorry I haven't had a chance to test this, I don't actually have the
sasl libs installed on this machine yet...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Jun 15 14:58:38 2006