You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2006/01/05 13:50:00 UTC
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
Re: svn commit: r17984 - trunk/subversion/libsvn_ra_dav
Posted by kf...@collab.net.
Garrett Rooney <ro...@electricjellyfish.net> writes:
> > 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?
>
> NULL seems to indicate that we didn't hit a copy (for the "method"
> field), or that we didn't hit an error (for the other two). I'll try
> and come up with a useful description and add it.
Thanks. It's mainly the first one that wasn't immediately obvious.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r17984 - trunk/subversion/libsvn_ra_dav
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 05 Jan 2006 07:50:00 -0600, kfogel@collab.net <kf...@collab.net> wrote:
> 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?
NULL seems to indicate that we didn't hit a copy (for the "method"
field), or that we didn't hit an error (for the other two). I'll try
and come up with a useful description and add it.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org