On 6. 7. 25 17:22, Greg Stein wrote:
I had previously indicated that I did not like this approach. This is a
one-off that is not suitable for the project.
If we're going to create callbacks to provide additional error information,
then I think we need a mechanism that can eventually be used across the
entire library. I think that we'd pass an error structure rather just a
simple string. If the only use of that callback is for SSL, that's fine and
we can grow into it. I think the structure would look something like this:
struct serf_error_t {
apr_status_t status;
int scope;
#define SERF_ERRSCOPE_CONTEXT 0
#define SERF_ERRSCOPE_CONNECTION 1
#define SERF_ERRSCOPE_REQUEST 2
...
I'm a firm believer in avoiding enumerations where they can be replaced
by structure. When I look at these constants with a library consumer's
hat on, questions immediately pop up: which context? which connection?
which request? So do we add a baton that points to the relevant objects
and incidentally loose type information? Seems to me we'd be better off
with several callbacks, one for each scope, than with one generic
callback that throws away information it has.
/* ### not sure about this. needed? */
int sub_status;
A consumer outside of Serf's internals is not likely to have the first
clue how to interpret this, unless we expose the internals in a public
header. I'd rather not do that.
/* NOTE: survives the duration of the callback; copy as needed */
const char *message;
/* Where was this error generated? FILE is static/forever. */
const char *file;
int lineno;
}
Not sure about file/line, either. That's something you write in a log
(and we can automagically log when the callbacks are invoked), but a
library consumer won't care. Though, I agree, it's useful info to have
in an error report. *THAT SAID*, however: __FILE__ exposes information
about the machine this was built on. If it was relative to the source
directory, that'd be ok. But it usually isn't. This is a problem in the
logs, too, but they're turned off/compiled out by default. If we want
the location info, there are better ways than using __FILE__. Yes, I
understand that you didn't literally mean __FILE__, but still, it bears
pointing out. It's one of the things that ticks me off about
Subversion's error tracing, for example.
If we're going to use a callback approach, then let's do it right. I
disagree with this one-off.
You only have to write some code to make it more agreeable. ;)
-- Brane
