[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: rev 4574 - in trunk/subversion: include libsvn_auth

From: <gstein_at_lyra.org>
Date: 2003-01-27 13:15:21 CET

On Fri, Jan 24, 2003 at 03:42:21PM -0600, sussman@tigris.org wrote:
...
 +++ trunk/subversion/include/svn_auth.h Fri Jan 24 15:42:11 2003
 @@ -26,6 +26,7 @@
  
  #include svn_types.h
  #include svn_wc.h
 +#include svn_client.h

Auth is a lower system than the client library. Even lower than the wc
library. The auth system should not provide wc-related providers -- the WC
should provide that itself. The prompt thing is okay, but the definition of
a prompt function should be local to the auth library rather than the client
library. You'll notice that libsvn_client makes zero use of the prompt
function, except for a reference in the client auth baton (which is
probably, inherently bogus in the new design).

libsvn_auth should be considered a base library which others build on. I
don't think it should know about WC-related things or really even a whole
lot about other Subversion concepts (e.g. RA or client).

...
 +++ trunk/subversion/libsvn_auth/simple_prompt_provider.c Fri Jan 24 15:42:13 2003
...
 +#include svn_client.h
 +#include svn_wc.h
 +#include svn_auth.h

Why svn_wc.h in this file? I see no use of WC types/functions.

 +/* Bikeshed: how many times we re-try prompting. */
 +#define SVN_AUTH_SIMPLE_PROMPT_RETRY_LIMIT 2

The prompter probably should not define this, but take it as an argument to
the provider-fetch function.

...
 +typedef struct
 +{
 + /* the same callback from the main provider baton */
 + svn_client_prompt_t prompt_func;
 + void *prompt_baton;

Might be simpler to just have a back-pointer to the provider baton.

...
 +{
 + simple_prompt_provider_baton_t *pb
 + = (simple_prompt_provider_baton_t *) provider_baton;

That cast doesn't do anything but make the line harder to read. Since
provider_baton is a 'void *', it is fine to assign it directly to pb.

 + svn_auth_cred_simple_t *creds = apr_pcalloc (pool, sizeof(*creds));
 + simple_prompt_iter_baton_t *ibaton = apr_pcalloc (pool, sizeof(*ibaton));
 + char *username, *password;

Why non-const? We don't intend to change these values, and it just means
that casts are needed later. I'd recommend altering the prototype on the
prompt function, rather than letting it's non-const return value monkey
stuff like this up.

...
 + creds = apr_pcalloc (pool, sizeof(*creds));
 +
 + SVN_ERR (ib-prompt_func (username, username: ,

If the prompt_func type were fixed, you could just do: creds-username.

...
 +/* The provider. */
 +const svn_auth_provider_t simple_prompt_provider =
 + {
 + SVN_AUTH_CRED_SIMPLE, /* username/passwd creds */
 + simple_prompt_first_creds,
 + simple_prompt_next_creds,
 + NULL /* this provider can't save creds. */
 + };

This needs to be 'static const'; otherwise, it appears outside of the
link-unit, thus infecting the global namespace with something that doesn't
start with 'svn'.

 +/* Public API */
 +void
 +svn_auth_get_simple_prompt_provider (const svn_auth_provider_t **provider,
 + void **provider_baton,
 + svn_client_prompt_t prompt_func,
 + void *prompt_baton,
 + const char *default_username,
 + const char *default_password,
 + apr_pool_t *pool)
 +{
 + simple_prompt_provider_baton_t *pb = apr_pcalloc (pool, sizeof(*pb));
 + pb-prompt_func = prompt_func;
 + pb-prompt_baton = prompt_baton;
 + pb-default_username = apr_pstrdup (pool, default_username);
 + pb-default_password = apr_pstrdup (pool, default_password);

You aren't checking whether the defaults are NULL before dup'ing them.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Sat Oct 14 02:19:17 2006

This is an archived mail posted to the Subversion Dev mailing list.