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?
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.
Cheers,
-g
