Bug 1253161 - Remove unused or unuseful macros in libssl

Tim, see https://codereview.appspot.com/291500043

[ttaubert, 2016-09-19 09:24:21]

https://codereview.appspot.com/307290043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (left):

https://codereview.appspot.com/307290043/diff/1/lib/ssl/ssl3con.c#oldcode11014
lib/ssl/ssl3con.c:11014: SEND_ALERT
So, no alert? Would we then just hang if we fail here?

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

https://codereview.appspot.com/307290043/diff/1/lib/ssl/ssl3con.c#newcode6302
lib/ssl/ssl3con.c:6302: /* Assert? */
What about the "Assert?" comment? That seems as useful as the defines you
removed :)

https://codereview.appspot.com/307290043/diff/1/lib/ssl/ssl3con.c#newcode6939
lib/ssl/ssl3con.c:6939: PORT_SetError(SEC_ERROR_UNSUPPORTED_KEYALG);
Is this even a SEC_ERROR_LIBRARY_FAILURE as we shouldn't get here?

[mt, 2016-09-19 10:16:51]

Fixed up.

https://codereview.appspot.com/307290043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (left):

https://codereview.appspot.com/307290043/diff/1/lib/ssl/ssl3con.c#oldcode11014
lib/ssl/ssl3con.c:11014: SEND_ALERT
On 2016/09/19 09:24:20, ttaubert wrote:
> So, no alert? Would we then just hang if we fail here?

Done.

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

https://codereview.appspot.com/307290043/diff/1/lib/ssl/ssl3con.c#newcode6302
lib/ssl/ssl3con.c:6302: /* Assert? */
On 2016/09/19 09:24:20, ttaubert wrote:
> What about the "Assert?" comment? That seems as useful as the defines you
> removed :)

PORT_Assert(0) added.

https://codereview.appspot.com/307290043/diff/1/lib/ssl/ssl3con.c#newcode6939
lib/ssl/ssl3con.c:6939: PORT_SetError(SEC_ERROR_UNSUPPORTED_KEYALG);
On 2016/09/19 09:24:20, ttaubert wrote:
> Is this even a SEC_ERROR_LIBRARY_FAILURE as we shouldn't get here?

PORT_Assert(0) added.  And LIBRARY_FAILURE.

[ttaubert, 2016-09-19 11:00:20]

LGTM, but please take a look at the alert again. If you say we don't need one
that's fine with me.

https://codereview.appspot.com/307290043/diff/1/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (left):

https://codereview.appspot.com/307290043/diff/1/lib/ssl/ssl3con.c#oldcode11014
lib/ssl/ssl3con.c:11014: SEND_ALERT
On 2016/09/19 10:16:51, mt wrote:
> On 2016/09/19 09:24:20, ttaubert wrote:
> > So, no alert? Would we then just hang if we fail here?
> 
> Done.

Does "done" mean you considered adding an alert but think we don't need one? :)

[mt, 2016-09-19 11:05:45]

On 2016/09/19 11:00:20, ttaubert wrote:
> Does "done" mean you considered adding an alert but think we don't need one?
:)

I thought that I added one...

https://codereview.appspot.com/307290043/diff2/1:20001/lib/ssl/ssl3con.c#new-...

[ttaubert, 2016-09-19 11:09:14]

On 2016/09/19 11:05:45, mt wrote:
> On 2016/09/19 11:00:20, ttaubert wrote:
> > Does "done" mean you considered adding an alert but think we don't need one?
> :)
> 
> I thought that I added one...
> 
>
https://codereview.appspot.com/307290043/diff2/1:20001/lib/ssl/ssl3con.c#new-...

Hmm, it's not showing up in the delta diff view [1]. Maybe a Rietveld bug?

[1] https://codereview.appspot.com/307290043/diff/1/lib/ssl/ssl3con.c