brainy commented on code in PR #9:
URL: https://github.com/apache/serf/pull/9#discussion_r2178729710
##########
buckets/ssl_buckets.c:
##########
@@ -334,10 +338,20 @@ detect_renegotiate(const SSL *s, int where, int ret)
static void log_ssl_error(serf_ssl_context_t *ctx)
{
- unsigned long e = ERR_get_error();
- serf__log(LOGLVL_ERROR, LOGCOMP_SSL, __FILE__, ctx->config,
- "SSL Error: %s\n", ERR_error_string(e, NULL));
+ unsigned long err;
+
+ while ((err = ERR_get_error())) {
+
+ serf__log(LOGLVL_ERROR, LOGCOMP_SSL, __FILE__, ctx->config,
+ "SSL Error: %s\n", ERR_error_string(err, NULL));
+ if (err && ctx->error_callback) {
+ char ebuf[256];
+ ERR_error_string_n(err, ebuf, sizeof(ebuf));
Review Comment:
Just above, you call `ERR_error_string()` to log the error, while here, you
copy the string to the stack. Is this actually necessary? I'd just document to
callback implementors that they have to copy the string inside the callback and
call `ERR_error_string()` just once.
##########
buckets/ssl_buckets.c:
##########
@@ -1652,22 +1658,32 @@ static int ssl_need_client_cert(SSL *ssl, X509 **cert,
EVP_PKEY **pkey)
}
return 1;
}
+ else {
+ err = ERR_get_error();
+ ERR_clear_error();
Review Comment:
You don't pass `err` to `log_ssl_error()` and clear it here; doesn't that
mean that nothing will happen? Why even call `ERR_get_error()` here, given that
it has no effect?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@serf.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
