Avoiding extra processing in darwinssl when not in verbose #1246
Conversation
The information extracted from the server certificates in step 3 is only used when in verbose mode, and there is no error handling or validation performed as that has already been done. Only run the certificate information extraction when in verbose mode and avoid the extra processing with the CURL_DISABLE_VERBOSE_STRINGS macro when not in verbose mode.
When not running in verbose mode, the helper functions for human readable translations are unused and cause a compiler warning. Contain the helpers with the verbose mode macro to only compile them when needed.
|
@danielgustafsson, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickzman, @bagder and @ldx to be potential reviewers. |
lib/vtls/darwinssl.c
Outdated
| @@ -364,7 +365,9 @@ CF_INLINE const char *SSLCipherNameForNumber(SSLCipherSuite cipher) | |||
| } | |||
| return "SSL_NULL_WITH_NULL_NULL"; | |||
| } | |||
| #endif /* CURL_DISABLE_VERBOSE_STRINGS */ | |||
There was a problem hiding this comment.
I think you can remove lines 368 and 370, so both inline functions are under the same preprocessor check. Otherwise, this looks fine to me.
This commit should be squashed into the previous one before pushing if the PR is accepted.
|
On 05 Feb 2017, at 21:11, Nick Zitzmann ***@***.***> wrote:
@nickzman commented on this pull request.
In lib/vtls/darwinssl.c:
> @@ -364,7 +365,9 @@ CF_INLINE const char *SSLCipherNameForNumber(SSLCipherSuite cipher)
}
return "SSL_NULL_WITH_NULL_NULL";
}
+#endif /* CURL_DISABLE_VERBOSE_STRINGS */
I think you can remove lines 368 and 370, so both inline functions are under the same preprocessor check. Otherwise, this looks fine to me.
Pushed a fixup commit for this.
|
|
step3 is a standard function i can see being added to in the future, and I'm concerned the way this is done could lead us to make mistakes by the reviewer not realizing almost the whole thing is guarded in ndef CURL_DISABLE_VERBOSE_STRINGS. It is almost 100 lines. Can we just move that whole thing into a separate function void show_verbose_cert_info or something and then make step3 look like this darwinssl_connect_step3(struct connectdata *conn,int sockindex)
{
struct Curl_easy *data = conn->data;
struct ssl_connect_data *connssl = &conn->ssl[sockindex];
#ifndef CURL_DISABLE_VERBOSE_STRINGS
show_verbose_cert_info(conn, sockindex);
#endif
connssl->connecting_state = ssl_connect_done;
} |
|
On 05 Feb 2017, at 22:43, Jay Satiro ***@***.***> wrote:
step3 is a standard function i can see being added to in the future, and I'm concerned the way this is done could lead us to make mistakes by the reviewer not realizing almost the whole thing is guarded in ndef CURL_DISABLE_VERBOSE_STRINGS. It is almost 100 lines. Can we just move that whole thing into a separate function void show_verbose_cert_info or something and then make step3 look like this
darwinssl_connect_step3(struct connectdata *conn,int
sockindex)
{
struct
Curl_easy *data = conn->data;
struct
ssl_connect_data *connssl = &conn->ssl[sockindex];
#
ifndef
CURL_DISABLE_VERBOSE_STRINGS
show_verbose_cert_info
(conn, sockindex);
#
endif
connssl->connecting_state = ssl_connect_done;
}
That’s a fair point, it does assist reviewing without complicating hacking. I’ll fix a version of this which isolates the code in a verbose function and then we can see which option is best.
|
Addressing review comments, should be squashed with previous commits before pushing if the PR is accepted.
|
@jay pushed an update which pulls the cert printing into a separate static void method. |
|
I modified this slightly and just landed it. Just to clarify Judging by your commit message it looked a lot like what you also wanted to do is stop the extra processing if the user did not enable verbose mode, not just verbose strings. Verbose mode is a different matter, for example the infof calls check for verbose mode and only print if it's enabled. so in infof there's something like this if(data->set.verbose)
show_verbose_server_cert(conn, sockindex);also since your ndef macros end after a large span I changed those endif lines to end with Thanks! |
The information extracted from the server certificates in step 3 is only used when in verbose mode, and there is no error handling or validation performed as that has already been done. Only run the certificate information extraction when in verbose mode and avoid the extra processing with the CURL_DISABLE_VERBOSE_STRINGS macro when not in verbose mode. No huge optimization but might as well not spend extra memory operations when not needed.
Also includes a commit to fix a compiler warning on unused functions when not in verbose mode.