Only generate shares we need

Only generate shares we need

[ekr-rietveld, 2016-07-01 15:56:43]

LGTM

https://codereview.appspot.com/299480043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right):

https://codereview.appspot.com/299480043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_dhe_unittest.cc:47: while (i < shares.len()) {
This is kind of ugly. I think storing the overall length in a temp would be
clearer because then you could work from 2.

https://codereview.appspot.com/299480043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_dhe_unittest.cc:55: }
You should enhance these tests to validate that there is no duplication and that
shares is a subset of supported_groups.

Also, you need a test where both EC and DHE are on.

https://codereview.appspot.com/299480043/diff/1/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/299480043/diff/1/lib/ssl/tls13con.c#newcode363
lib/ssl/tls13con.c:363: return rv;
juberti but return SECFailure

Actually, I tend to think we would be happier returning closer to the call site.
It means duplication of the test, but it's cleaner. 

Also, can I suggest default: PORT_Asset(0)

[mt, 2016-07-11 06:20:49]

I made the requested changes, except as noted.

https://codereview.appspot.com/299480043/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right):

https://codereview.appspot.com/299480043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_dhe_unittest.cc:47: while (i < shares.len()) {
On 2016/07/01 15:56:43, ekr-webrtc wrote:
> This is kind of ugly. I think storing the overall length in a temp would be
> clearer because then you could work from 2.

Not sure what you are thinking here.  Storing the overall length only replaces
instances of shares.len().

Do you think that this is better?

size_t i;
for (i = 2; i < shares.len(); i += 4 + tmp) {
  ASSERT_TRUE(shares.Read(i, 2, &tmp));
  assert_group(static_cast<uint16_t>(tmp));
  ASSERT_TRUE(shares.Read(i + 2, 2, &tmp));
}
EXPECT_EQ(i, shares.len());

https://codereview.appspot.com/299480043/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_dhe_unittest.cc:55: }
On 2016/07/01 15:56:43, ekr-webrtc wrote:
> You should enhance these tests to validate that there is no duplication and
that
> shares is a subset of supported_groups.
> 
> Also, you need a test where both EC and DHE are on.

Done.

https://codereview.appspot.com/299480043/diff/1/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/299480043/diff/1/lib/ssl/tls13con.c#newcode363
lib/ssl/tls13con.c:363: return rv;
On 2016/07/01 15:56:43, ekr-webrtc wrote:
> juberti but return SECFailure
>
> Actually, I tend to think we would be happier returning closer to the call
site.
> It means duplication of the test, but it's cleaner. 

Done.
 
> Also, can I suggest default: PORT_Asset(0)

The compiler will pick this up (I checked) because the switch is exhaustive for
the enumeration.