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 2005/02/08 14:22:13 UTC
Re: svn commit: r12937 - branches/locking/subversion/libsvn_ra_dav
On Mon, 7 Feb 2005 sussman@tigris.org wrote:
> Modified: branches/locking/subversion/libsvn_ra_dav/fetch.c
> Url: http://svn.collab.net/viewcvs/svn/branches/locking/subversion/libsvn_ra_dav/fetch.c?view=diff&rev=12937&p1=branches/locking/subversion/libsvn_ra_dav/fetch.c&r1=12936&p2=branches/locking/subversion/libsvn_ra_dav/fetch.c&r2=12937
> ==============================================================================
> --- branches/locking/subversion/libsvn_ra_dav/fetch.c (original)
> +++ branches/locking/subversion/libsvn_ra_dav/fetch.c Mon Feb 7 17:20:22 2005
> @@ -1242,6 +1258,319 @@
>
> return err;
> }
...
> +
> +/*
> + * The get-locks-report xml request body is super-simple.
> + * The server doesn't need anything but the URI in the REPORT request line.
> + *
> + * <S:get-locks-report xmlns...>
> + * </S:get-locks-report>
> + *
> + * The get-locks-report xml response is just a list of svn_lock_t's
> + * that exist at or "below" the request URI. (The server runs
> + * svn_repos_fs_get_locks()).
> + *
> + * <S:get-locks-report xmlns...>
> + * <S:lock>
> + * <S:path>/foo/bar/baz</S:path>
> + * <S:token>opaquelocktoken:706689a6-8cef-0310-9809-fb7545cbd44e
> + * </S:token>
> + * <S:owner>fred</S:owner>
> + * <S:comment encoding="base64">ET39IGCB93LL4M</S:comment>
> + * <S:creationdate>2005-02-07 14:17:08 -0600 (Mon, 07 Feb 2005)
> + * </S:creationdate>
I hope you mean
<S:creationdate>2005-02-07T14:17:08Z</S:creationdate>
i.e. not the human date format.
> + * <S:expirationdate>2005-02-08 14:17:08 -0600 (Mon, 08 Feb 2005)
Same.
> + * </S:expirationdate>
> + * </S:lock>
> + * ...
> + * </S:get-locks-report>
> + *
> + *
> + * The <path> and <token> and date-element cdata is xml-escaped by mod_dav_svn.
> + * The <owner> and <comment> cdata is xml-escaped, or *possibly* base64.
It is confusing to say "or *possibly*". It is always XML escaped, per
definition.
> +static int getlocks_end_element(void *userdata, int state,
> + const char *ns, const char *ln)
> +{
> + get_locks_baton_t *baton = userdata;
> + const svn_ra_dav__xml_elm_t *elm;
> + svn_error_t *err;
> +
> + elm = svn_ra_dav__lookup_xml_elem(getlocks_report_elements, ns, ln);
> +
> + /* Just skip unknown elements. */
> + if (elm == NULL)
> + return NE_XML_DECLINE;
> +
> + switch (elm->id)
> + {
> + case ELEM_lock:
> + /* finish the whole svn_lock_t. */
> + apr_hash_set(baton->lock_hash, baton->current_lock->path,
> + APR_HASH_KEY_STRING, baton->current_lock);
> + break;
> +
I think you want to validate the lock here. Else, misbehaving servers can
crash the client. This means checking that mandatory fields really were
set.
> + case ELEM_lock_path:
> + /* neon has already xml-unescaped the cdata for us. */
> + baton->current_lock->path = apr_pstrdup(baton->pool,
> + baton->cdata_accum->data);
> + /* clean up the accumulator. */
> + svn_stringbuf_setempty(baton->cdata_accum);
> + svn_pool_clear(baton->scratchpool);
> + break;
> +
> + case ELEM_lock_token:
> + /* neon has already xml-unescaped the cdata for us. */
> + baton->current_lock->token = apr_pstrdup(baton->pool,
> + baton->cdata_accum->data);
> + /* clean up the accumulator. */
> + svn_stringbuf_setempty(baton->cdata_accum);
> + svn_pool_clear(baton->scratchpool);
> + break;
> +
> + case ELEM_lock_creationdate:
> + err = svn_time_from_cstring(&(baton->current_lock->creation_date),
> + baton->cdata_accum->data,
> + baton->scratchpool);
Yeah, it is the ISO date format.
> + /* clean up the accumulator. */
> + svn_stringbuf_setempty(baton->cdata_accum);
> + svn_pool_clear(baton->scratchpool);
> + break;
> +
> + case ELEM_lock_expirationdate:
> + err = svn_time_from_cstring(&(baton->current_lock->expiration_date),
> + baton->cdata_accum->data,
> + baton->scratchpool);
> + /* clean up the accumulator. */
> + svn_stringbuf_setempty(baton->cdata_accum);
> + svn_pool_clear(baton->scratchpool);
> + break;
> +
> + case ELEM_lock_owner:
> + case ELEM_lock_comment:
> + {
> + const char *final_val;
> +
> + if (baton->encoding)
> + {
> + /* ### possibly recognize other encodings someday */
What other encodings would that be? Wouldn't it be simple to just use a
boolean in the baton for base64?
> + if (strcmp(baton->encoding, "base64") == 0)
> + {
> + svn_string_t *encoded_val;
> + const svn_string_t *decoded_val;
> +
> + encoded_val = svn_string_create_from_buf(baton->cdata_accum,
> + baton->scratchpool);
> + decoded_val = svn_base64_decode_string(encoded_val,
> + baton->scratchpool);
> + final_val = decoded_val->data;
> + }
> + else
> + /* unrecognized encoding! */
> + return NE_XML_ABORT;
> +
> + baton->encoding = NULL;
> + }
> + else
> + {
> + /* neon has already xml-unescaped the cdata for us. */
> + final_val = baton->cdata_accum->data;
> + }
> +
> + if (elm->id == ELEM_lock_owner)
> + baton->current_lock->owner = apr_pstrdup(baton->pool, final_val);
> + if (elm->id == ELEM_lock_comment)
> + baton->current_lock->comment = apr_pstrdup(baton->pool, final_val);
> +
> + /* clean up the accumulator. */
> + svn_stringbuf_setempty(baton->cdata_accum);
> + svn_pool_clear(baton->scratchpool);
> + break;
> + }
> +
> +
> + default:
> + break;
> + }
> +
> + return 0;
> +}
err is leaked. I think you need to store the error in the baton and return
it after the parse.
> +
> +
> +
> +svn_error_t *
> +svn_ra_dav__get_locks(svn_ra_session_t *session,
> + apr_hash_t **locks,
> + const char *path,
> + apr_pool_t *pool)
> +{
> + svn_ra_dav__session_t *ras = session->priv;
> + const char *body, *url;
> + svn_error_t *err;
> + int status_code = 0;
> + get_locks_baton_t *baton;
> +
> + baton = apr_pcalloc(pool, sizeof(*baton));
> + baton->lock_hash = apr_hash_make (pool);
> + baton->pool = pool;
> + baton->scratchpool = svn_pool_create(pool);
> + baton->cdata_accum = svn_stringbuf_create("", baton->scratchpool);
> +
> + body = apr_psprintf(pool,
> + "<?xml version=\"1.0\" encoding=\"utf-8\"?>"
> + "<S:get-locks-report xmlns:S=\"" SVN_XML_NAMESPACE "\" "
> + "xmlns:D=\"DAV:\">"
> + "</S:get-locks-report>");
> +
> +
> + /* We always run the report on the 'public' URL, which represents
> + HEAD anyway. If the path doesn't exist in HEAD, then
> + svn_ra_get_locks() *should* fail. Lock queries are always on HEAD. */
> + url = svn_path_url_add_component (ras->url, path, pool);
> +
> + err = svn_ra_dav__parsed_request(ras->sess, "REPORT", url,
> + body, NULL, NULL,
> + getlocks_start_element,
> + getlocks_cdata_handler,
> + getlocks_end_element,
> + baton,
> + NULL, /* extra headers */
> + &status_code,
> + pool);
> +
> + /* Map status 501: Method Not Implemented to our not implemented error.
> + 1.0.x servers and older don't support this report. */
> + if (status_code == 501)
> + return svn_error_create (SVN_ERR_RA_NOT_IMPLEMENTED, err,
> + _("Server does not support locking features."));
TRailing dot:-)
> +
> + if (err && err->apr_err == SVN_ERR_UNSUPPORTED_FEATURE)
> + return svn_error_quick_wrap(err,
> + _("Server does not support locking features"));
> +
> + else if (err)
> + return err;
> +
> + svn_pool_destroy(baton->scratchpool);
> +
> + *locks = baton->lock_hash;
> + return err;
> +}
> +
Best,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12937 - branches/locking/subversion/libsvn_ra_dav
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Tue, 8 Feb 2005, Ben Collins-Sussman wrote:
>
> On Feb 8, 2005, at 8:22 AM, Peter N. Lundblad wrote:
> >>
> >> + switch (elm->id)
> >> + {
> >> + case ELEM_lock:
> >> + /* finish the whole svn_lock_t. */
> >> + apr_hash_set(baton->lock_hash, baton->current_lock->path,
> >> + APR_HASH_KEY_STRING, baton->current_lock);
> >> + break;
> >> +
> > I think you want to validate the lock here. Else, misbehaving servers
> > can
> > crash the client. This means checking that mandatory fields really were
> > set.
>
> Hm, maybe we should create some svn_lock_validate() function in
> libsvn_subr? I imagine that other RA layers might want to validate
> locks coming from the network too, yes?
>
Not that I know of. libsvn_ra_svn's praser will validate the input in the
sense that it won't allow a missing path field fr example.
> >> const char *final_val;
> >> +
> >> + if (baton->encoding)
> >> + {
> >> + /* ### possibly recognize other encodings someday */
> >
> > What other encodings would that be? Wouldn't it be simple to just use a
> > boolean in the baton for base64?
>
> kfogel and cmpilato wanted to leave the doors open for other possible
> encodings in the future. Nothing specific in mind, just wanted to keep
> it flexible.
>
I don't feel strongly about it, but I think that is premature. You can't
add the new encoding to a server in 1.x without breaking compatibility.
And, to be honest, it is not that there are a lot of encodings that we
would need to support. I think it's just confusing. But, I leave it up to
you.
> > err is leaked. I think you need to store the error in the baton and
> > return
> > it after the parse.
>
> What err??
>
You have a variable named err in that function. It is set during date
parsing but never cleared, nor returned AFAICT.
Bye,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r12937 - branches/locking/subversion/libsvn_ra_dav
Posted by Ben Collins-Sussman <su...@collab.net>.
On Feb 8, 2005, at 8:22 AM, Peter N. Lundblad wrote:
>>
>> + switch (elm->id)
>> + {
>> + case ELEM_lock:
>> + /* finish the whole svn_lock_t. */
>> + apr_hash_set(baton->lock_hash, baton->current_lock->path,
>> + APR_HASH_KEY_STRING, baton->current_lock);
>> + break;
>> +
> I think you want to validate the lock here. Else, misbehaving servers
> can
> crash the client. This means checking that mandatory fields really were
> set.
Hm, maybe we should create some svn_lock_validate() function in
libsvn_subr? I imagine that other RA layers might want to validate
locks coming from the network too, yes?
>> const char *final_val;
>> +
>> + if (baton->encoding)
>> + {
>> + /* ### possibly recognize other encodings someday */
>
> What other encodings would that be? Wouldn't it be simple to just use a
> boolean in the baton for base64?
kfogel and cmpilato wanted to leave the doors open for other possible
encodings in the future. Nothing specific in mind, just wanted to keep
it flexible.
>
>> + if (strcmp(baton->encoding, "base64") == 0)
>> + {
>> + svn_string_t *encoded_val;
>> + const svn_string_t *decoded_val;
>> +
>> + encoded_val =
>> svn_string_create_from_buf(baton->cdata_accum,
>> +
>> baton->scratchpool);
>> + decoded_val = svn_base64_decode_string(encoded_val,
>> +
>> baton->scratchpool);
>> + final_val = decoded_val->data;
>> + }
>> + else
>> + /* unrecognized encoding! */
>> + return NE_XML_ABORT;
>> +
>> + baton->encoding = NULL;
>> + }
>> + else
>> + {
>> + /* neon has already xml-unescaped the cdata for us. */
>> + final_val = baton->cdata_accum->data;
>> + }
>> +
>> + if (elm->id == ELEM_lock_owner)
>> + baton->current_lock->owner = apr_pstrdup(baton->pool,
>> final_val);
>> + if (elm->id == ELEM_lock_comment)
>> + baton->current_lock->comment = apr_pstrdup(baton->pool,
>> final_val);
>> +
>> + /* clean up the accumulator. */
>> + svn_stringbuf_setempty(baton->cdata_accum);
>> + svn_pool_clear(baton->scratchpool);
>> + break;
>> + }
>> +
>> +
>> + default:
>> + break;
>> + }
>> +
>> + return 0;
>> +}
>
> err is leaked. I think you need to store the error in the baton and
> return
> it after the parse.
What err??
I'll fix the rest of this stuff... thanks!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org