Jeremy Whitlock wrote on Fri, 10 Oct 2008 at 23:37 -0600:
> Hi All,
> I have attached a patch for review that will remove the platform
> visibility from the Subversion auth functions. This will also fix the
> SWIG Python bindings. The only reason I've not committed this is I do
> not have a non-OSX box available yet for testing other systems and I'd
> hate to break Subversion over the weekend. Can someone test this on
> Linux and/or Windows? (If you can, compile both Subversion and the
> swig-py bindings and then run tests on both.) On OSX, all Subversion
> tests pass and the SWIG Python tests pass as well.
>
When the platform-specific providers aren't available, Subversion will
fall back to the basic provider (store the user/pass in plaintext in
~/.subversion). The fallback will be silent (since the tests disable
all interactive prompts, including the 'plaintext passwords' prompt).
Therefore, I suspect your patch may be passing the tests but doesn't use
keychain at all. Partially because your patch #if's away the
declaration of svn_auth_get_keychain_simple_provider()...
More below.
> [[[
> Remove platform visibility for Subversion auth functions.
...
> ]]]
> Index: subversion/libsvn_subr/cmdline.c
> ===================================================================
> --- subversion/libsvn_subr/cmdline.c (revision 33596)
> +++ subversion/libsvn_subr/cmdline.c (working copy)
> @@ -530,10 +530,18 @@
> {
> #ifdef SVN_HAVE_KEYCHAIN_SERVICES
> svn_auth_get_keychain_simple_provider(&provider, pool);
I'm surprised this call compiled, since your patch #if'd away the
declaration of svn_auth_get_keychain_simple_provider().
> - APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
>
> + if (provider)
> + {
> + APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
Nit: over 80 characters.
Also, when I said on IRC that 'individual providers can promise more' -- we
could make the individual providers we declare promise to return non-NULL on
the appropriate platforms.
For example,
@@ svn_auth.h @@
- * @note This function is only available on Mac OS 10.2 and higher.
+ * @note This function returns non-NULL iff the platform is Mac OS ™10.2.
*/
void
svn_auth_get_keychain_simple_provider(svn_auth_provider_object_t **provider,
and then we don't need to touch this code in cmdline.c. (But presumably
the bindings will be happier because the functions are defined on all
platforms.)
...
> Index: subversion/libsvn_subr/win32_crypto.c
> ===================================================================
> --- subversion/libsvn_subr/win32_crypto.c (revision 33596)
> +++ subversion/libsvn_subr/win32_crypto.c (working copy)
> @@ -18,8 +18,6 @@
>
> /* ==================================================================== */
>
> -#if defined(WIN32) && !defined(__MINGW32__)
> -
> /*** Includes. ***/
>
> #include <apr_pools.h>
> @@ -33,8 +31,11 @@
>
> #include "svn_private_config.h"
>
> +#include <apr_base64.h>
> +
> +#if defined(WIN32) && !defined(__MINGW32__)
> +
> #include <wincrypt.h>
> -#include <apr_base64.h>
>
So the #if and its #endif are on opposite side of the section delimiter (^L).
> /*-----------------------------------------------------------------------*/
> /* Windows simple provider, encrypts the password on Win2k and later. */
> @@ -158,18 +159,6 @@
> windows_simple_save_creds
> };
>
> -
> -/* Public API */
> -void
> -svn_auth_get_windows_simple_provider(svn_auth_provider_object_t **provider,
> - apr_pool_t *pool)
> -{
> - svn_auth_provider_object_t *po = apr_pcalloc(pool, sizeof(*po));
> -
> - po->vtable = &windows_simple_provider;
> - *provider = po;
> -}
> -
>
> /*-----------------------------------------------------------------------*/
> /* Windows SSL server trust provider, validates ssl certificate using */
> @@ -281,16 +270,36 @@
> NULL,
> NULL,
> };
> +#endif /* WIN32 */
>
> /* Public API */
> void
> +svn_auth_get_windows_simple_provider(svn_auth_provider_object_t **provider,
> + apr_pool_t *pool)
> +{
> + svn_auth_provider_object_t *po = apr_pcalloc(pool, sizeof(*po));
> +
> +#if defined(WIN32) && !defined(__MINGW32__)
> + po->vtable = &windows_simple_provider;
> +#else
> + po->vtable = NULL;
> +#endif
> +
> + *provider = po;
> +}
> +
What I had in mind (and what other parts of your diff expect) is
{
#if defined(WIN32)
/* body of function */
#else
*provider = NULL;
#endif
}
but your version could work too. Either way, would be nice to
explicitly document it.
> Index: subversion/include/svn_client.h
> ===================================================================
> --- subversion/include/svn_client.h (revision 33596)
> +++ subversion/include/svn_client.h (working copy)
> @@ -160,7 +160,7 @@
> apr_pool_t *pool);
>
>
> -#if (defined(WIN32) && !defined(__MINGW32__)) || defined(DOXYGEN) || defined(CTYPESGEN)
> +#if defined(DOXYGEN) || defined(CTYPESGEN)
> /**
> * Create and return @a *provider, an authentication provider of type @c
> * svn_auth_cred_simple_t that gets/sets information from the user's
> @@ -187,7 +187,7 @@
> void
> svn_client_get_windows_simple_provider(svn_auth_provider_object_t **provider,
> apr_pool_t *pool);
> -#endif /* WIN32 && !__MINGW32__ || DOXYGEN || CTYPESGEN */
> +#endif /* DOXYGEN || CTYPESGEN */
>
This preprocessor condition will not compile the function on *any*
platform. Presumably not what you intended.
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-10-11 10:57:26 CEST