Re: svn commit: r17984 - trunk/subversion/libsvn_ra_dav

rooneg@tigris.org writes:
> Log:
> Allow DAV sessions to be reused without building up unlimited numbers
> of Neon hook callbacks that refer to potentially bogus memory. This
> fixes the svnsync tests over DAV with pool debugging turned on.
>
> * subversion/libsvn_ra_dav/ra_dav.h
> (svn_ra_dav__session_t): Add the copy baton.
>
> * subversion/libsvn_ra_dav/commit.c
> (svn_ra_dav__get_commit_editor): Only set up the hooks the first
> time through, move the copy baton into the session so it can persist
> there.
>
> --- trunk/subversion/libsvn_ra_dav/commit.c (original)
> +++ trunk/subversion/libsvn_ra_dav/commit.c Wed Jan 4 18:53:04 2006
> @@ -1580,11 +1580,22 @@
> svn_ra_dav__session_t *ras = session->priv;
> svn_delta_editor_t *commit_editor;
> commit_ctx_t *cc;
> - struct copy_baton *cb;
>
> - /* Build a copy_baton for COPY requests. */
> - cb = apr_pcalloc(pool, sizeof(*cb));
> - cb->pool = pool;
> + /* Only initialize the baton the first time through. */
> + if (! ras->cb)
> + {
> + /* Build a copy_baton for COPY requests. */
> + ras->cb = apr_pcalloc(ras->pool, sizeof(*ras->cb));
> +
> + /* Register request hooks in the neon session. They specifically
> + allow any COPY requests (ne_copy()) to parse <D:error>
> + responses. They're no-ops for other requests. */
> + ne_hook_create_request(ras->sess, create_request_hook, ras->cb);
> + ne_hook_pre_send(ras->sess, pre_send_hook, ras->cb);
> + }
> +
> + /* Make sure the baton uses our current pool, so we don't leak. */
> + ras->cb->pool = pool;

This issue wasn't really introduced by this commit, I just happened to
notice it as a result of reviewing this commit:

The documentation for 'struct copy_baton' doesn't really talk about
what its fields mean when they're NULL. Right now, it says:

   /* Context for parsing <D:error> bodies, used when we call ne_copy(). */
   struct copy_baton
   {
     /* The method neon is about to execute. */
     const char *method;
     
     /* A parser for handling <D:error> responses from mod_dav_svn. */
     ne_xml_parser *error_parser;
   
     /* If <D:error> is returned, here's where the parsed result goes. */
     svn_error_t *err;
   
     /* A place for allocating fields in this structure. */
     apr_pool_t *pool;
   };

But when the baton is initialized above, the first three fields will
be NULL. What would be a good description of the semantics of that?

> /* Build the main commit editor's baton. */
> cc = apr_pcalloc(pool, sizeof(*cc));
> @@ -1598,7 +1609,7 @@
> cc->callback_baton = callback_baton;
> cc->tokens = lock_tokens;
> cc->keep_locks = keep_locks;
> - cc->cb = cb;
> + cc->cb = ras->cb;
>
> /* If the caller didn't give us any way of storing wcprops, then
> there's no point in getting back a MERGE response full of VR's. */
> @@ -1620,12 +1631,6 @@
> ** log message onto the thing.
> */
> SVN_ERR( apply_log_message(cc, log_msg, pool) );
> -
> - /* Register request hooks in the neon session. They specifically
> - allow any COPY requests (ne_copy()) to parse <D:error>
> - responses. They're no-ops for other requests. */
> - ne_hook_create_request(ras->sess, create_request_hook, cb);
> - ne_hook_pre_send(ras->sess, pre_send_hook, cb);
>
> /*
> ** Set up the editor.
>
> Modified: trunk/subversion/libsvn_ra_dav/ra_dav.h
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_ra_dav/ra_dav.h?rev=17984&p1=trunk/subversion/libsvn_ra_dav/ra_dav.h&p2=trunk/subversion/libsvn_ra_dav/ra_dav.h&r1=17983&r2=17984
> ==============================================================================
> --- trunk/subversion/libsvn_ra_dav/ra_dav.h (original)
> +++ trunk/subversion/libsvn_ra_dav/ra_dav.h Wed Jan 4 18:53:04 2006
> @@ -173,6 +173,8 @@
>
> struct lock_request_baton *lrb; /* used by lock/unlock */
>
> + struct copy_baton *cb; /* used by COPY */
> +
> } svn_ra_dav__session_t;

No other comments -- the rest of the patch looked exactly as one would
expect from the log message :-).

Thanks,
-Karl

-- 
www.collab.net  <>  CollabNet  |  Distributed Development On Demand
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org