1396488 - SSL_Option(Set|Get)(|Default) don't really accept a boolean

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[Martin Thomson [:mt:]]

The value of SSL options is actually an integer (PRIntn), but the header file lists PRBool (which is typedef'd to PRIntn).  There are already cases where we pass a value of 2 or 3.

I've found a case where an integer would be useful and I'm proposing that we change the API official.

Comment 1

[Kai Engert (:KaiE:)]

If you change PRBool to PRIntn, we should retain ABI compatibility.

If old application code, that passes PRBool*, is built against modified headers, will compilers complain?

Comment 2

[Eric Rescorla (:ekr)]

They shouldn't, because it's just a typedef for PRIntn
http://searchfox.org/mozilla-central/source/nsprpub/pr/include/prtypes.h#475

Comment 3

[Martin Thomson [:mt:]]

Comment on attachment 8904131 [details]
Change SSL_Option(Set|Get)(|Default) to use PRIntn

Flagging Kai for review here because he doesn't seem to have an account on the new phab instance yet.

Comment 4

[Kai Engert (:KaiE:)]

I think I created an account, but I'm not getting any email notifications.

Comment 5

[Kai Engert (:KaiE:)]

I'll review, but in addition I've asked Hubert by email to give us feedback, if he can see any potential negative consequences, despite our optimism.

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 8904131 [details]
Change SSL_Option(Set|Get)(|Default) to use PRIntn

I've provided feedback in Phabricator. I've suggested some documentation and debug assertion changes. I'll mark this as reviewed in phabricator after your response.

Comment 7

[Hubert Kario]

for the record: I agree with Eric, it shouldn't cause any issues

Comment 8

[Martin Thomson [:mt:]]

Thanks Hubert.  Kai, I have updated the code, and responded.  The only change is adding comments.  I hope that's OK.

Comment 9

[Martin Thomson [:mt:]]

https://hg.mozilla.org/projects/nss/rev/c3ccbe0e91cf2ed741f649f1b7f10054cfafc2dc

Note that this is on the NSS_TLS13_DRAFT19_BRANCH branch.  Hopefully that will merge at some point.