Re: error callbacks

[Branko Čibej]

On 22. 7. 25 09:23, Daniel Sahlberg wrote:
Den mån 14 juli 2025 kl 01:27 skrev Greg Stein<gst...@gmail.com>:
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'm not sure if we need a struct. Since the struct is in the public
headers, wouldn't we need to decide NOW on all the fields that must go in,
or have a private struct with public accessor functions?

I'd prefer function arguments to a struct in any case. It's not as if 
we're going to extend this indefinitely ... *unless* we want the struct 
to also have its own (global) pool, the way svn_error_t does.

Even then, it's more up to the caller to decide what to do with the 
error info.


Agree that we need to provide some context so the application can
understand in which request the error occured, but couldn't that be up to
the application to provide a reasonable baton?


The caller has no way to know whether the error occurred during context, 
connection or request processing.


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.

I like the idea of having a sub_status. It might not make much sense to a
random user, but for debugging purposes it might be useful. Of course it
would be nice to match any subsystem error to proper SERF errors, but maybe
it doesn't make sense to replicate EVERY error code that could happen.
Maybe it is enough to know there was an error in loading an SSL certificate
and we can then look up the sub_status in OpenSSL's error code. (We do this
at $dayjob, our system vendor is returning an error code for "database
error" and the subcode can then be referenced in the database vendor's
error code listing).


We have to map errors to apr_status_t in any case, but having the 
original code (and string) available is indeed better.


...
      /* 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

I tried to follow the code and I can't figure out if there is a place where
we can store the callback function pointer, except in the
serf_ssl_context_t, just like it is done right now. So if my understanding
is correct, we probably have to add a function pointer to any and all other
context_t:s where we might want to have an error callback. Is this correct?

Assuming my understanding is correct, and the goal is to enable
applications to have only ONE error callback, would it make sense to rename
the function pointer type from serf_ssl_error_cb_t to serf_error_cb_t? To
differentiate between the kind of error, we could add an additional
parameter error_type.

We still need to have setter functions for each context_t that support
error callbacks and the baton will thus be context dependent. The
application using Serf will be able to figure out the type of the baton
based on the error_type.


[[[
Index: serf_bucket_types.h
===================================================================
--- serf_bucket_types.h (revision 1927380)
+++ serf_bucket_types.h (working copy)
@@ -687,16 +687,30 @@
      void *data);

  /**
- * Callback type for detailed TLS error strings. This callback will be
fired
- * every time the underlying crypto library encounters an error. The
message
- * lasts only as long as the callback, if the caller wants to set aside the
- * message for later use, a copy must be made.
+ * Type of error in the serf_error_cb_t callback
+ */
+typedef enum serf_error_type_t {
+    SERF_SSL_ERROR
+} serf_error_type_t;
+
+/**
+ * Callback type for detailed error strings.
   *
+ * Depending on @a error_type the baton may be of different types.
+ *
+ * The message lasts only as long as the callback, if the caller wants to
+ * set aside the message for later use, a copy must be made.
+ *
   * It is possible that for a given error multiple strings will be returned
   * in multiple callbacks. The caller may choose to handle all strings, or
   * may choose to ignore all strings but the last most detailed one.
+ *
+ * SERF_SSL_ERROR
+ * This callback will be fired every time the underlying crypto library
+ * encounters an error.
   */
-typedef apr_status_t (*serf_ssl_error_cb_t)(
+typedef apr_status_t (*serf_error_cb_t)(
+    serf_error_type_t error_type,
      void *baton,
      apr_status_t status,
      const char *message);
@@ -710,7 +724,7 @@
   */
  void serf_ssl_error_cb_set(
      serf_ssl_context_t *context,
-    serf_ssl_error_cb_t callback,
+    serf_error_cb_t callback,
      void *baton);

  /**
]]]

  The code in ssl_buckets is more or less a search/replace based on the
changes above:
[[[
Index: buckets/ssl_buckets.c
===================================================================
--- buckets/ssl_buckets.c       (revision 1927380)
+++ buckets/ssl_buckets.c       (working copy)
@@ -207,7 +207,7 @@
      EVP_PKEY *cached_cert_pw;

      /* Error callback */
-    serf_ssl_error_cb_t error_callback;
+    serf_error_cb_t error_callback;
      void *error_baton;

      apr_status_t pending_err;
@@ -364,7 +364,7 @@
          if (err && ctx->error_callback) {
              char ebuf[256];
              ERR_error_string_n(err, ebuf, sizeof(ebuf));
-            ctx->error_callback(ctx->error_baton, ctx->fatal_err, ebuf);
+            ctx->error_callback(SERF_SSL_ERROR, ctx->error_baton,
ctx->fatal_err, ebuf);
          }

      }
@@ -1677,7 +1677,7 @@
                  char ebuf[1024];
                  ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
                  apr_snprintf(ebuf, sizeof(ebuf), "could not open URI: %s",
cert_uri);
-                ctx->error_callback(ctx->error_baton, ctx->fatal_err,
ebuf);
+                ctx->error_callback(SERF_SSL_ERROR, ctx->error_baton,
ctx->fatal_err, ebuf);
              }

              break;
@@ -1694,7 +1694,7 @@
                      char ebuf[1024];
                      ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
                      apr_snprintf(ebuf, sizeof(ebuf), "could not read URI:
%s", cert_uri);
-                    ctx->error_callback(ctx->error_baton, ctx->fatal_err,
ebuf);
+                    ctx->error_callback(SERF_SSL_ERROR, ctx->error_baton,
ctx->fatal_err, ebuf);
                  }

                  break;
@@ -1883,9 +1883,9 @@
              if (ctx->error_callback) {
                  char ebuf[1024];
                  apr_snprintf(ebuf, sizeof(ebuf), "could not open PKCS12:
%s", cert_path);
-                ctx->error_callback(ctx->error_baton, ctx->fatal_err,
ebuf);
+                ctx->error_callback(SERF_SSL_ERROR, ctx->error_baton,
ctx->fatal_err, ebuf);
                  apr_strerror(status, ebuf, sizeof(ebuf));
-                ctx->error_callback(ctx->error_baton, ctx->fatal_err,
ebuf);
+                ctx->error_callback(SERF_SSL_ERROR, ctx->error_baton,
ctx->fatal_err, ebuf);
              }
              return -1;
          }
@@ -1970,7 +1970,7 @@
                                  char ebuf[1024];
                                  ctx->fatal_err =
SERF_ERROR_SSL_CERT_FAILED;
                                  apr_snprintf(ebuf, sizeof(ebuf), "could
not parse PKCS12: %s", cert_path);
-                                ctx->error_callback(ctx->error_baton,
ctx->fatal_err, ebuf);
+                                ctx->error_callback(SERF_SSL_ERROR,
ctx->error_baton, ctx->fatal_err, ebuf);
                              }

                              log_ssl_error(ctx);
@@ -1985,7 +1985,7 @@
                      char ebuf[1024];
                      ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
                      apr_snprintf(ebuf, sizeof(ebuf), "PKCS12 needs a
password: %s", cert_path);
-                    ctx->error_callback(ctx->error_baton, ctx->fatal_err,
ebuf);
+                    ctx->error_callback(SERF_SSL_ERROR, ctx->error_baton,
ctx->fatal_err, ebuf);
                  }

                  log_ssl_error(ctx);
@@ -1999,7 +1999,7 @@
                      char ebuf[1024];
                      ctx->fatal_err = SERF_ERROR_SSL_CERT_FAILED;
                      apr_snprintf(ebuf, sizeof(ebuf), "could not parse
PKCS12: %s", cert_path);
-                    ctx->error_callback(ctx->error_baton, ctx->fatal_err,
ebuf);
+                    ctx->error_callback(SERF_SSL_ERROR, ctx->error_baton,
ctx->fatal_err, ebuf);
                  }

                  log_ssl_error(ctx);
@@ -2082,7 +2082,7 @@

  void serf_ssl_error_cb_set(
      serf_ssl_context_t *context,
-    serf_ssl_error_cb_t callback,
+    serf_error_cb_t callback,
      void *baton)
  {
      context->error_callback = callback;
]]]

Would this satisfy Greg's concern about having a more generalised solution?


Cheers,
Daniel