On 14. 7. 25 01:27, Greg Stein wrote:
On Mon, Jul 7, 2025 at 3:28 AM Branko Čibej <br...@apache.org> wrote:
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.
Hmm. Understood, and I hear that. Maybe we can include things like
serf_request_t *, etc and one or more might be NULL? ie. provide as
much context as possible?
I don't like the idea of $N different error callbacks, each one used
in different ways/contexts. Keep Serf simple with one error structure,
and one callback mechanism, and let the APP sort out how it uses it.
... if we put "all available context" into the one structure, then an
APP could have a generic callback for all situations (instead of
one-per-scope).
Still thinking this through. I do like your idea about using actual
serf structures instead of random batons. (tho we should still throw a
baton in, for the lulz)
> /* ### 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.
It was a random question. I don't know how to use it either, but have
seen "sub-code" concepts in error handling, so figured I'd put that
into play for discussion.
>...
> /* 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.
I saw your work on Subversion trimming the root of the path. Totally
agree with that idea. I wonder if we can *inject* a CPP symbol into
each file build, so that we don't even have to bother with runtime
path processing (and potential failures/leaks). "SERF_FILE" or such.
Serf doesn't have the capability to create stack traces like svn has.
But I *do* think we can specify where an error originated, which helps
with debug. Serf's logging might capture some of this, but that is a
separate system which might not be configured. It would also help to
pass this information to the APP, so that the APP can incorporate the
error into its own logs.
> 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. ;
serf_error_t acceptable conceptually?
Sure.
From a purist point of view, though, every time I see pointer to a
struct passed as a parameter, I ask myself if it couldn't just all be
parameters; i.e., there's no real need to have a struct in the first
place, unless it's going to be an extensible private struct with public
accessor functions.
I believe our initial use would be for the SSL callback that Graham is
working on. Then we could grow usage over time/interest, as we run
into issues. I really have a problem with a one-off callback suitable
only for one edge case.
+1 all over that.
-- Brane
