Regularize some errors thrown by SecureContext.

Regularize some errors thrown by SecureContext.

This is done by replacing a call to SSL_CTX_set_client_CA_list() with
calls to SSL_CTX_add_client_CA(), preserving the diagnostic status
return value discarded by the former.

R=iposva@google.com, whesse@google.com

Committed: https://github.com/dart-lang/sdk/commit/f6ddb4568f81503329c28afdcbd232e78c4764f3

[zra, 2016-03-02 20:58:12]

Description was changed from

==========
Regularize some errors thrown by SecureContext.

This is done by replacing calls to SSL_CTX_set_client_CA_list() with
calls to SSL_CTX_add_client_CA(), preserving the diagnostic status
return value discarded by the former.
==========

to

==========
Regularize some errors thrown by SecureContext.

This is done by replacing a call to SSL_CTX_set_client_CA_list() with
calls to SSL_CTX_add_client_CA(), preserving the diagnostic status
return value discarded by the former.
==========

[zra, 2016-03-02 20:58:38]

zra@google.com changed reviewers:
+ iposva@google.com, whesse@google.com

[Ivan Posva, 2016-03-02 21:56:50]

-Ivan

https://codereview.chromium.org/1761583002/diff/1/runtime/bin/secure_socket.cc
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/1761583002/diff/1/runtime/bin/secure_socket.c...
runtime/bin/secure_socket.cc:966: if (TryPKCS12(status != 0)) {
Can we please rewrite this to be less backwards to read?

[zra, 2016-03-02 23:12:34]

https://codereview.chromium.org/1761583002/diff/1/runtime/bin/secure_socket.cc
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/1761583002/diff/1/runtime/bin/secure_socket.c...
runtime/bin/secure_socket.cc:966: if (TryPKCS12(status != 0)) {
On 2016/03/02 21:56:50, Ivan Posva wrote:
> Can we please rewrite this to be less backwards to read?

Done.

[Ivan Posva, 2016-03-02 23:22:15]

-Ivan

https://codereview.chromium.org/1761583002/diff/20001/runtime/bin/secure_sock...
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/1761583002/diff/20001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:740: if ((status == 0) && TryPKCS12()) {
This would read even better (at least for me) and avoids the extra elsif.

if (status == 0) {
  if (TryPKCS12()) {
    ...
  }
} else {
  ERR_clear_error();
}

https://codereview.chromium.org/1761583002/diff/20001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:905: return NULL;
Returning NULL in a function typed as returning an int.

https://codereview.chromium.org/1761583002/diff/20001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:950: if ((ERR_GET_LIB(err) != ERR_LIB_PEM) ||
Isn't this essemtially TryPKCS12()?

[zra, 2016-03-03 00:06:24]

https://codereview.chromium.org/1761583002/diff/20001/runtime/bin/secure_sock...
File runtime/bin/secure_socket.cc (right):

https://codereview.chromium.org/1761583002/diff/20001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:740: if ((status == 0) && TryPKCS12()) {
On 2016/03/02 23:22:15, Ivan Posva wrote:
> This would read even better (at least for me) and avoids the extra elsif.
> 
> if (status == 0) {
>   if (TryPKCS12()) {
>     ...
>   }
> } else {
>   ERR_clear_error();
> }

Done.

https://codereview.chromium.org/1761583002/diff/20001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:905: return NULL;
On 2016/03/02 23:22:15, Ivan Posva wrote:
> Returning NULL in a function typed as returning an int.

Done.

https://codereview.chromium.org/1761583002/diff/20001/runtime/bin/secure_sock...
runtime/bin/secure_socket.cc:950: if ((ERR_GET_LIB(err) != ERR_LIB_PEM) ||
On 2016/03/02 23:22:15, Ivan Posva wrote:
> Isn't this essemtially TryPKCS12()?

Renamed TryPKCS12 to NoPEMStartLine, and used it instead.

[Ivan Posva, 2016-03-03 00:19:46]

LGTM from my side. Maybe wait for Bill's assessment too?

Thanks,
-Ivan

[zra, 2016-03-04 15:44:46]

@whesse did you have any comments? Thanks!

[Bill Hesse, 2016-03-04 15:51:50]

lgtm

Lgtm. I think you could combine the two if statements around line 620, if the
comment goes before then.

[zra, 2016-03-04 17:02:23]

Description was changed from

==========
Regularize some errors thrown by SecureContext.

This is done by replacing a call to SSL_CTX_set_client_CA_list() with
calls to SSL_CTX_add_client_CA(), preserving the diagnostic status
return value discarded by the former.
==========

to

==========
Regularize some errors thrown by SecureContext.

This is done by replacing a call to SSL_CTX_set_client_CA_list() with
calls to SSL_CTX_add_client_CA(), preserving the diagnostic status
return value discarded by the former.

R=iposva@google.com, whesse@google.com

Committed:
https://github.com/dart-lang/sdk/commit/f6ddb4568f81503329c28afdcbd232e78c4764f3
==========

[zra, 2016-03-04 17:02:24]

Committed patchset #3 (id:40001) manually as
f6ddb4568f81503329c28afdcbd232e78c4764f3 (presubmit successful).

[zra, 2016-03-04 17:03:41]

On 2016/03/04 15:51:50, Bill Hesse wrote:
> Lgtm. I think you could combine the two if statements around line 620, if the
> comment goes before then.

Thanks. I left it as-is to be consistent with the similar code in other places.