dsahlberg-apache-org commented on code in PR #9:
URL: https://github.com/apache/serf/pull/9#discussion_r2208642925


##########
buckets/ssl_buckets.c:
##########
@@ -1877,7 +1915,7 @@ static int ssl_need_client_cert(SSL *ssl, X509 **cert, 
EVP_PKEY **pkey)
             return 1;
         }
         else {
-            int err = ERR_get_error();
+            err = ERR_get_error();
             ERR_clear_error();

Review Comment:
   Why moving the declaration from here to the top of the function? Better keep 
scope limited whenever possible, just to catch accidental errors (pun intended).
   (Yes it obviously should be unsigned long instead of int, so a change would 
be needed anyhow).
   



##########
buckets/ssl_buckets.c:
##########
@@ -353,10 +357,17 @@ 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())) {
+
+        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);

Review Comment:
   Is it really necessary to use an internal char array and calling 
ERR_error_string_n to copy the error message to this buffer. The error_callback 
must copy the message to an application internal buffer anyway. Wouldn't it be 
enough to:
   
   char *ebuf = ERR_error_string(err, NULL);
   ctx->error_callback(ctx->error_baton, ctx->fatal_err, ebuff);
   



-- 
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

Reply via email to