Jon Foster wrote on Mon, Sep 20, 2010 at 22:59:02 +0100:
> Daniel Shahaf wrote:
> > Jon Foster wrote on Mon, Sep 20, 2010 at 11:01:02 +0100:
> > > However, there is a simpler way! The <D:status> element contains
> > > the HTTP error code, usually "500 Internal Server Error". So we
> > > could pick a special HTTP status code to mean SVN_ERR_BAD_OLD_VALUE.
> > > I think of old_value_p as a precondition for the operation, a bit
> > > like "If-Modified-Since:", so I'd suggest "412 Precondition Failed".
> > > Note that generic DAV clients won't ever see this message, because
> > > they won't be sending old_value_p.
> >
> > I'll commit this once I have an ra_serf version too. Would you
> > like to write the ra_serf part of the patch, too?
>
> Sure. See attached.
> This updated patch is against the atomic-revprop branch.
>
> [[[
> Signal SVN_ERR_BAD_OLD_VALUE over DAV using a HTTP 412 status
> code inside a 207 multi-status response.
>
> * subversion/mod_dav_svn/util.c:
No trailing colon here ----------^
> (dav_svn__convert_err): Map SVN_ERR_BAD_OLD_VALUE to 412.
>
> * subversion/libsvn_ra_neon/util.c:
> (multistatus_baton_t): New member contains_precondition_error.
> (end_207_element): Check for HTTP 412 status code and create
> a SVN_ERR_BAD_OLD_VALUE error if found.
>
> * subversion/libsvn_ra_serf/ra_serf.h:
> (svn_ra_serf__server_error_t): New member
> contains_precondition_error.
>
> * subversion/libsvn_ra_serf/util.c:
> (parse_dav_status): New method.
> (svn_ra_serf__handle_discard_body): Initialise new member.
> (start_207): Parse DAV:status XML element.
> (end_207): Parse DAV:status XML element. Use SVN_ERR_BAD_OLD_VALUE
> error code if applicable.
> (svn_ra_serf__handle_multistatus_only): Initialise new member.
>
> Patch by: Jon Foster <jon.fos...@cabot.co.uk>
> ]]]
>
> Kind regards,
>
> Jon
>
> Index: subversion/libsvn_ra_serf/util.c
> ===================================================================
> --- subversion/libsvn_ra_serf/util.c (revision 999102)
> +++ subversion/libsvn_ra_serf/util.c (working copy)
> @@ -836,6 +836,7 @@ svn_ra_serf__handle_discard_body(serf_request_t *r
> {
> server_err->error = svn_error_create(APR_SUCCESS, NULL, NULL);
> server_err->has_xml_response = TRUE;
> + server_err->contains_precondition_error = FALSE;
> server_err->cdata = svn_stringbuf_create("", pool);
> server_err->collect_cdata = FALSE;
> server_err->parser.pool = server_err->error->pool;
(So, this is the error handler for a function that normally discards its
response, and the meat of the patch is in
svn_ra_serf__handle_multistatus_only().)
> @@ -945,6 +946,31 @@ svn_ra_serf__handle_status_only(serf_request_t *re
> return svn_error_return(err);
> }
>
> +/* Given a string like "HTTP/1.1 500 (status)", parse out the numeric status
> + code. Ignores leading whitespace. This function will overwrite and then
> + clear the passed buf. */
Sounds like a strange semantics. Couldn't you just take a scratch_pool
argument and duplicate the buffer, leaving the caller's copy unchanged?
Changing it in-place seems like a shoot-oneself-in-the-foot move...
> +static svn_error_t *
> +parse_dav_status(svn_stringbuf_t *buf, int *status_code_out)
> +{
> + svn_error_t * err;
> + const char * token;
> + char * tok_status = 0;
Style nit: no space after the * in the last three lines.
Also, no need to initialize TOK_STATUS (says svn_cstring_split_append()).
> +
> + svn_stringbuf_strip_whitespace(buf);
> + token = apr_strtok(buf->data, " \t\r\n", &tok_status);
> + if (token)
> + token = apr_strtok(NULL, " \t\r\n", &tok_status);
> + if (!token)
> + return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> + "Malformed DAV:status CDATA");
Should the cdata be included in the error text? (using svn_error_createf())
> + err = svn_cstring_atoi(status_code_out, token);
> + if (err)
> + return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, err,
> + "Malformed DAV:status CDATA");
> + svn_stringbuf_setempty(buf);
> + return SVN_NO_ERROR;
> +}
> +
> /*
> * Expat callback invoked on a start element tag for a 207 response.
> */
> @@ -968,6 +994,14 @@ start_207(svn_ra_serf__xml_parser_t *parser,
> svn_stringbuf_setempty(ctx->cdata);
> ctx->collect_cdata = TRUE;
> }
> + else if (ctx->in_error &&
> + strcmp(name.namespace, "DAV:") == 0 &&
> + strcmp(name.name, "status") == 0)
> + {
> + /* Start collecting cdata. */
> + svn_stringbuf_setempty(ctx->cdata);
Okay; D:responsedescription and D:status are siblings [1], so it's okay to
reuse CTX->cdata.
[1] http://paste.lisp.org/display/113346
<paranoia on>
Are you sure they will always be siblings? If we aren't sure that yes,
we could just use two stringbufs (instead of reusing ctx->cdata).
> + ctx->collect_cdata = TRUE;
> + }
>
> return SVN_NO_ERROR;
> }
> @@ -993,9 +1027,24 @@ end_207(svn_ra_serf__xml_parser_t *parser,
> ctx->collect_cdata = FALSE;
> ctx->error->message = apr_pstrmemdup(ctx->error->pool,
> ctx->cdata->data,
> ctx->cdata->len);
> - ctx->error->apr_err = SVN_ERR_RA_DAV_REQUEST_FAILED;
> + if (ctx->contains_precondition_error)
> + ctx->error->apr_err = SVN_ERR_BAD_OLD_VALUE;
> + else
> + ctx->error->apr_err = SVN_ERR_RA_DAV_REQUEST_FAILED;
> }
> + else if (ctx->in_error &&
> + strcmp(name.namespace, "DAV:") == 0 &&
> + strcmp(name.name, "status") == 0)
> + {
> + int status_code;
>
> + ctx->collect_cdata = FALSE;
> +
> + SVN_ERR(parse_dav_status(ctx->cdata, &status_code));
Our convention is to have the output parameters first.
> + if (status_code == 412)
> + ctx->contains_precondition_error = TRUE;
> + }
> +
> return SVN_NO_ERROR;
> }
>
> @@ -1044,6 +1093,7 @@ svn_ra_serf__handle_multistatus_only(serf_request_
> {
> server_err->error = svn_error_create(APR_SUCCESS, NULL, NULL);
> server_err->has_xml_response = TRUE;
> + server_err->contains_precondition_error = FALSE;
> server_err->cdata = svn_stringbuf_create("", pool);
> server_err->collect_cdata = FALSE;
> server_err->parser.pool = server_err->error->pool;
> Index: subversion/libsvn_ra_serf/ra_serf.h
> ===================================================================
> --- subversion/libsvn_ra_serf/ra_serf.h (revision 999102)
> +++ subversion/libsvn_ra_serf/ra_serf.h (working copy)
> @@ -673,6 +673,9 @@ typedef struct svn_ra_serf__server_error_t {
> /* Have we seen an error tag? */
> svn_boolean_t in_error;
>
> + /* Have we seen a HTTP "412 Precondition Failed" error? */
> + svn_boolean_t contains_precondition_error;
> +
> /* Should we be collecting the XML cdata? */
> svn_boolean_t collect_cdata;
>
