1028764 - Remove some useless declarations

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Sylvestre Ledru [:Sylvestre]]

Attachment: remove-useless-declaration.diff

The attached patches remove some useless/dead declarations.
AFAIK, there is no side effect.

Comment 1

[Kai Engert (:KaiE:)]

Can you please explain WHY exactly you think they are useless declarations? You are removing functional code. It's not immediately clear why this code is useless.

You probably already went through the code to understand why it's unnecessary.

Instead of requiring us to try to repeat your work, and draw the same conclusions as you - could you please save us time, by explaining why these are unnecessary, so that all we have to do is checking that your arguments are reasonable?

Comment 2

[Kai Engert (:KaiE:)]

In particular I'm referring to the variable assignments.

(You don't need to talk about the removed variables.)

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 8444198 [details] [diff] [review]
remove-useless-declaration.diff

>-    if (!doPriv && !doPub) doPriv = PR_TRUE;
>-

ok, because doPriv isn't used after this line.


>-        doKeyGen = PR_TRUE; /* Always do a keygen for session keys.
>-                               Import of hardcoded key is not supported */

ok, only used in the else path, variable not used in remaining code.


>-    maxout -= fullblocks;

ok


>     if (holdingLock) {
> 	PZ_Unlock(blindingParamsList.lock);
>-	holdingLock = PR_FALSE;
>     }

You're right that this isn't really necessary, because it's so close to the end of the function. But for consistency with the remainder of the function, it might be worth it to keep it. Inside this function, the variable is always consistent with the lock, by removing this assignment, you're breaking the rule.

It's a matter of taste. My taste is consistency.


>-    unsigned long controlMask = mod->evControlMask;
>+    unsigned long controlMask;
...
>     mod->evControlMask |= SECMOD_END_WAIT;
>     controlMask = mod->evControlMask;

ok...




> NSSLOWCERTCertificate *
> nsslowcert_FindCertByIssuerAndSN(NSSLOWCERTCertDBHandle *handle, NSSLOWCERTIssuerAndSN *issuerAndSN)
> {
>     SECItem certKey;
>     SECItem *sn = &issuerAndSN->serialNumber;
>     SECItem *issuer = &issuerAndSN->derIssuer;
>     NSSLOWCERTCertificate *cert;
>-    int data_left = sn->len-1;
>+    int data_left;

If this variable is unnecessary outside the following if block, then please remove this line altogether, and declare the variable locally inside the if block.




> nsslowcert_FindTrustByIssuerAndSN(NSSLOWCERTCertDBHandle *handle, 
> 					NSSLOWCERTIssuerAndSN *issuerAndSN)
> {
>     SECItem certKey;
>     SECItem *sn = &issuerAndSN->serialNumber;
>     SECItem *issuer = &issuerAndSN->derIssuer;
>     NSSLOWCERTTrust *trust;
>     unsigned char keyBuf[512];
>-    int data_left = sn->len-1;
>+    int data_left;

same here, please move declaration inside the if block


>diff --git a/lib/softoken/pkcs11c.c b/lib/softoken/pkcs11c.c
>--- a/lib/softoken/pkcs11c.c
>+++ b/lib/softoken/pkcs11c.c
>@@ -6318,17 +6318,16 @@ CK_RV NSC_DeriveKey( CK_SESSION_HANDLE h
> 		** server_write_key[CipherSpec.key_material]
> 		** final_server_write_key = MD5(server_write_key +
> 		**                    ServerHello.random + ClientHello.random);
> 		*/
> 		MD5_Begin(md5);
> 		MD5_Update(md5, &key_block[i], effKeySize);
>             	MD5_Update(md5, srcrdata, sizeof srcrdata);
> 		MD5_End(md5, key_block2, &outLen, MD5_LENGTH);
>-		i += effKeySize;

I'd prefer to keep it for consistency with the surrounding code. It's only unnecessary because there's no other operation involving i following. But removing it could easily introduce an error when future developers do copy/paste and miss this line when adding a following operation.

Matter of taste.


> 	secret.len  = effKeySize;
>-	i          += effKeySize;
> 	keyblk.data = key_block2;
> 	keyblk.len  = keySize;

same consistency argument. I'd keep it.


> 		    PORT_SetError(SEC_ERROR_KEYGEN_FAIL);
>-		    rv = SECFailure;
> 		    break;

You're right this assignment doesn't have an effect, because the break leaves the "for" loop, and line 867 sets it unconditionally to success. However, could you have found a real bug? Do you think the original programmer intended to return an error? If yes, this should be fixed. If no, a comment should be added, why setting an error code with returning success is intended...


> 	    return SECFailure;
> 	}
>-	maxBytes        -= extLen;

ok


>     const PRInt32      limit     = sslCopyLimit;
>     PRBool             blocking;
>-    PRIOVec            myIov     = { 0, 0 };
>+    PRIOVec            myIov;

This function is slightly tricky to read, because of its use of macros. I'd prefer to keep this simple initialization to an apparent safe default, it shouldn't hurt.

Comment 5

[Kai Engert (:KaiE:)]

Comment on attachment 8444198 [details] [diff] [review]
remove-useless-declaration.diff

r- primarily because of the need to clarify the removal of the rv=SECFailure and potential bug here.

Your other changes seem functional correct, but I'd prefer to keep some of them unchanged.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Attachment: patch v2

I agree with all your comments.
I will report a new bug for the rv=SECFailure issue.

Comment 7

[Kai Engert (:KaiE:)]

Comment on attachment 8490850 [details] [diff] [review]
patch v2

r=kaie

I'll remove the unnecessary whitespace change in derive.c

Your changes to file rsa.c are no longer necessary, it seems this has already been cleaned up in the meantime.

Comment 8

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/9f358fbaae22