You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <da...@elego.de> on 2011/05/11 18:00:54 UTC
Re: svn commit: r1101918 - in
/subversion/trunk/subversion/libsvn_ra_serf: locks.c update.c util.c
cmpilato@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...?
Daniel
(AFAICT, the error-codes-in-use spaces don't intersect right now)
Re: svn commit: r1101918 - in
/subversion/trunk/subversion/libsvn_ra_serf: locks.c update.c util.c
Posted by Daniel Shahaf <da...@elego.de>.
I can look into the svn parts once you deal with the serf parts.
Greg Stein wrote on Wed, May 11, 2011 at 15:35:19 -0400:
> On May 11, 2011 2:05 PM, "C. Michael Pilato" <cm...@collab.net> wrote:
> >
> > On 05/11/2011 12:00 PM, Daniel Shahaf wrote:
> >...
> > >
> > > 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.
>
> Good idea. I can fix that.
>
> > 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,
>
> You have a guarantee :-)
>
> > 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.
>
> Yup. For now, it can just call svn_error_wrap_apr, but we can update it
> after serf exposes a new function. That will necessitate a 0.8 release.
>
> Cheers,
> -g
Re: svn commit: r1101918 - in /subversion/trunk/subversion/libsvn_ra_serf:
locks.c update.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, May 11, 2011 at 17:20, Bert Huijben <be...@qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
>> Sent: woensdag 11 mei 2011 22:41
>> To: Branko Čibej
>> Cc: dev@subversion.apache.org
>> Subject: Re: svn commit: r1101918 - in
>> /subversion/trunk/subversion/libsvn_ra_serf: locks.c update.c util.c
>>
>> Branko Čibej wrote on Wed, May 11, 2011 at 22:15:58 +0200:
>> > On 11.05.2011 22:11, Daniel Shahaf wrote:
>> > > Greg Stein wrote on Wed, May 11, 2011 at 15:35:19 -0400:
>> > >> On May 11, 2011 2:05 PM, "C. Michael Pilato" <cm...@collab.net>
>> wrote:
>> > >>> On 05/11/2011 12:00 PM, Daniel Shahaf wrote:
>> > >>> ...
>> > >>>> 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.
>> > >> Good idea. I can fix that.
>> > >>
>> > >>> 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,
>> > >> You have a guarantee :-)
>> > > Since svn_error_t.apr_err may contain either an svn error code or a serf
>> > > error code, do we care to have an API that tells people which one it is?
>> > >
>> > > Use case: API consumers who don't log err->message and want to
>> > > call svn_strerror(err->apr_err).
>> >
>> > It would be much better if the interface code converted Serf error codes
>> > to an interval that does not conflict with either APR or Subverison
>> > codes. Otherwise you'll never see the end of hacking special-case error
>> > normalization APIs every time we happen to start using another library
>> > that works on top of APR.
The ranges are already distinct:
[?? .. APR_OS_START_USERERR) belongs to APR
[APR_OS_START_USERERR+100 .. ??] are "serf's" range.
[APR_OS_START_USERERR+5000 .. ??] are "Subversion's" range.
(I use quote's because there is no IANA that assigns these ranges...)
>> >
>>
>> We don't add dependencies very often.
>>
>> That said, if Greg can guarantee that Serf will never use more than 5000
>> error codes, we could use
We certainly won't (since many of the errors serf propagated were just
coming out of APR, so we don't need many specific to serf). But with
that said, the error handling will be changing, per below.
>>
>> svn_error_t *
>> svn_error_wrap_serf(apr_status_t serf_err, ...)
>> {
>> return svn_error_wrap_apr(serf_err +
>> SVN_ERROR_SERF_CATEGORY_START, ...);
>> }
>>
>> > (It doesn't help that APR does a poor job of aiding code-space
>> > reservation, but crying over spilt milk won't solve the problem at hand.)
>
> When I introduced many svn_error_t * return paths in ra_serf, I also found a different problem: Subversion and Serf both return APR errors, like the standard EOF error code. When serf receives that error from a handler it expects that the network layer returned that error, while it could have been that we reached the end of a tempfile in our libsvn_wc code.
>
> It would be really nice (but maybe a lot of work) if serf would only use errors in its own range for these errors, to make sure we don't accidentally return them.
Funny you mention that. Just a few days ago, I thought about changing
the serf bucket reading functions to return one of three values:
1) serf_bucket_ready -- please read some more
2) serf_bucket_wait -- nothing available now, come back after a network event
3) serf_bucket_eof -- you'll never get anything more out of me
And leave the errors to just that: errors.
I've also kind of recently decided against the 0.x changing API
concept since it means that I need to maintain the serf release
branch, that 1.7 binds against, for a long long time. It may as well
be called a 1.0 release. But that's a mess that I'll figure out
post-1.7. For 1.8, there will be a completely different serf API, and
it will have an error structure similar to svn's (which is why the
above's enum return value works better). I don't know how svn will be
able to merge a serf_error_t * into its own chain, but that'll be a
problem to solve later. (at a minimum, we can just convert the data to
a string and stash that into svn_error_t.message)
Note that even if serf switches to an error structure like
Subversion's, there is still an error code inside there. I haven't
figured out how to best partition those among all (unknown) users.
This is the problem that Branko refers to, and which I haven't found a
good solution. The best that I can think of is logging a pair:
<prefix-string, code-integer>. Thus, we'd have
foo_error_create("serf", SERF_CODE, ...). But that kinda sucks. The
integer is nice to be able to check err->code in an "if" statement. If
you now need to check that *and* do a string compare... ugh.
Anyways... none of that really solves the problem at hand, and is
really a heads up for big changes later in ra_serf.
Cheers,
-g
RE: svn commit: r1101918 - in /subversion/trunk/subversion/libsvn_ra_serf: locks.c update.c util.c
Posted by Bert Huijben <be...@qqmail.nl>.
> -----Original Message-----
> From: Daniel Shahaf [mailto:d.s@daniel.shahaf.name]
> Sent: woensdag 11 mei 2011 22:41
> To: Branko Čibej
> Cc: dev@subversion.apache.org
> Subject: Re: svn commit: r1101918 - in
> /subversion/trunk/subversion/libsvn_ra_serf: locks.c update.c util.c
>
> Branko Čibej wrote on Wed, May 11, 2011 at 22:15:58 +0200:
> > On 11.05.2011 22:11, Daniel Shahaf wrote:
> > > Greg Stein wrote on Wed, May 11, 2011 at 15:35:19 -0400:
> > >> On May 11, 2011 2:05 PM, "C. Michael Pilato" <cm...@collab.net>
> wrote:
> > >>> On 05/11/2011 12:00 PM, Daniel Shahaf wrote:
> > >>> ...
> > >>>> 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.
> > >> Good idea. I can fix that.
> > >>
> > >>> 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,
> > >> You have a guarantee :-)
> > > Since svn_error_t.apr_err may contain either an svn error code or a serf
> > > error code, do we care to have an API that tells people which one it is?
> > >
> > > Use case: API consumers who don't log err->message and want to
> > > call svn_strerror(err->apr_err).
> >
> > It would be much better if the interface code converted Serf error codes
> > to an interval that does not conflict with either APR or Subverison
> > codes. Otherwise you'll never see the end of hacking special-case error
> > normalization APIs every time we happen to start using another library
> > that works on top of APR.
> >
>
> We don't add dependencies very often.
>
> That said, if Greg can guarantee that Serf will never use more than 5000
> error codes, we could use
>
> svn_error_t *
> svn_error_wrap_serf(apr_status_t serf_err, ...)
> {
> return svn_error_wrap_apr(serf_err +
> SVN_ERROR_SERF_CATEGORY_START, ...);
> }
>
> > (It doesn't help that APR does a poor job of aiding code-space
> > reservation, but crying over spilt milk won't solve the problem at hand.)
When I introduced many svn_error_t * return paths in ra_serf, I also found a different problem: Subversion and Serf both return APR errors, like the standard EOF error code. When serf receives that error from a handler it expects that the network layer returned that error, while it could have been that we reached the end of a tempfile in our libsvn_wc code.
It would be really nice (but maybe a lot of work) if serf would only use errors in its own range for these errors, to make sure we don't accidentally return them.
/**
* Check whether a real error occurred. Note that bucket read functions
* can return EOF and EAGAIN as part of their "normal" operation, so they
* should not be considered an error.
*/
#define SERF_BUCKET_READ_ERROR(status) ((status) \
&& !APR_STATUS_IS_EOF(status) \
&& !APR_STATUS_IS_EAGAIN(status))
Bert
Re: svn commit: r1101918 - in
/subversion/trunk/subversion/libsvn_ra_serf: locks.c update.c util.c
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Branko Čibej wrote on Wed, May 11, 2011 at 22:15:58 +0200:
> On 11.05.2011 22:11, Daniel Shahaf wrote:
> > Greg Stein wrote on Wed, May 11, 2011 at 15:35:19 -0400:
> >> On May 11, 2011 2:05 PM, "C. Michael Pilato" <cm...@collab.net> wrote:
> >>> On 05/11/2011 12:00 PM, Daniel Shahaf wrote:
> >>> ...
> >>>> 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.
> >> Good idea. I can fix that.
> >>
> >>> 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,
> >> You have a guarantee :-)
> > Since svn_error_t.apr_err may contain either an svn error code or a serf
> > error code, do we care to have an API that tells people which one it is?
> >
> > Use case: API consumers who don't log err->message and want to
> > call svn_strerror(err->apr_err).
>
> It would be much better if the interface code converted Serf error codes
> to an interval that does not conflict with either APR or Subverison
> codes. Otherwise you'll never see the end of hacking special-case error
> normalization APIs every time we happen to start using another library
> that works on top of APR.
>
We don't add dependencies very often.
That said, if Greg can guarantee that Serf will never use more than 5000
error codes, we could use
svn_error_t *
svn_error_wrap_serf(apr_status_t serf_err, ...)
{
return svn_error_wrap_apr(serf_err + SVN_ERROR_SERF_CATEGORY_START, ...);
}
> (It doesn't help that APR does a poor job of aiding code-space
> reservation, but crying over spilt milk won't solve the problem at hand.)
>
> -- Brane
Re: svn commit: r1101918 - in /subversion/trunk/subversion/libsvn_ra_serf:
locks.c update.c util.c
Posted by Branko Čibej <br...@e-reka.si>.
On 11.05.2011 22:11, Daniel Shahaf wrote:
> Greg Stein wrote on Wed, May 11, 2011 at 15:35:19 -0400:
>> On May 11, 2011 2:05 PM, "C. Michael Pilato" <cm...@collab.net> wrote:
>>> On 05/11/2011 12:00 PM, Daniel Shahaf wrote:
>>> ...
>>>> 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.
>> Good idea. I can fix that.
>>
>>> 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,
>> You have a guarantee :-)
> Since svn_error_t.apr_err may contain either an svn error code or a serf
> error code, do we care to have an API that tells people which one it is?
>
> Use case: API consumers who don't log err->message and want to
> call svn_strerror(err->apr_err).
It would be much better if the interface code converted Serf error codes
to an interval that does not conflict with either APR or Subverison
codes. Otherwise you'll never see the end of hacking special-case error
normalization APIs every time we happen to start using another library
that works on top of APR.
(It doesn't help that APR does a poor job of aiding code-space
reservation, but crying over spilt milk won't solve the problem at hand.)
-- Brane
Re: svn commit: r1101918 - in
/subversion/trunk/subversion/libsvn_ra_serf: locks.c update.c util.c
Posted by Daniel Shahaf <da...@elego.de>.
Greg Stein wrote on Wed, May 11, 2011 at 15:35:19 -0400:
> On May 11, 2011 2:05 PM, "C. Michael Pilato" <cm...@collab.net> wrote:
> >
> > On 05/11/2011 12:00 PM, Daniel Shahaf wrote:
> >...
> > >
> > > 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.
>
> Good idea. I can fix that.
>
> > 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,
>
> You have a guarantee :-)
Since svn_error_t.apr_err may contain either an svn error code or a serf
error code, do we care to have an API that tells people which one it is?
Use case: API consumers who don't log err->message and want to
call svn_strerror(err->apr_err).
Re: svn commit: r1101918 - in /subversion/trunk/subversion/libsvn_ra_serf:
locks.c update.c util.c
Posted by Greg Stein <gs...@gmail.com>.
On May 11, 2011 2:05 PM, "C. Michael Pilato" <cm...@collab.net> wrote:
>
> On 05/11/2011 12:00 PM, Daniel Shahaf wrote:
>...
> >
> > 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.
Good idea. I can fix that.
> 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,
You have a guarantee :-)
> 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.
Yup. For now, it can just call svn_error_wrap_apr, but we can update it
after serf exposes a new function. That will necessitate a 0.8 release.
Cheers,
-g
Re: svn commit: r1101918 - in /subversion/trunk/subversion/libsvn_ra_serf:
locks.c update.c util.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 05/11/2011 12:00 PM, Daniel Shahaf wrote:
> cmpilato@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 <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand
Re: svn commit: r1101918 - in /subversion/trunk/subversion/libsvn_ra_serf:
locks.c update.c util.c
Posted by "C. Michael Pilato" <cm...@collab.net>.
On 05/11/2011 12:00 PM, Daniel Shahaf wrote:
> cmpilato@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 <cm...@collab.net>
CollabNet <> www.collab.net <> Distributed Development On Demand