Author: brane
Date: Mon Jan 12 17:00:45 2026
New Revision: 1931269
Log:
On the serf_error_callbacks branch: Add Serf error callback handling
using a context callback with our session as the baton. Assumes that
we use one Serf context per ra_serf session, which is currently true.
[in subversion/libsvn_ra_serf]
* ra_serf.h
(svn_ra_serf__session_t::serf_error): New member.
(svn_ra_serf__wrap_err_stack): New prototype and debug-mode macro.
* serf.c
(sess_error_callback): New; the Serf error callback.
(svn_ra_serf__open, ra_serf_dup_session): Register the callback when
the Serf context is created.
* util.c: Optionally include serf_bucket_util.h; it declares the required
prototype for serf_default_readline().
(conn_setup): When a new Serf SSL context was created, tell it to use
our session's error callback.
(svn_ra_serf__context_run): Report errors collected from Serf.
(bucket_limited_readline): Put this function in a conditional block,
otherwise we get a warning about an unused function when experimental
Serf features are turned on.
* util_error.c: Include stdarg.h.
(format_error_message): New; hoist error mesage formatting from
svn_ra_serf__wrap_err().
(svn_ra_serf__wrap_err): Delegate to format_error_message().
(svn_ra_serf__wrap_err_stack): Implement, using format_error_message().
Modified:
subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/ra_serf.h
subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/serf.c
subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/util.c
subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/util_error.c
Modified:
subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/ra_serf.h
==============================================================================
---
subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/ra_serf.h
Mon Jan 12 16:32:44 2026 (r1931268)
+++
subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/ra_serf.h
Mon Jan 12 17:00:45 2026 (r1931269)
@@ -172,6 +172,10 @@ struct svn_ra_serf__session_t {
/* Error that we've received but not yet returned upstream. */
svn_error_t *pending_error;
+ /* Errors gathered from Serf error callbacks.
+ FIXME: Can we just use pending_error? */
+ svn_error_t *serf_error;
+
/* List of authn types supported by the client.*/
int authn_types;
@@ -1570,14 +1574,23 @@ svn_ra_serf__create_sb_bucket(svn_spillb
apr_pool_t *result_pool,
apr_pool_t *scratch_pool);
-/** Wrap STATUS from an serf function. If STATUS is not serf error code,
- * this is equivalent to svn_error_wrap_apr().
+/* Wrap STATUS from an serf function. If STATUS is not serf error code,
+ * this is equivalent to svn_error_wrap_apr().
*/
svn_error_t *
svn_ra_serf__wrap_err(apr_status_t status,
const char *fmt,
...);
+/* Like svn_ra_serf__wrap_err but also accept a child error to maintain
+ * an error stack.
+ */
+svn_error_t *
+svn_ra_serf__wrap_err_stack(apr_status_t status,
+ svn_error_t *child,
+ const char *fmt,
+ ...);
+
/* Create a bucket that just returns DATA (with length LEN) and then returns
the APR_EAGAIN status */
serf_bucket_t *
@@ -1675,7 +1688,8 @@ svn_ra_serf__default_readline(serf_bucke
/* Wrapper macros to collect file and line information */
#define svn_ra_serf__wrap_err \
(svn_error__locate(__FILE__,__LINE__), (svn_ra_serf__wrap_err))
-
+#define svn_ra_serf__wrap_err_stack \
+ (svn_error__locate(__FILE__,__LINE__), (svn_ra_serf__wrap_err_stack))
#endif
#ifdef __cplusplus
Modified:
subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/serf.c
==============================================================================
--- subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/serf.c
Mon Jan 12 16:32:44 2026 (r1931268)
+++ subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/serf.c
Mon Jan 12 17:00:45 2026 (r1931269)
@@ -481,6 +481,22 @@ get_user_agent_string(apr_pool_t *pool)
major, minor, patch);
}
+#if defined(SVN__SERF_EXPERIMENTAL) && SERF_VERSION_AT_LEAST(1, 5, 0)
+static apr_status_t
+sess_error_callback(void *baton,
+ unsigned source,
+ apr_status_t status,
+ const char *message)
+{
+ const char* prefix = (source & SERF_ERROR_CB_SSL_CONTEXT) ? "TLS" : "NET";
+ svn_ra_serf__session_t *const session = baton;
+
+ session->serf_error = svn_ra_serf__wrap_err_stack(
+ status, session->serf_error, _("%s: %s"), prefix, message);
+ return SVN_NO_ERROR;
+}
+#endif
+
/* Implements svn_ra__vtable_t.open_session(). */
static svn_error_t *
svn_ra_serf__open(svn_ra_session_t *session,
@@ -520,7 +536,17 @@ svn_ra_serf__open(svn_ra_session_t *sess
serf_sess->cancel_baton = callback_baton;
/* todo: reuse serf context across sessions */
+
+ /* odot: For now, it's better to keep context-per-session, otherwise
+ dealing with Serf's error callbacks will be a real pain.
+ It would be different if we could guarantee a single Serf
+ context per svn_client_ctx_t. */
serf_sess->context = serf_context_create(serf_sess->pool);
+#if defined(SVN__SERF_EXPERIMENTAL) && SERF_VERSION_AT_LEAST(1, 5, 0)
+ serf_context_error_callback_set(serf_sess->context,
+ sess_error_callback,
+ serf_sess);
+#endif
SVN_ERR(svn_ra_serf__blncache_create(&serf_sess->blncache,
serf_sess->pool));
@@ -775,6 +801,11 @@ ra_serf_dup_session(svn_ra_session_t *ne
/* conn_latency */
new_sess->context = serf_context_create(result_pool);
+#if defined(SVN__SERF_EXPERIMENTAL) && SERF_VERSION_AT_LEAST(1, 5, 0)
+ serf_context_error_callback_set(new_sess->context,
+ sess_error_callback,
+ new_sess);
+#endif
SVN_ERR(load_config(new_sess, old_sess->config,
result_pool, scratch_pool));
Modified:
subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/util.c
==============================================================================
--- subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/util.c
Mon Jan 12 16:32:44 2026 (r1931268)
+++ subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/util.c
Mon Jan 12 17:00:45 2026 (r1931269)
@@ -31,6 +31,9 @@
#include <serf.h>
#include <serf_bucket_types.h>
+#if defined(SVN__SERF_EXPERIMENTAL) && SERF_VERSION_AT_LEAST(1, 5, 0)
+#include <serf_bucket_util.h>
+#endif
#include "svn_hash.h"
#include "svn_dirent_uri.h"
@@ -549,6 +552,11 @@ conn_setup(apr_socket_t *sock,
ssl_server_cert_cb,
conn);
+#if defined(SVN__SERF_EXPERIMENTAL) && SERF_VERSION_AT_LEAST(1, 5, 0)
+ serf_ssl_use_context_error_callback(conn->ssl_context,
+ conn->session->context);
+#endif
+
/* See if the user wants us to trust "default" openssl CAs. */
if (conn->session->trust_default_ca)
{
@@ -954,16 +962,20 @@ svn_ra_serf__context_run(svn_ra_serf__se
SVN_ERR(err);
if (status)
{
+ svn_error_t *const collected_errors = sess->serf_error;
+ sess->serf_error = NULL;
+
/* ### This omits SVN_WARNING, and possibly relies on the fact that
### MAX(SERF_ERROR_*) < SVN_ERR_BAD_CATEGORY_START? */
if (status >= SVN_ERR_BAD_CATEGORY_START && status < SVN_ERR_LAST)
{
/* apr can't translate subversion errors to text */
- SVN_ERR_W(svn_error_create(status, NULL, NULL),
+ SVN_ERR_W(svn_error_create(status, collected_errors, NULL),
_("Error running context"));
}
- return svn_ra_serf__wrap_err(status, _("Error running context"));
+ return svn_ra_serf__wrap_err_stack(status, collected_errors,
+ _("Error running context"));
}
return SVN_NO_ERROR;
@@ -2159,6 +2171,7 @@ svn_ra_serf__get_dirent_props(apr_uint32
return props;
}
+#if !defined(SVN__SERF_EXPERIMENTAL) || !SERF_VERSION_AT_LEAST(1, 5, 0)
static apr_status_t
bucket_limited_readline(serf_bucket_t *bucket, int acceptable,
apr_size_t requested, int *found,
@@ -2271,6 +2284,7 @@ bucket_limited_readline(serf_bucket_t *b
return status;
}
+#endif
apr_status_t
svn_ra_serf__default_readline(serf_bucket_t *bucket, int acceptable,
Modified:
subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/util_error.c
==============================================================================
---
subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/util_error.c
Mon Jan 12 16:32:44 2026 (r1931268)
+++
subversion/branches/serf_error_callbacks/subversion/libsvn_ra_serf/util_error.c
Mon Jan 12 17:00:45 2026 (r1931269)
@@ -20,6 +20,9 @@
* under the License.
* ====================================================================
*/
+
+#include <stdarg.h>
+
#include <serf.h>
#include "svn_utf.h"
@@ -41,18 +44,16 @@
#undef svn_error_quick_wrap
#undef svn_error_wrap_apr
#undef svn_ra_serf__wrap_err
+#undef svn_ra_serf__wrap_err_stack
-svn_error_t *
-svn_ra_serf__wrap_err(apr_status_t status,
- const char *fmt,
- ...)
-{
- const char *serf_err_msg = serf_error_string(status);
- svn_error_t *err;
- va_list ap;
-
- err = svn_error_create(status, NULL, NULL);
+static void
+format_error_message(svn_error_t *err,
+ apr_status_t status,
+ const char *serf_err_msg,
+ const char *fmt,
+ va_list args)
+{
if (serf_err_msg || fmt)
{
const char *msg;
@@ -78,9 +79,7 @@ svn_ra_serf__wrap_err(apr_status_t statu
/* Append it to the formatted message. */
if (fmt)
{
- va_start(ap, fmt);
- msg = apr_pvsprintf(err->pool, fmt, ap);
- va_end(ap);
+ msg = apr_pvsprintf(err->pool, fmt, args);
}
else
{
@@ -96,6 +95,37 @@ svn_ra_serf__wrap_err(apr_status_t statu
err->message = msg;
}
}
+}
+
+
+svn_error_t *
+svn_ra_serf__wrap_err(apr_status_t status,
+ const char *fmt,
+ ...)
+{
+ svn_error_t *err;
+ va_list args;
+
+ err = svn_error_create(status, NULL, NULL);
+ va_start(args, fmt);
+ format_error_message(err, status, serf_error_string(status), fmt, args);
+ va_end(args);
+ return err;
+}
+
+
+svn_error_t *
+svn_ra_serf__wrap_err_stack(apr_status_t status,
+ svn_error_t *child,
+ const char *fmt,
+ ...)
+{
+ svn_error_t *err;
+ va_list args;
+ err = svn_error_create(status, child, NULL);
+ va_start(args, fmt);
+ format_error_message(err, status, serf_error_string(status), fmt, args);
+ va_end(args);
return err;
}
