You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by "Peter N. Lundblad" <pe...@famlundblad.se> on 2006/01/27 08:42:31 UTC
Re: svn commit: r18260 - trunk/subversion/libsvn_ra_serf
On Fri, 27 Jan 2006 jerenkrantz@tigris.org wrote:
> Author: jerenkrantz
> Date: Fri Jan 27 00:10:02 2006
> New Revision: 18260
>
> Modified:
> trunk/subversion/libsvn_ra_serf/serf.c
>
> Log:
> ra_serf: Perform the REPORT request for update, but do not parse the correctly
> returned response yet.
>
> This code is reaching the point where there are obvious opportunities to
> refactor, optimize, and move code around, but we're not ready to do that until
> we can sucessfully drive a checkout.
>
Yeah, I'll try to avoid making points about that below:-)
> Modified: trunk/subversion/libsvn_ra_serf/serf.c
> Url: http://svn.collab.net/viewcvs/svn/trunk/subversion/libsvn_ra_serf/serf.c?rev=18260&p1=trunk/subversion/libsvn_ra_serf/serf.c&p2=trunk/subversion/libsvn_ra_serf/serf.c&r1=18259&r2=18260
> ==============================================================================
> --- trunk/subversion/libsvn_ra_serf/serf.c (original)
> +++ trunk/subversion/libsvn_ra_serf/serf.c Fri Jan 27 00:10:02 2006
> @@ -27,6 +27,7 @@
> #include "svn_pools.h"
> #include "svn_ra.h"
> #include "svn_dav.h"
> +#include "svn_xml.h"
> #include "../libsvn_ra/ra_loader.h"
> #include "svn_config.h"
> #include "svn_delta.h"
> @@ -136,8 +137,7 @@
> apr_sockaddr_t *address;
>
> serf_sess = apr_pcalloc(pool, sizeof(*serf_sess));
> - /* todo: create a subpool? */
> - serf_sess->pool = pool;
> + apr_pool_create(&serf_sess->pool, pool);
Should be svn_pool_create, but why do you need a subpool?
> +
> +static void add_tag_buckets(serf_bucket_t *agg_bucket, const char *tag,
> + const char *value, serf_bucket_alloc_t *bkt_alloc)
I would use _element_ instead of _tag_ in the name, since it adds a whole
element, not just a tag.
Also, a docstring here (and on the other functions) would be really
valuable, because to me it is not obvious exactly what this function does
and I don't think you should have to read the code to know what a function
does. One can guess what this function does, but does it, for example,
escape the value string with XML entities? Note that we have functions to
create XML tags already in svn_xml.h, but they work on stringbufs, and
maybe it is more efficient to work on serf buckets directly (or some other
good reason) in which case this is fine of course.
> +{
...
> + tmp = SERF_BUCKET_SIMPLE_STRING(value, bkt_alloc);
> + serf_bucket_aggregate_append(agg_bucket, tmp);
> +
OK, it does not escape the element content. Would it be an improvement to
do so? Escaping and encoding bugs are quite common...
> +
> +static svn_error_t *
> +set_path(void *report_baton,
> + const char *path,
> + svn_revnum_t revision,
> + svn_boolean_t start_empty,
> + const char *lock_token,
> + apr_pool_t *pool)
> +{
> + report_context_t *report = report_baton;
> + serf_bucket_t *tmp;
> +
> + tmp = SERF_BUCKET_SIMPLE_STRING_LEN("<S:entry rev=\"",
> + sizeof("<S:entry rev=\"")-1,
> + report->sess->bkt_alloc);
> + serf_bucket_aggregate_append(report->buckets, tmp);
> +
> + tmp = SERF_BUCKET_SIMPLE_STRING(apr_ltoa(pool, revision),
> + report->sess->bkt_alloc);
> + serf_bucket_aggregate_append(report->buckets, tmp);
> +
> + tmp = SERF_BUCKET_SIMPLE_STRING_LEN("\"", sizeof("\"")-1,
> + report->sess->bkt_alloc);
> + serf_bucket_aggregate_append(report->buckets, tmp);
> +
> + if (lock_token)
> + {
> + tmp = SERF_BUCKET_SIMPLE_STRING_LEN(" lock-token=\"",
> + sizeof(" lock-token=\"")-1,
> + report->sess->bkt_alloc);
> + serf_bucket_aggregate_append(report->buckets, tmp);
> +
> + tmp = SERF_BUCKET_SIMPLE_STRING(lock_token,
> + report->sess->bkt_alloc);
Need to escape the lock token, even if it is unlikely that failing to do
so will cause problems just in this case.
> + serf_bucket_aggregate_append(report->buckets, tmp);
> +
> + tmp = SERF_BUCKET_SIMPLE_STRING_LEN("\"", sizeof("\"")-1,
> + report->sess->bkt_alloc);
> + serf_bucket_aggregate_append(report->buckets, tmp);
> + }
> +
> + if (start_empty)
> + {
> + tmp = SERF_BUCKET_SIMPLE_STRING_LEN(" start-empty=\"true\"",
> + sizeof(" start-empty=\"true\"")-1,
> + report->sess->bkt_alloc);
> + serf_bucket_aggregate_append(report->buckets, tmp);
> + }
> +
> + tmp = SERF_BUCKET_SIMPLE_STRING_LEN(">", sizeof(">")-1,
> + report->sess->bkt_alloc);
> + serf_bucket_aggregate_append(report->buckets, tmp);
> +
...
> + tmp = SERF_BUCKET_SIMPLE_STRING(path, report->sess->bkt_alloc);
Here, it's more important.
> + vcc_url = fetch_prop(props, sess->repos_url.path, "DAV:",
> + "version-controlled-configuration");
> +
Wouldn't it be better to have #defines for these string constants, making
it harder to misspell them?
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r18260 - trunk/subversion/libsvn_ra_serf
Posted by Greg Stein <gs...@lyra.org>.
On Fri, Jan 27, 2006 at 10:02:31AM -0800, Garrett Rooney wrote:
> On 1/27/06, Greg Hudson <gh...@mit.edu> wrote:
> > On Fri, 2006-01-27 at 09:24 -0800, Justin Erenkrant
> > > (And, I'm trying to avoid using svn-isms for now.)
>...
> I suspect parts of this code (utility routines for parsing XML, making
> specific types of WebDAV requests, etc) will eventually migrate into
> serf itself, and thus Justin wants to avoid depending on too many
> svn-isms until the boundaries become more well defined.
Yup. serf can/should grow helper functions over its very raw
interface. The dividing line is a bit unknown right now. (this is also
the reason for the direct binding to Expat rather than using svn_xml)
Parsing 207 (Multistatus) responses is a real bitch. No need to make
all clients do that themselves (and get it wrong).
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r18260 - trunk/subversion/libsvn_ra_serf
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/27/06, Greg Hudson <gh...@mit.edu> wrote:
> On Fri, 2006-01-27 at 09:24 -0800, Justin Erenkrant
> > (And, I'm trying to avoid using svn-isms for now.)
>
> Why? I can see using Apache httpd conventions for an httpd module like
> mod_authz_svn (although I don't agree with it), but this is pretty
> squarely Subversion code.
I suspect parts of this code (utility routines for parsing XML, making
specific types of WebDAV requests, etc) will eventually migrate into
serf itself, and thus Justin wants to avoid depending on too many
svn-isms until the boundaries become more well defined.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r18260 - trunk/subversion/libsvn_ra_serf
Posted by Sander Striker <s....@striker.nl>.
Greg Hudson wrote:
> On Fri, 2006-01-27 at 09:24 -0800, Justin Erenkrantz wrote:
>
>>The key for the subpool is to have serf's socket be in a separate pool
>>from the main session pool. Without that (i.e. everything in one
>>pool), the underlying socket gets cleared before the serf cleanup
>>routines are called and it leads to a segfault. Placing serf in its
>>own subpool resolves this problem.
>
>
> Two criticisms:
>
> * APR documents that cleanups are run in reverse order of
> registration, but I don't see any documentation that subpool cleanups
> will be run after pool cleanups.
And they're not. pool cleanups will be run after subpool cleanups.
The first thing that happens on apr_pool_clear() is destruction of
its subpools, only after that the pools cleanups are run.
Aparently this isn't obvious, so I guess I'll need to go and document
that.
> So, unless I missed something, you seem to have resolved this problem
> by relying on undocumented behavior,
> when you could just register your cleanup after the socket is created
> and rely on documented behavior.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r18260 - trunk/subversion/libsvn_ra_serf
Posted by Greg Hudson <gh...@MIT.EDU>.
On Fri, 2006-01-27 at 09:24 -0800, Justin Erenkrantz wrote:
> The key for the subpool is to have serf's socket be in a separate pool
> from the main session pool. Without that (i.e. everything in one
> pool), the underlying socket gets cleared before the serf cleanup
> routines are called and it leads to a segfault. Placing serf in its
> own subpool resolves this problem.
Two criticisms:
* APR documents that cleanups are run in reverse order of
registration, but I don't see any documentation that subpool cleanups
will be run after pool cleanups. So, unless I missed something, you
seem to have resolved this problem by relying on undocumented behavior,
when you could just register your cleanup after the socket is created
and rely on documented behavior.
* Will you eventually need to do anything to the socket besides close
it? If not, why not just rely on the socket's own cleanup and do
nothing? (Speaking of which, setting serf_sess->conn to NULL feels like
rearranging deck chairs on a sinking ship; the sole cleanup operation
for a data structure does not need to worry too much about the field
values of that structure's fields just prior to its being freed.)
> (And, I'm trying to avoid using svn-isms for now.)
Why? I can see using Apache httpd conventions for an httpd module like
mod_authz_svn (although I don't agree with it), but this is pretty
squarely Subversion code.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r18260 - trunk/subversion/libsvn_ra_serf
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Fri, 27 Jan 2006, Justin Erenkrantz wrote:
> On 1/27/06, Greg Stein <gs...@lyra.org> wrote:
> > On Fri, Jan 27, 2006 at 09:42:31AM +0100, Peter N. Lundblad wrote:
> > >...
> > > > - /* todo: create a subpool? */
> > > > - serf_sess->pool = pool;
> > > > + apr_pool_create(&serf_sess->pool, pool);
> > >
> > > Should be svn_pool_create, but why do you need a subpool?
> >
> > Right. Unless it is going to be cleared at some points, then a subpool
> > isn't going to be all that helpful.
>
> The key for the subpool is to have serf's socket be in a separate pool
> from the main session pool. Without that (i.e. everything in one
> pool), the underlying socket gets cleared before the serf cleanup
> routines are called and it leads to a segfault. Placing serf in its
> own subpool resolves this problem.
>
Can you document that in the code. It's not at all obvious.
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r18260 - trunk/subversion/libsvn_ra_serf
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 1/27/06, Greg Stein <gs...@lyra.org> wrote:
> On Fri, Jan 27, 2006 at 09:42:31AM +0100, Peter N. Lundblad wrote:
> >...
> > > - /* todo: create a subpool? */
> > > - serf_sess->pool = pool;
> > > + apr_pool_create(&serf_sess->pool, pool);
> >
> > Should be svn_pool_create, but why do you need a subpool?
>
> Right. Unless it is going to be cleared at some points, then a subpool
> isn't going to be all that helpful.
The key for the subpool is to have serf's socket be in a separate pool
from the main session pool. Without that (i.e. everything in one
pool), the underlying socket gets cleared before the serf cleanup
routines are called and it leads to a segfault. Placing serf in its
own subpool resolves this problem.
> If it gets misspelled, then it simply won't work. The failure mode is
> direct and obvious, so a symbolic conatant isn't going to improve
> things.
I'm tossing ideas around about how to better handle properties. Not
sure exactly how it'll play out though. Stay tuned on that front. --
justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r18260 - trunk/subversion/libsvn_ra_serf
Posted by Greg Stein <gs...@lyra.org>.
On Fri, Jan 27, 2006 at 09:42:31AM +0100, Peter N. Lundblad wrote:
>...
> > - /* todo: create a subpool? */
> > - serf_sess->pool = pool;
> > + apr_pool_create(&serf_sess->pool, pool);
>
> Should be svn_pool_create, but why do you need a subpool?
Right. Unless it is going to be cleared at some points, then a subpool
isn't going to be all that helpful.
>...
> > + vcc_url = fetch_prop(props, sess->repos_url.path, "DAV:",
> > + "version-controlled-configuration");
> > +
> Wouldn't it be better to have #defines for these string constants, making
> it harder to misspell them?
If it gets misspelled, then it simply won't work. The failure mode is
direct and obvious, so a symbolic conatant isn't going to improve
things.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r18260 - trunk/subversion/libsvn_ra_serf
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On 1/27/06, Peter N. Lundblad <pe...@famlundblad.se> wrote:
> > + apr_pool_create(&serf_sess->pool, pool);
>
> Should be svn_pool_create, but why do you need a subpool?
See my reply to Greg's post.
(And, I'm trying to avoid using svn-isms for now.)
> I would use _element_ instead of _tag_ in the name, since it adds a whole
> element, not just a tag.
*shrug*
> Also, a docstring here (and on the other functions) would be really
> valuable, because to me it is not obvious exactly what this function does
> and I don't think you should have to read the code to know what a function
> does. One can guess what this function does, but does it, for example,
> escape the value string with XML entities? Note that we have functions to
> create XML tags already in svn_xml.h, but they work on stringbufs, and
> maybe it is more efficient to work on serf buckets directly (or some other
> good reason) in which case this is fine of course.
I am only trying to complete a proof of concept here. Once I can do
the checkout process, I'm going to throw away most of this code and
start again. First, I need to make myself understand what code is
needed and to prove that serf has all of the necessary features.
Once that happens, I'll go through and document everything. Fear not...
> OK, it does not escape the element content. Would it be an improvement to
> do so? Escaping and encoding bugs are quite common...
I'm purposely not escaping or encoding at this point. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org