Greg Stein wrote on Tue, Jul 09, 2013 at 20:59:48 -0400:
> On Tue, Jul 9, 2013 at 4:40 PM, Daniel Shahaf <d...@daniel.shahaf.name> wrote:
> > Bert Huijben wrote on Tue, Jul 09, 2013 at 22:32:00 +0200:
> >...
> >> There is no rule that apr_err must be set to something that is defined by
> >> APR or Subversion.
>
> Correct.
>
...
> apr_status_t is intended to hold all of these errors, so we shouldn't
> attempt to remap them. Handle the ones that you recognize, and
> propagate everything else (maybe somebody up the call chain knows how
> to handle it). And that caller may be outside of our APIs. We publish
> an apr_status_t to them for review.
Okay. Suppose an API caller gets 120171 as the error code from
svn_ra_foo(). I ignore the question of how to recognise that 120171 is
not a Subversion error[1].
1 - How does he know that 120171 is a serf error?
2 - Given that 120171 is a serf error, how does he know that it is "SSL
communications related errors" (docstring of SERF_ERROR_SSL_COMM_FAILED)?
3 - How does such an API caller get an error string for that error code?
What are your answers to these questions?
Today the answers are: 1 - by assuming that Subversion doesn't/won't
link to another apr_status_t library which assigns a meaning to 120171;
2 - by hardcoding the values, or including serf.h in their build; 3 - by
inspecting err->message.
For 1, I suggest to remove the need for that assumption by fronting the
serf error code by a Subversion error code which declares
"The ->apr_err values in children of this error are libserf-generated
(so should be passed to serf_error_message() or apr_strerror(), for
example)".
The rationale here is to let every (current and not-yet-existing)
library use the entire APR_OS_START_USERERR range (just like Private Use
codepoints in Unicode), rather than having to have APR maintain
a registry.
For 1 again, I suggest alternatively: add
#define SVN_ERROR_IS_SERF_ERROR(status) \
((status) > APR_OS_START_USERERR+100 && \
(status) < APR_OS_START_USERERR+200)
to our public API. (I'm working off your statement that the reservation
is one hundred codes.)
I don't have a specific idea for 2. Requiring API consumers to include
serf.h in their builds is less than ideal: how non-C API users deal with
this? I think their options are to roll their own serf.h -> $lang error
codes extractor or to consider 120171 an "unrecognized" error code
(since no SVN_ERROR_IN_CATEGORY() returns TRUE for it).
For 3 I think the existing situation is fine.
Daniel
[1] See svn_error_codes.h:95 for the problem: we reserve the right to
define more SVN_ERR_* codes in the [120001, 125000] range, so future
compatibility considerations kick in.
[2] As an aside:
subversion/include/svn_error_codes.h:74: SVN_WARNING =
APR_OS_START_USERERR + 1,
modules/proxy/ajp.h:86:#define AJP_EOVERFLOW (APR_OS_START_USERERR +
1)
include/apreq_error.h:53:#define APREQ_ERROR_GENERAL APR_OS_START_USERERR
include/apreq_error.h:55:#define APREQ_ERROR_TAINTED
(APREQ_ERROR_GENERAL + 1)
