New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoiding extra processing in darwinssl when not in verbose #1246

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@danielgustafsson
Member

danielgustafsson commented Feb 5, 2017

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.

danielgustafsson added some commits Feb 5, 2017

Avoid parsing certificates when not in verbose mode
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.
Fix compiler warning on unused function
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.
@mention-bot

This comment has been minimized.

mention-bot commented Feb 5, 2017

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

@@ -364,7 +365,9 @@ CF_INLINE const char *SSLCipherNameForNumber(SSLCipherSuite cipher)
}
return "SSL_NULL_WITH_NULL_NULL";
}
#endif /* CURL_DISABLE_VERBOSE_STRINGS */

This comment has been minimized.

@nickzman

nickzman Feb 5, 2017

Collaborator

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.

Put both functions under the same preprocessor block
This commit should be squashed into the previous one before
pushing if the PR is accepted.
@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Feb 5, 2017

@ldx

ldx approved these changes Feb 5, 2017

@jay

This comment has been minimized.

Member

jay commented Feb 5, 2017

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;
}
@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Feb 5, 2017

Extrace verbose-mode cert printing to separate method
Addressing review comments, should be squashed with previous
commits before pushing if the PR is accepted.
@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Feb 6, 2017

@jay pushed an update which pulls the cert printing into a separate static void method.

@nickzman

I have no objections to this change. Thanks for your contribution.

@bagder bagder added the SSL/TLS label Feb 7, 2017

@jay jay closed this in 3509aa8 Feb 7, 2017

@jay

This comment has been minimized.

Member

jay commented Feb 7, 2017

I modified this slightly and just landed it. Just to clarify

#ifndef CURL_DISABLE_VERBOSE_STRINGS
the code here is included if the user builds libcurl with verbose strings, the default.
#endif

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) printf whatever
so we'll call infof always, without checking data->set.verbose
in this case because the processing is heavy I modified your changes slightly to tackle it pre-emptively by checking the verbose mode flag before calling the function. so now it looks 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 /* !FOO */ instead of /* FOO */

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment