> -----Original Message-----
> From: Daniel Shahaf [mailto:d...@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" <cmpil...@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
