922269 - Fix function comments

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Wan-Teh Chang]

Attachment: Fix the "called from ssl3_xxx" comments in ssl3con.c

I am using this bug report to fix function comments that don't warrant their
own bug reports.

The first patch updates the "called from ssl3_xxx" comments in ssl3con.c.
ssl3_HandleServerHelloDone now does the bulk of its work in the new
ssl3_SendClientSecondRound function, so the functions that were called by
ssl3_HandleServerHelloDone directly are now called by ssl3_SendClientSecondRound.
Brian, feel free to reject this patch. (The change to add ssl3_HandleFinished
to the list for ssl3_SendNextProto is worth keeping though.)

Comment 1

[Wan-Teh Chang]

Attachment: Document the ownership of the CERTCertificate objects in CERTCertList

Comment 2

[Ryan Sleevi]

Comment on attachment 812214 [details] [diff] [review]
Document the ownership of the CERTCertificate objects in CERTCertList

Review of attachment 812214 [details] [diff] [review]:
-----------------------------------------------------------------

r- on styleistic nits, but r+ for adding documentation.

::: lib/certdb/cert.h
@@ +1226,5 @@
>  void
>  CERT_RemoveCertListNode(CERTCertListNode *node);
>  
> +/*
> + * The new cert list node takes ownership of |cert|. |cert| is freed

The use of |cert| is a Chromium-ism.

In NSS, "cert" is used (see lines 106-209, 116-117, 123-124, etc)

@@ +1233,5 @@
>  SECStatus
>  CERT_AddCertToListTail(CERTCertList *certs, CERTCertificate *cert);
>  
> +/*
> + * The new cert list node takes ownership of |cert|. |cert| is freed

Same here

@@ +1240,5 @@
>  SECStatus
>  CERT_AddCertToListHead(CERTCertList *certs, CERTCertificate *cert);
>  
>  SECStatus
>  CERT_AddCertToListTailWithData(CERTCertList *certs, CERTCertificate *cert,

Seems like you should document this

Comment 3

[Wan-Teh Chang]

Comment on attachment 812207 [details] [diff] [review]
Fix the "called from ssl3_xxx" comments in ssl3con.c

https://hg.mozilla.org/projects/nss/rev/741d64d4656c

Comment 4

[Wan-Teh Chang]

Attachment: Document the ownership of the CERTCertificate objects in CERTCertList, v2

Ryan: please review the new patch. Thanks.

I remember seeing the use of |cert| in Mozilla code and Bugzilla comments
before the Chromium project existed. It has crept into the NSS source tree
because many NSS developers are familiar with this notation from either
Mozilla or Chromium. I think it is a good notation.

Comment 5

[Wan-Teh Chang]

Comment on attachment 815120 [details] [diff] [review]
Document the ownership of the CERTCertificate objects in CERTCertList, v2

https://hg.mozilla.org/projects/nss/rev/41112f489386

Comment 6

[Kai Engert (:KaiE:)]

changing target milestone to 3.15.4

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, and this bug has priority 'P2'.
:beurdouche, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 8

[Marco Castelluccio [:marco]]

We have modified the bot to only consider P1 as high priority, so I'm cancelling the needinfo here.

Comment 9

[Anna Weine]

I have observed a lot of notation like |something|.

I will close this bug as solved. Feel free to re-open the bug if you consider that it would benefit from some more work.