"Bert Huijben" <B.Huijben_at_competence.biz> writes:
> Some users think SharpSvn is a nice api to make parallel requests to
> subversion servers. I received some crash reports lately and this patch
> resolves at least the crash I could reproduce in a testcase.
>
> This patch fixes a race condition if two neon requests are started
> approximately at the same time. The first call starts initializing and
> the second call thinks initialization is already completed.
This patch looks good to me (I'm running 'make check' on it right now).
But I have one minor question:
> --- subversion/libsvn_ra_neon/session.c (revision 32490)
> +++ subversion/libsvn_ra_neon/session.c (working copy)
> @@ -962,6 +965,25 @@
> }
>
> static svn_error_t *
> +initialize_neon(apr_pool_t* pool)
> +{
> + /* pool = NULL */
> + if (ne_sock_init() != 0)
> + return svn_error_create(SVN_ERR_RA_DAV_SOCK_INIT, NULL,
> + _("Network socket initialization failed"));
> +
> + return SVN_NO_ERROR;
> +}
Why the comment "/* pool = NULL */" ? Wouldn't it be more accurate to
say that pool is ignored? It doesn't matter whether it's NULL or not;
we're not using the value anyway...
In fact, it might be best just to name the parameter itself
'ignored_pool' or something.
> +/* Initializes neon when not initialized before */
> +static svn_error_t *
> +ensure_neon_initialized(void)
> +{
> + SVN_ERR(svn_atomic__init_once(&neon_initialized, initialize_neon, NULL));
> + return SVN_NO_ERROR;
> +}
Here's where a comment about "/* pool = NULL */" might be appropriate;
or a comment that the pool will be unused, or something like that :-).
Aside from that clarity issue, though, this looks fine to me.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-08-15 19:31:50 CEST