You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Jeremy Whitlock <jc...@gmail.com> on 2008/10/11 05:37:45 UTC
[PATCH] Remove platform visibility from Subversion auth functions (Was: Python bindings build seems broken)
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.
--
Take care,
Jeremy Whitlock
http://www.thoughtspark.org
[[[
Remove platform visibility for Subversion auth functions.
* subversion/libsvn_subr/cmdline.c (svn_cmdline_set_up_auth_baton): Perform NULL
checks when retrieving the available auth providers.
* subversion/libsvn_subr/win32_crypto.c: Make WIN32 platform check affect
visibility to Win32 headers only.
(svn_auth_get_windows_simple_provider,
svn_auth_get_windows_ssl_server_trust_provider): Added WIN32 platform check
within the functions and made public to all platforms.
* subversion/libsvn_subr/macos_keychain.c
(svn_auth_get_keychain_simple_provider,
svn_auth_get_keychain_ssl_client_cert_pw_provider): Added DARWIN platform
check within the functions and made public to all platforms.
* subversion/libsvn_auth_gnome_keyring/gnome_keyring.c
(svn_auth_get_gnome_keyring_simple_provider,
svn_auth_get_gnome_keyring_ssl_client_cert_pw_provider): Added WIN32 and
DARWIN checks within the functions.
* subversion/bindings/javahl/native/SVNClient.cpp (getContext): Perform NULL
checks when retrieving the available auth providers.
* subversion/include/svn_client.h
(svn_client_get_windows_simple_provider): Removed WIN32 platform check and
added check for CTYPESGEN.
* subversion/include/svn_auth.h
(svn_auth_get_windows_simple_provider): Removed WIN32 platform check.
(svn_auth_get_keychain_simple_provider,
svn_auth_get_keychain_ssl_client_cert_pw_provider): Removed DARWIN platform
check.
(svn_auth_gnome_keyring_version, svn_auth_get_gnome_keyring_simple_provider
svn_auth_get_gnome_keyring_ssl_client_cert_pw_provider,
svn_auth_kwallet_version, svn_auth_get_kwallet_simple_provider,
svn_auth_get_kwallet_ssl_client_cert_pw_provider): Removed DARWIN and WIN32
platform checks.
* subversion/libsvn_client/compat_providers.c
(svn_client_get_windows_simple_provider): Removed WIN32 platform check.
* subversion/libsvn_auth_kwallet/kwallet.cpp
(svn_auth_get_kwallet_simple_provider,
svn_auth_get_kwallet_ssl_client_cert_pw_provider): Added WIN32 and
DARWIN platform checks within the 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);
- APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+ if (provider)
+ {
+ APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *)
= provider;
+ }
+
svn_auth_get_keychain_ssl_client_cert_pw_provider(&provider, pool);
- APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+
+ if (provider)
+ {
+ APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *)
= provider;
+ }
#endif
continue;
}
@@ -542,7 +550,11 @@
{
#if defined(WIN32) && !defined(__MINGW32__)
svn_auth_get_windows_simple_provider(&provider, pool);
- APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+
+ if (provider)
+ {
+ APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *)
= provider;
+ }
#endif
continue;
}
@@ -612,7 +624,11 @@
/* The server-cert, client-cert, and client-cert-password providers. */
#if defined(WIN32) && !defined(__MINGW32__)
svn_auth_get_windows_ssl_server_trust_provider(&provider, pool);
- APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+
+ if (provider)
+ {
+ APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+ }
#endif
svn_auth_get_ssl_server_trust_file_provider(&provider, pool);
APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
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>
/*-----------------------------------------------------------------------*/
/* 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;
+}
+
+/* Public API */
+void
svn_auth_get_windows_ssl_server_trust_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_server_trust_provider;
+#else
+ po->vtable = NULL;
+#endif
+
*provider = po;
}
-
-#endif /* WIN32 */
Index: subversion/libsvn_subr/macos_keychain.c
===================================================================
--- subversion/libsvn_subr/macos_keychain.c (revision 33596)
+++ subversion/libsvn_subr/macos_keychain.c (working copy)
@@ -226,6 +226,7 @@
keychain_ssl_client_cert_pw_save_creds
};
+#endif /* SVN_HAVE_KEYCHAIN_SERVICES */
/* Public API */
void
@@ -234,7 +235,12 @@
{
svn_auth_provider_object_t *po = apr_pcalloc(pool, sizeof(*po));
+#if defined(DARWIN)
po->vtable = &keychain_simple_provider;
+#else
+ po->vtable = NULL;
+#endif
+
*provider = po;
}
@@ -245,7 +251,10 @@
{
svn_auth_provider_object_t *po = apr_pcalloc(pool, sizeof(*po));
+#if defined(DARWIN)
po->vtable = &keychain_ssl_client_cert_pw_provider;
+#else
+ po->vtable = NULL;
+#endif
*provider = po;
}
-#endif /* SVN_HAVE_KEYCHAIN_SERVICES */
Index: subversion/libsvn_auth_gnome_keyring/gnome_keyring.c
===================================================================
--- subversion/libsvn_auth_gnome_keyring/gnome_keyring.c (revision 33596)
+++ subversion/libsvn_auth_gnome_keyring/gnome_keyring.c (working copy)
@@ -186,10 +186,17 @@
{
svn_auth_provider_object_t *po = apr_pcalloc(pool, sizeof(*po));
+#if !defined(DARWIN) && !defined(WIN32)
po->vtable = &gnome_keyring_simple_provider;
+#else
+ po->vtable = NULL;
+#endif
+
*provider = po;
+#if !defined(DARWIN) && !defined(WIN32)
gnome_keyring_init();
+#endif
}
@@ -251,6 +258,11 @@
{
svn_auth_provider_object_t *po = apr_pcalloc(pool, sizeof(*po));
+#if !defined(DARWIN) && !defined(WIN32)
po->vtable = &gnome_keyring_ssl_client_cert_pw_provider;
+#else
+ po->vtable = NULL;
+#endif
+
*provider = po;
}
Index: subversion/bindings/javahl/native/SVNClient.cpp
===================================================================
--- subversion/bindings/javahl/native/SVNClient.cpp (revision 33596)
+++ subversion/bindings/javahl/native/SVNClient.cpp (working copy)
@@ -1229,16 +1229,28 @@
/* The main disk-caching auth providers, for both
* 'username/password' creds and 'username' creds. */
svn_auth_provider_object_t *provider;
-#if defined(WIN32) && !defined(__MINGW32__)
+#if defined(WIN32) && !defined(__MINGW32__)
svn_auth_get_windows_simple_provider(&provider, pool);
- APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+
+ if (provider)
+ {
+ APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+ }
#endif
#ifdef SVN_HAVE_KEYCHAIN_SERVICES
svn_auth_get_keychain_simple_provider(&provider, pool);
- APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+ if (provider)
+ {
+ APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+ }
+
svn_auth_get_keychain_ssl_client_cert_pw_provider(&provider, pool);
- APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+
+ if (provider)
+ {
+ APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+ }
#endif
#ifdef SVN_HAVE_GNOME_KEYRING
SVN_JNI_ERR(get_auth_provider(&provider, "gnome_keyring", "simple",
@@ -1276,7 +1288,11 @@
/* The server-cert, client-cert, and client-cert-password providers. */
#if defined(WIN32) && !defined(__MINGW32__)
svn_auth_get_windows_ssl_server_trust_provider(&provider, pool);
- APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+
+ if (provider)
+ {
+ APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
+ }
#endif
svn_auth_get_ssl_server_trust_file_provider(&provider, pool);
APR_ARRAY_PUSH(providers, svn_auth_provider_object_t *) = provider;
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 */
/** Create and return @a *provider, an authentication provider of type @c
* svn_auth_cred_username_t that gets/sets information from a user's
Index: subversion/include/svn_auth.h
===================================================================
--- subversion/include/svn_auth.h (revision 33596)
+++ subversion/include/svn_auth.h (working copy)
@@ -785,7 +785,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
@@ -808,9 +808,9 @@
void
svn_auth_get_windows_simple_provider(svn_auth_provider_object_t **provider,
apr_pool_t *pool);
-#endif /* WIN32 && !__MINGW32__ || DOXYGEN || CTYPESGEN */
+#endif /* DOXYGEN || CTYPESGEN */
-#if defined(DARWIN) || 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
@@ -843,9 +843,9 @@
svn_auth_get_keychain_ssl_client_cert_pw_provider
(svn_auth_provider_object_t **provider,
apr_pool_t *pool);
-#endif /* DARWIN || DOXYGEN || CTYPESGEN */
+#endif /* DOXYGEN || CTYPESGEN */
-#if (!defined(DARWIN) && !defined(WIN32)) || defined(DOXYGEN)
+#if defined(DOXYGEN) || defined(CTYPESGEN)
/**
* Get libsvn_auth_gnome_keyring version information.
*
@@ -937,7 +937,7 @@
svn_auth_get_kwallet_ssl_client_cert_pw_provider
(svn_auth_provider_object_t **provider,
apr_pool_t *pool);
-#endif /* (!DARWIN && !WIN32) || DOXYGEN */
+#endif /* DOXYGEN || CTYPESGEN */
/** Create and return @a *provider, an authentication provider of type @c
Index: subversion/libsvn_client/compat_providers.c
===================================================================
--- subversion/libsvn_client/compat_providers.c (revision 33596)
+++ subversion/libsvn_client/compat_providers.c (working copy)
@@ -57,14 +57,12 @@
svn_auth_get_simple_provider2(provider, NULL, NULL, pool);
}
-#if defined(WIN32) && !defined(__MINGW32__)
void
svn_client_get_windows_simple_provider(svn_auth_provider_object_t **provider,
apr_pool_t *pool)
{
svn_auth_get_windows_simple_provider(provider, pool);
}
-#endif /* WIN32 */
void svn_client_get_username_provider(svn_auth_provider_object_t **provider,
apr_pool_t *pool)
Index: subversion/libsvn_auth_kwallet/kwallet.cpp
===================================================================
--- subversion/libsvn_auth_kwallet/kwallet.cpp (revision 33596)
+++ subversion/libsvn_auth_kwallet/kwallet.cpp (working copy)
@@ -222,7 +222,12 @@
svn_auth_provider_object_t *po =
static_cast<svn_auth_provider_object_t *> (apr_pcalloc(pool, sizeof(*po)));
+#if !defined(DARWIN) && !defined(WIN32)
po->vtable = &kwallet_simple_provider;
+#else
+ po->vtable = NULL;
+#endif
+
*provider = po;
}
}
@@ -287,6 +292,12 @@
svn_auth_provider_object_t *po =
static_cast<svn_auth_provider_object_t *> (apr_pcalloc(pool, sizeof(*po)));
+
+#if !defined(DARWIN) && !defined(WIN32)
po->vtable = &kwallet_ssl_client_cert_pw_provider;
+#else
+ po->vtable = NULL;
+#endif
+
*provider = po;
}
Re: [PATCH] Remove platform visibility from Subversion auth functions (Was: Python bindings build seems broken)
Posted by Jeremy Whitlock <jc...@gmail.com>.
>> Everything looks sound. It compiles because all of the stuff that is
>> public to all systems only reference structures and functions defined
>> in the Subversion headers.
>
> That's the point. The declarations of svn_auth_*keychain* were excluded
> by the (accidental) line:
>
> -#if defined(DARWIN) || defined(DOXYGEN) || defined(CTYPESGEN)
> +#if defined(DOXYGEN) || defined(CTYPESGEN)
>
> Hence my puzzlement.
Understood.
>> Yeah. I think I was being a little too paranoid. I was just saying
>> that if a NULL can be returned, should we check for it and the answer
>> is no if we know 100% that it can't be NULL and in our case, we can.
>> NULL only is returned if you call a function for another platform and
>> that can't happen in cmdline.c since we conditionally include calls to
>> those functions.
>>
>
> Yes.
Great. I have what I need to fix this up. I also have a box with
kwallet and gnome-keyring now so I should be able to make the
necessary changes and test against all but Windows. If I get
everything looking good with the next iteration, I'll commit. At
least the approach was sound and that was what I was looking for.
Thank you so much for working with me on this.
--
Take care,
Jeremy Whitlock
http://www.thoughtspark.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Remove platform visibility from Subversion auth functions
(Was: Python bindings build seems broken)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Jeremy Whitlock wrote on Sat, 11 Oct 2008 at 08:53 -0600:
> > Good to hear that. (I still don't understand how it could have worked,
> > though.)
>
> Everything looks sound. It compiles because all of the stuff that is
> public to all systems only reference structures and functions defined
> in the Subversion headers.
That's the point. The declarations of svn_auth_*keychain* were excluded
by the (accidental) line:
-#if defined(DARWIN) || defined(DOXYGEN) || defined(CTYPESGEN)
+#if defined(DOXYGEN) || defined(CTYPESGEN)
Hence my puzzlement.
> At runtime, it works because from a functionality standpoint, nothing
> changed. For the OSX stuff, the only thing that changed really was
> that we now have NULL checks in cmdline.c, which will remove since
> we've made the promise to return non-NULL on OSX systems. Does this
> help explain?
>
Agreed on the runtime. At compile time I would have expected a warning,
at least. But I'm not too worried about it; just fix it in the next
iteration.
> > I don't understand the question. If we make a promise in the API, then by
> > definition callers don't have to check for it (or, callers that *do* check
> > for it (for paranoia or debugging reasons) should use SVN_ERR_ASSERT or
> > SVN_ERR_MALFUNCTION.) Does that answer your question?
>
> Yeah. I think I was being a little too paranoid. I was just saying
> that if a NULL can be returned, should we check for it and the answer
> is no if we know 100% that it can't be NULL and in our case, we can.
> NULL only is returned if you call a function for another platform and
> that can't happen in cmdline.c since we conditionally include calls to
> those functions.
>
Yes.
Daniel
> --
> Take care,
>
> Jeremy Whitlock
> http://www.thoughtspark.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Remove platform visibility from Subversion auth functions (Was: Python bindings build seems broken)
Posted by Jeremy Whitlock <jc...@gmail.com>.
> Good to hear that. (I still don't understand how it could have worked,
> though.)
Everything looks sound. It compiles because all of the stuff that is
public to all systems only reference structures and functions defined
in the Subversion headers. At runtime, it works because from a
functionality standpoint, nothing changed. For the OSX stuff, the
only thing that changed really was that we now have NULL checks in
cmdline.c, which will remove since we've made the promise to return
non-NULL on OSX systems. Does this help explain?
>> > 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.)
>>
>> Better documentation makes sense. Are you sure with the "promise" to
>> return non-NULL is enough? It just seemed like a good idea although
>> the only feasible way to get a NULL was on an unsupported platform.
>>
>
> I don't understand the question. If we make a promise in the API, then by
> definition callers don't have to check for it (or, callers that *do* check
> for it (for paranoia or debugging reasons) should use SVN_ERR_ASSERT or
> SVN_ERR_MALFUNCTION.) Does that answer your question?
Yeah. I think I was being a little too paranoid. I was just saying
that if a NULL can be returned, should we check for it and the answer
is no if we know 100% that it can't be NULL and in our case, we can.
NULL only is returned if you call a function for another platform and
that can't happen in cmdline.c since we conditionally include calls to
those functions.
--
Take care,
Jeremy Whitlock
http://www.thoughtspark.org
Re: [PATCH] Remove platform visibility from Subversion auth functions
(Was: Python bindings build seems broken)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Jeremy Whitlock wrote on Sat, 11 Oct 2008 at 04:20 -0600:
> > 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.
>
> I tested the Keychain stuff and it worked as expected. Whenever I
> used the trunk build, with patch, to access something with cached
> credentials stored in the Keychain, the Keychain was accessed. (On
> OSX, I get a prompt asking if it's okay to use the new version of
> Subversion to access the keychain since the cached credentials were
> stored by an older version of Subversion.) Seems to be working fine
> on OSX.
Good to hear that. (I still don't understand how it could have worked,
though.)
> > 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.)
>
> Better documentation makes sense. Are you sure with the "promise" to
> return non-NULL is enough? It just seemed like a good idea although
> the only feasible way to get a NULL was on an unsupported platform.
>
I don't understand the question. If we make a promise in the API, then by
definition callers don't have to check for it (or, callers that *do* check
for it (for paranoia or debugging reasons) should use SVN_ERR_ASSERT or
SVN_ERR_MALFUNCTION.) Does that answer your question?
Thanks,
Daniel
Re: [PATCH] Remove platform visibility from Subversion auth functions (Was: Python bindings build seems broken)
Posted by Jeremy Whitlock <jc...@gmail.com>.
> 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.
I tested the Keychain stuff and it worked as expected. Whenever I
used the trunk build, with patch, to access something with cached
credentials stored in the Keychain, the Keychain was accessed. (On
OSX, I get a prompt asking if it's okay to use the new version of
Subversion to access the keychain since the cached credentials were
stored by an older version of Subversion.) Seems to be working fine
on OSX.
>
>> [[[
>> 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.
Sorry about that. Somehow missed that one.
> 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.)
Better documentation makes sense. Are you sure with the "promise" to
return non-NULL is enough? It just seemed like a good idea although
the only feasible way to get a NULL was on an unsupported platform.
> ...
>> 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).
I can fix that.
>> /*-----------------------------------------------------------------------*/
>> /* 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.
Yeah. Better documentation here will help. A for who's way is ideal,
I like yours better. It's a little easier to identify what you're
trying to do.
>> 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.
Yeah, that wasn't intended.
Thanks for looking into this. I'll get these fixed and ready for commit.
--
Take care,
Jeremy Whitlock
http://www.thoughtspark.org
Re: [PATCH] Remove platform visibility from Subversion auth functions
(Was: Python bindings build seems broken)
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
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
Re: [PATCH] Remove platform visibility from Subversion auth functions (Was: Python bindings build seems broken)
Posted by Jeremy Whitlock <jc...@gmail.com>.
After discussion with a few guys in #svn-dev, I think this approach is
flawed. Please see my response to the original thread for further
details and please ignore this patch.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org