You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Küng <to...@gmail.com> on 2007/12/07 20:14:10 UTC
[PATCH] Allow custom user agent string
Hi,
As discussed before
(http://subversion.tigris.org/servlets/BrowseList?list=dev&by=thread&from=620729),
I took another shot at this.
The attached patch introduces two new functions:
svn_ra_set_client_namestring and svn_ra_get_client_namestring to allow
the clients set the last part of the useragent string. The first part of
the useragent string is not changed, it's still "SVN/" with the svn
library version.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] Allow custom user agent string
Posted by Stefan Küng <to...@gmail.com>.
Eric Gillespie wrote:
> "Justin Erenkrantz" <ju...@erenkrantz.com> writes:
>
>> On Dec 15, 2007 7:56 AM, Eric Gillespie <ep...@pretzelnet.org> wrote:
>>> This looks good to me, though I had to correct the calls to
>>> svn_ra_get_client_namestring. Anyone object if I commit it?
>> Go wild. -- justin
>
> Committed in r28503.
Thanks!
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Allow custom user agent string
Posted by Eric Gillespie <ep...@pretzelnet.org>.
"Justin Erenkrantz" <ju...@erenkrantz.com> writes:
> On Dec 15, 2007 7:56 AM, Eric Gillespie <ep...@pretzelnet.org> wrote:
> > This looks good to me, though I had to correct the calls to
> > svn_ra_get_client_namestring. Anyone object if I commit it?
>
> Go wild. -- justin
Committed in r28503.
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Allow custom user agent string
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Dec 15, 2007 7:56 AM, Eric Gillespie <ep...@pretzelnet.org> wrote:
> This looks good to me, though I had to correct the calls to
> svn_ra_get_client_namestring. Anyone object if I commit it?
Go wild. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Allow custom user agent string
Posted by Eric Gillespie <ep...@pretzelnet.org>.
=?ISO-8859-1?Q?Stefan_K=FCng?= <to...@gmail.com> writes:
> I've changed the patch to add a 'const char *useragent' to the
> connection struct.
>
> New patch attached.
This looks good to me, though I had to correct the calls to
svn_ra_get_client_namestring. Anyone object if I commit it?
Thanks, Stefan.
--
Eric Gillespie <*> epg@pretzelnet.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Allow custom user agent string
Posted by Stefan Küng <to...@gmail.com>.
Justin Erenkrantz wrote:
> On Dec 9, 2007 2:42 AM, Stefan Küng <to...@gmail.com> wrote:
>> Well, I used the same approach as svn_wc_set_adm_dir() : that function
>> doesn't really need a pool argument either. IIRC, the discussion back
>> then was that all functions should take a pool argument, even if not
>> currently used so it can be extended more easily in the future.
>
> Well, used in the manner of this patch, a pool would be unsafe.
True. I've removed the pool argument.
>> AFAIK, there are not that many requests done for an operation. Which
>> means it shouldn't be a big problem?
>
> No - it is a big problem. There's at least 2x-per-file HTTP requests
> on an update (GET & PROPFIND).
>
> So, yah, that can be a lot of requests - so allocating from the
> session pool is badness - especially when we're dealing with a string
> that shouldn't change after initialization (based on the docstring).
I see. I only tested this with neon when I checked how much that code
part was called. That's why I didn't notice that. But it worked when I
ran some command with serf outside the debugger, so I assumed it was ok.
I've changed the patch to add a 'const char *useragent' to the
connection struct.
New patch attached.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] Allow custom user agent string
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Dec 9, 2007 2:42 AM, Stefan Küng <to...@gmail.com> wrote:
> Well, I used the same approach as svn_wc_set_adm_dir() : that function
> doesn't really need a pool argument either. IIRC, the discussion back
> then was that all functions should take a pool argument, even if not
> currently used so it can be extended more easily in the future.
Well, used in the manner of this patch, a pool would be unsafe.
> AFAIK, there are not that many requests done for an operation. Which
> means it shouldn't be a big problem?
No - it is a big problem. There's at least 2x-per-file HTTP requests
on an update (GET & PROPFIND).
So, yah, that can be a lot of requests - so allocating from the
session pool is badness - especially when we're dealing with a string
that shouldn't change after initialization (based on the docstring).
> attached is a corrected patch: the first one forgot to add the '0'
> argument for apr_pstrcat as the last argument and therefore would
> randomly crash in release builds.
Please use NULL instead of 0. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Allow custom user agent string
Posted by Stefan Küng <to...@gmail.com>.
Justin Erenkrantz wrote:
> On Dec 7, 2007 12:14 PM, Stefan Küng <to...@gmail.com> wrote:
>> +const char * svn_ra_get_client_namestring(apr_pool_t *pool);
>
> Does this really need to take a pool argument? The implementation
> doesn't need it.
Well, I used the same approach as svn_wc_set_adm_dir() : that function
doesn't really need a pool argument either. IIRC, the discussion back
then was that all functions should take a pool argument, even if not
currently used so it can be extended more easily in the future.
> But, a note on the serf bits...
>
[snip]
>
> The usage for ra_serf at least would appear that we'd allocate memory
> from the session pool on every request. We should just add a field to
> the ra_serf session structure and initialize it to the User-Agent
> value on session init and reuse that on every request rather than
> doing a pstrcat from the session pool per request. -- justin
AFAIK, there are not that many requests done for an operation. Which
means it shouldn't be a big problem?
Anyway:
attached is a corrected patch: the first one forgot to add the '0'
argument for apr_pstrcat as the last argument and therefore would
randomly crash in release builds.
Stefan
--
___
oo // \\ "De Chelonian Mobile"
(_,\/ \_/ \ TortoiseSVN
\ \_/_\_/> The coolest Interface to (Sub)Version Control
/_/ \_\ http://tortoisesvn.net
Re: [PATCH] Allow custom user agent string
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
On Dec 7, 2007 12:14 PM, Stefan Küng <to...@gmail.com> wrote:
> +const char * svn_ra_get_client_namestring(apr_pool_t *pool);
Does this really need to take a pool argument? The implementation
doesn't need it.
But, a note on the serf bits...
> Index: subversion/libsvn_ra_serf/propfind_buckets.c
> ===================================================================
> --- subversion/libsvn_ra_serf/propfind_buckets.c (revision 28326)
> +++ subversion/libsvn_ra_serf/propfind_buckets.c (working copy)
> @@ -141,10 +141,16 @@
> {
> prop_context_t *ctx = bucket->data;
> serf_bucket_t *hdrs_bkt, *body_bkt;
> + const char * useragent = NULL;
>
> + useragent = apr_pstrcat(ctx->conn->session->pool,
> + USER_AGENT,
> + "/",
> + svn_ra_get_client_namestring(ctx->conn->session->pool));
> body_bkt = create_propfind_body(bucket);
>
> serf_bucket_request_become(bucket, "PROPFIND", ctx->path, body_bkt);
> +
> #if SERF_VERSION_AT_LEAST(0,1,3)
> if (ctx->conn->session->using_proxy)
> {
> @@ -158,7 +164,7 @@
> hdrs_bkt = serf_bucket_request_get_headers(bucket);
>
> serf_bucket_headers_setn(hdrs_bkt, "Host", ctx->conn->hostinfo);
> - serf_bucket_headers_setn(hdrs_bkt, "User-Agent", USER_AGENT);
> + serf_bucket_headers_setn(hdrs_bkt, "User-Agent", useragent);
> if (ctx->conn->using_compression == TRUE)
> {
> serf_bucket_headers_setn(hdrs_bkt, "Accept-Encoding", "gzip");
> Index: subversion/libsvn_ra_serf/util.c
> ===================================================================
> --- subversion/libsvn_ra_serf/util.c (revision 28326)
> +++ subversion/libsvn_ra_serf/util.c (working copy)
> @@ -265,13 +265,19 @@
> serf_bucket_t *body_bkt, const char *content_type)
> {
> serf_bucket_t *hdrs_bkt;
> + const char * useragent = NULL;
>
> + useragent = apr_pstrcat(conn->session->pool,
> + USER_AGENT,
> + "/",
> + svn_ra_get_client_namestring(conn->session->pool));
> +
> *req_bkt = serf_bucket_request_create(method, url, body_bkt,
> serf_request_get_alloc(request));
>
> hdrs_bkt = serf_bucket_request_get_headers(*req_bkt);
> serf_bucket_headers_setn(hdrs_bkt, "Host", conn->hostinfo);
> - serf_bucket_headers_setn(hdrs_bkt, "User-Agent", USER_AGENT);
> + serf_bucket_headers_setn(hdrs_bkt, "User-Agent", useragent);
> if (content_type)
> {
> serf_bucket_headers_setn(hdrs_bkt, "Content-Type", content_type);
The usage for ra_serf at least would appear that we'd allocate memory
from the session pool on every request. We should just add a field to
the ra_serf session structure and initialize it to the User-Agent
value on session init and reuse that on every request rather than
doing a pstrcat from the session pool per request. -- justin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org