RE: [PATCH 3/3] atomic-revprop: Signal the error as a HTTP status code

Hi,

Updated patch attached.

[[[
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
  (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.
   (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.foster_at_cabot.co.uk>
]]]

Daniel Shahaf wrote:
> > [[[
> > 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 ----------^

Fixed.

[...]
> > 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->contains_precondition_error = FALSE;
>
> (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().)

After reviewing this code again, I've dropped that chunk. I've
convinced
myself that the variable is never used in that code path.

> > @@ -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...

Changed.

> > +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()).

Both done.

> > +
> > + 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())

Can do, and in the new patch I have.

> > + 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).

Even with the old code, if there are any elements with CDATA nested
inside <D:responsedescription>, Serf is going to go wrong. If we
collect CDATA into different variables, then it'll just go wrong in
a different way (e.g. the HTTP status line might be shown as part of
the message).

I think there's a whole package of work that could be done to make
Serf resilient against weird XML, but I think that's separate from
this work. (It's also quite hard to test).

> > + ctx->collect_cdata = TRUE;
[...]
> > + SVN_ERR(parse_dav_status(ctx->cdata, &status_code));
> Our convention is to have the output parameters first.

Changed.

Thanks for the thourough review!

Jon

**********************************************************************
This email and its attachments may be confidential and are intended solely for the use of the individual to whom it is addressed. Any views or opinions expressed are solely those of the author and do not necessarily represent those of Cabot Communications Ltd.

If you are not the intended recipient of this email and its attachments, you must take no action based upon them, nor must you copy or show them to anyone.

Cabot Communications Limited
Verona House, Filwood Road, Bristol BS16 3RY, UK
+44 (0) 1179584232

Co. Registered in England number 02817269

Please contact the sender if you believe you have received this email in error.

**********************************************************************

______________________________________________________________________
This email has been scanned by the MessageLabs Email Security System.
For more information please visit http://www.messagelabs.com/email
______________________________________________________________________