On Wed, Mar 4, 2009 at 23:45, Lieven Govaerts <lgo_at_mobsol.be> wrote:
>...
> +++ trunk/subversion/libsvn_ra_serf/auth.c Wed Mar 4 14:45:12 2009 (r36331)
>...
> +/**
> + * Baton passed to the response header callback function
> + */
> +typedef struct {
> + int code;
> + const char *header;
> + svn_ra_serf__handler_t *ctx;
> + serf_request_t *request;
> + serf_bucket_t *response;
> + svn_error_t *err;
> + apr_pool_t *pool;
> + const svn_ra_serf__auth_protocol_t *prot;
> + char *last_prot_name;
> +} auth_baton_t;
last_prot_name should be const.
> +
> +/**
> + * handle_auth_header is called for each header in the response. It filters
> + * out the Authenticate headers (WWW or Proxy depending on what's needed) and
> + * tries to find a matching protocol handler.
> + *
> + * Returns a non-0 value of a matching handler was found.
> + */
> +static int
> +handle_auth_header(void *baton,
> + const char *key,
> + const char *header)
> +{
> + auth_baton_t *ab = (auth_baton_t *)baton;
No need for a cast.
> + svn_ra_serf__session_t *session = ab->ctx->session;
> + svn_ra_serf__connection_t *conn = ab->ctx->conn;
> + svn_boolean_t proto_found = FALSE;
> + char *auth_name = NULL, *auth_attr = NULL;
> + const char *auth_hdr = NULL;
> + const svn_ra_serf__auth_protocol_t *prot = NULL;
> +
> + /* We're only interested in xxxx-Authenticate headers. */
> + if (ab->code == 401)
> + auth_hdr = "WWW-Authenticate";
> + else if (ab->code == 407)
> + auth_hdr = "Proxy-Authenticate";
No need for this. ab->header is set properly.
>...
> svn_ra_serf__handle_auth(int code,
> - svn_ra_serf__session_t *session,
> - svn_ra_serf__connection_t *conn,
> + svn_ra_serf__handler_t *ctx,
> serf_request_t *request,
> serf_bucket_t *response,
> apr_pool_t *pool)
> {
> + svn_ra_serf__session_t *session = ctx->session;
> serf_bucket_t *hdrs;
> - const svn_ra_serf__auth_protocol_t *prot = NULL;
> - char *auth_name = NULL, *auth_attr, *auth_hdr=NULL, *header, *header_attr;
> - svn_error_t *cached_err = SVN_NO_ERROR;
> + auth_baton_t *ab;
> + char *auth_hdr = NULL;
> +
> + ab = apr_pcalloc(pool, sizeof(*ab));
No need to put the baton on the heap. Just make it a local variable.
If you mark auth_hdr as const, then you won't have to cast away const
later in this function.
> + ab->code = code;
> + ab->request = request;
> + ab->response = response;
> + ab->ctx = ctx;
> + ab->err = SVN_NO_ERROR;
> + ab->pool = pool;
That pool is not used. The scanning function used the session pool
instead. Which is the correct pool? If ab->pool, then use it. If
session->pool, then remove this one.
> hdrs = serf_bucket_response_get_headers(response);
> +
> if (code == 401)
> - auth_hdr = (char*)serf_bucket_headers_get(hdrs, "WWW-Authenticate");
> + ab->header = "WWW-Authenticate";
> else if (code == 407)
> - auth_hdr = (char*)serf_bucket_headers_get(hdrs, "Proxy-Authenticate");
> + ab->header = "Proxy-Authenticate";
> +
> + /* Before iterating over all authn headers, check if there are any. */
> + auth_hdr = (char*)serf_bucket_headers_get(hdrs, ab->header);
Like here.
>...
> +++ trunk/subversion/libsvn_ra_serf/auth_digest.h Wed Mar 4 14:45:12 2009 (r36331, copy of r36330, branches/ra_serf-digest-authn/subversion/libsvn_ra_serf/auth_digest.h)
>...
> +/* Stores the context information related to Digest authentication.
> + The context is per connection. */
> +typedef struct
> +{
> + /* nonce-count for digest authentication */
> + unsigned int digest_nc;
> +
> + char *ha1;
> +
> + char *realm;
> + char *cnonce;
> + char *nonce;
> + char *opaque;
> + char *algorithm;
> + char *qop;
> + char *username;
> +
> + apr_pool_t *pool;
> +} serf_digest_context_t;
Let's see some "const" on those fields (assuming they can be). This
structure also impinges on serf's namespace: it should be put into
serf, or renamed with an svn_ prefix.
> +
> +/* Digest implementation of an ra_serf authentication protocol providor.
> + handle_digest_auth prepares the authentication headers for a new request
> + based on the response of the server. */
> +svn_error_t *
> +handle_digest_auth(svn_ra_serf__handler_t *ctx,
> + serf_request_t *request,
> + serf_bucket_t *response,
> + char *auth_hdr,
> + char *auth_attr,
> + apr_pool_t *pool);
Can const be applied to some/most of those parameters?
>...
> +++ trunk/subversion/libsvn_ra_serf/win32_auth_sspi.c Wed Mar 4 14:45:12 2009 (r36331)
> @@ -117,18 +117,20 @@ init_sspi_connection(svn_ra_serf__sessio
> {
> const char *tmp;
> apr_size_t tmp_len;
> + serf_sspi_context_t *sspi_context;
>
> SVN_ERR(svn_atomic__init_once(&sspi_initialized, initialize_sspi, pool));
>
> - conn->sspi_context = (serf_sspi_context_t*)
> - apr_palloc(pool, sizeof(serf_sspi_context_t));
> - conn->sspi_context->ctx.dwLower = 0;
> - conn->sspi_context->ctx.dwUpper = 0;
> - conn->sspi_context->state = sspi_auth_not_started;
> + sspi_context =
> + (serf_sspi_context_t *)apr_palloc(pool, sizeof(serf_sspi_context_t));
> + sspi_context->ctx.dwLower = 0;
> + sspi_context->ctx.dwUpper = 0;
> + sspi_context->state = sspi_auth_not_started;
> + conn->auth_context = (void*)sspi_context;
No need for casts.
>...
> @@ -152,6 +153,7 @@ handle_sspi_auth(svn_ra_serf__session_t
> char *base64_token, *token = NULL, *last;
> apr_size_t tmp_len, token_len = 0;
>
> + serf_sspi_context_t *sspi_context = (serf_sspi_context_t *)conn->auth_context;
No need for a cast.
>...
> @@ -299,18 +301,20 @@ init_proxy_sspi_connection(svn_ra_serf__
> {
> const char *tmp;
> apr_size_t tmp_len;
> + serf_sspi_context_t *sspi_context;
>
> SVN_ERR(svn_atomic__init_once(&sspi_initialized, initialize_sspi, pool));
>
> - conn->proxy_sspi_context = (serf_sspi_context_t*)
> - apr_palloc(pool, sizeof(serf_sspi_context_t));
> - conn->proxy_sspi_context->ctx.dwLower = 0;
> - conn->proxy_sspi_context->ctx.dwUpper = 0;
> - conn->proxy_sspi_context->state = sspi_auth_not_started;
> + sspi_context =
> + (serf_sspi_context_t *)apr_palloc(pool, sizeof(serf_sspi_context_t));
> + sspi_context->ctx.dwLower = 0;
> + sspi_context->ctx.dwUpper = 0;
> + sspi_context->state = sspi_auth_not_started;
> + conn->proxy_auth_context = (void*)sspi_context;
No need for casts.
>
> /* Setup the initial request to the server with an SSPI header */
> SVN_ERR(sspi_get_credentials(NULL, 0, &tmp, &tmp_len,
> - conn->proxy_sspi_context));
> + sspi_context));
> svn_ra_serf__encode_auth_header("NTLM", &conn->proxy_auth_value, tmp,
> tmp_len,
> pool);
> @@ -334,6 +338,7 @@ handle_proxy_sspi_auth(svn_ra_serf__sess
> const char *tmp;
> char *base64_token, *token = NULL, *last;
> apr_size_t tmp_len, token_len = 0;
> + serf_sspi_context_t *sspi_context;
>
> base64_token = apr_strtok(auth_attr, " ", &last);
> if (base64_token)
> @@ -346,11 +351,13 @@ handle_proxy_sspi_auth(svn_ra_serf__sess
> /* We can get a whole batch of 401 responses from the server, but we should
> only start the authentication phase once, so if we started authentication
> ignore all responses with initial NTLM authentication header. */
> - if (!token && conn->proxy_sspi_context->state != sspi_auth_not_started)
> + sspi_context = (serf_sspi_context_t *)conn->proxy_auth_context;
No need for a cast.
>...
Cheers,
-g
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=1273070
Received on 2009-03-05 18:48:14 CET