Here are my notes for David Waite, after reviewing his certs branch
for the Nth time. I'm about to merge the branch into /trunk, because
it looks really good. :-)
Various cleanups done by me:
* Please read HACKING. There are many project formatting conventions
you're not following, which I've had to clean up:
* In almost every C file in Subversion, we use the GNU function
style: foo (blah), not foo(blah). Our rule is: keep the style
of the C file consistent. This means I reformatted your new
routines almost everywhere, because they weren't using
GNU-style. (The exception was libsvn_ra_dav/session.c, which
consistently uses the latter style.)
* When a function returns svn_error_t *, return SVN_NO_ERROR,
rather than NULL. We don't want to confuse semantics.
* We write ISO C code, not C++ or C99 code. I got flat-out
compile errors at first, because in libsvn_ra_dav/session.c, you
were declaring variables in the middle of blocks all over the
place!
* We declare pointers like 'svn_foo_t *foo', not 'svn_foo_t*
foo'. Your variable/function declarations are inconsistent;
you seem to alternate back and forth between the two styles.
Sorry to nitpick, but the svn community prides itself on having very
neat, consistent, easy-to-read code. :-)
* svn_auth.h was #including ne_session.h. That's not good; remember
that we only want libsvn_ra_dav to depend on neon, since the
compilation of that "pair" of libraries is optional. I think this
was simply vestigial; I removed the #include, and everything
compiled fine.
* I removed the useless "#ifndef NO_SSL" stuff from main.c. We can
register all the cert providers uncondititonally. If ra_dav is
present, or if neon doesn't have SSL support, then the providers
will simply never be called. Also, I moved your prompt-provider
creation within the 'non interactive' block; prompt providers aren't
registered if the user passes --non-interactive.
Things you still need to fix:
* The runtime parameter SVN_AUTH_PARAM_SSL_SERVER_CERTIFICATE and
SSL_SERVER_DNAME are still defined in svn_auth.h. None of your
providers use them. And yet your neon callback is still putting the
server cert in the parameter hash! You should remove this unused
stuff.
* Your file-based providers don't implement next_creds() functions,
which is totally fine. But I think it might be courteous to make
their first_creds() routines at least return a NULL iter_baton
anyway. That's what we do for the wc-cache providers.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Mar 20 20:27:38 2003