On 05/11/2011 12:00 PM, Daniel Shahaf wrote:
> cmpilato_at_apache.org wrote on Wed, May 11, 2011 at 15:20:46 -0000:
>> Author: cmpilato
>> Date: Wed May 11 15:20:46 2011
>> New Revision: 1101918
>>
>> URL: http://svn.apache.org/viewvc?rev=1101918&view=rev
>> Log:
>> Fix issue #3874 ("Unhandled serf_bucket_response_status errors").
>>
>> * subversion/libsvn_ra_serf/update.c (handle_stream),
>> * subversion/libsvn_ra_serf/locks.c (handle_lock),
>> * subversion/libsvn_ra_serf/util.c (svn_ra_serf__handle_status_only,
>> svn_ra_serf__handle_multistatus_only, handle_response):
>> Don't ignore errorful status returns from serf_bucket_response_status().
>>
>> Modified:
>> subversion/trunk/subversion/libsvn_ra_serf/locks.c
>> subversion/trunk/subversion/libsvn_ra_serf/update.c
>> subversion/trunk/subversion/libsvn_ra_serf/util.c
>>
>> Modified: subversion/trunk/subversion/libsvn_ra_serf/locks.c
>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/locks.c?rev=1101918&r1=1101917&r2=1101918&view=diff
>> ==============================================================================
>> --- subversion/trunk/subversion/libsvn_ra_serf/locks.c (original)
>> +++ subversion/trunk/subversion/libsvn_ra_serf/locks.c Wed May 11 15:20:46 2011
>> @@ -360,9 +360,13 @@ handle_lock(serf_request_t *request,
>> const char *val;
>>
>> serf_status_line sl;
>> - apr_status_t rv;
>> + apr_status_t status;
>>
>> - rv = serf_bucket_response_status(response, &sl);
>> + status = serf_bucket_response_status(response, &sl);
>> + if (SERF_BUCKET_READ_ERROR(status))
>> + {
>> + return svn_error_wrap_apr(status, NULL);
>> + }
>>
>> ctx->status_code = sl.code;
>> ctx->reason = sl.reason;
>
> When wrapping apr_status_t's returned by serf, shouldn't we decorate
> them in some way so that code that interprets them knows to look them up
> in serf.h:SERF_ERROR_* rather than in svn_error_codes.h ?
>
> Not sure, perhaps a wrapper err->apr_err link that signals to
> subr/error.c to call into serf to stringify the child link...?
I'm actually not sure that Serf even provides a stringification function at
all. svn_error_wrap_apr() will use 'status' as the err->apr_err value, but
will call APR's stringification function which, I would expect, would just
drop some generic string in place (since the Serf error code range is
outside of APR's own). Of course, there's no guarantee that Subversion's
and Serf's error code ranges won't overlap, only that Serf's and APR's won't
and that Subversion's and APR's won't.
Perhaps the best short-term solution is to not use svn_error_wrap_apr(), but
to introduce svn_error_wrap_serf() instead, which can handle APR error codes
as svn_error_wrap_apr() does, but map Serf error codes to something
Subversion-ish.
Thoughts?
--
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Received on 2011-05-11 20:05:47 CEST