Branko Čibej <brane_at_wandisco.com> writes:
> error stacks look sane. But, before committing the change, I'd like
> someone else to review the patch because it's just a wee bit tricky in
> places what with all the #ifdefs.
Looks OK, a few minor things:
> Index: subversion/libsvn_subr/error.c
> ===================================================================
> --- subversion/libsvn_subr/error.c (revision 1663941)
> +++ subversion/libsvn_subr/error.c (working copy)
> @@ -28,6 +28,10 @@
> #include <apr_pools.h>
> #include <apr_strings.h>
>
> +#if defined(SVN_DEBUG) && APR_HAS_THREADS
> +#include <apr_thread_proc.h>
> +#endif
Why not an unconditional include?
> +
> #include <zlib.h>
>
> #ifndef SVN_ERR__TRACING
> @@ -38,12 +42,55 @@
> #include "svn_pools.h"
> #include "svn_utf.h"
>
> +#include "private/svn_error_private.h"
> +#include "svn_private_config.h"
> +
> +#if defined(SVN_DEBUG) && APR_HAS_THREADS
> +#include "private/svn_atomic.h"
> +#include "pools.h"
> +#endif
Again, why not unconditional?
> +
> +
> #ifdef SVN_DEBUG
> -/* XXX FIXME: These should be protected by a thread mutex.
> - svn_error__locate and make_error_internal should cooperate
> - in locking and unlocking it. */
> +# if APR_HAS_THREADS
> +static apr_threadkey_t *error_file_key = NULL;
> +static apr_threadkey_t *error_line_key = NULL;
>
> -/* XXX TODO: Define mutex here #if APR_HAS_THREADS */
> +/* No-op destructor for apr_threadkey_private_create(). */
> +static void null_threadkey_dtor(void *stuff) {}
> +
> +/* Init-once function for the thread-local keys.
> + This function will never return an error. */
It is easier for the reader if "svn_atomic__init_once" is explict rather
than implied by "Init-once". If svn_atomic__init_once used a typedef
for the callback that would be even better, but that is a separate
change.
> +static svn_error_t *
> +locate_init_once(void *ignored_baton, apr_pool_t *ignored_pool)
> +{
> + /* Strictly speaking, this is a memory leak, since we're creating an
> + unmanaged, top-level pool and never destroying it. We do this
> + because this pool controls the lifetime of the thread-local
> + storage for error locations, and that storage must always be
> + available. */
> + apr_pool_t *threadkey_pool = svn_pool__create_unmanaged(TRUE);
> + apr_status_t status;
> +
> + status = apr_threadkey_private_create(&error_file_key,
> + null_threadkey_dtor,
--
Philip Martin | Subversion Committer
WANdisco // *Non-Stop Data*
Received on 2015-03-04 13:23:32 CET