On Wed, May 11, 2011 at 17:20, Bert Huijben <bert_at_qqmail.nl> wrote:
>
>
>> -----Original Message-----
>> From: Daniel Shahaf [mailto:d.s_at_daniel.shahaf.name]
>> Sent: woensdag 11 mei 2011 22:41
>> To: Branko Čibej
>> Cc: dev_at_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" <cmpilato_at_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
Received on 2011-05-12 01:50:09 CEST