Closed
Bug 968348
Opened 10 years ago
Closed 10 years ago
Fix some 'unused' build warnings in security/manager/ssl/src/
Categories
(Core :: Security, defect)
Core
Security
Tracking
()
RESOLVED
FIXED
mozilla30
People
(Reporter: dholbert, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
4.40 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
5.48 KB,
patch
|
Details | Diff | Splinter Review |
In a Try run with security/manager/ssl/src tagged as FAIL_ON_WARNINGS, I ran across a few "unused variable" build warnings there (treated as errors in my Try run). In particular, from the Linux builds: { ../../../../../security/manager/ssl/src/SSLServerCertVerification.cpp:505:13: error: variable 'srv' set but not used [-Werror=unused-but-set-variable] ../../../../../security/manager/ssl/src/nsNSSCertificate.cpp:826:13: error: variable 'srv' set but not used [-Werror=unused-but-set-variable] } https://tbpl.mozilla.org/php/getParsedLog.php?id=34093621&tree=Try ...and from the mac opt build: { ../../../../../../security/manager/ssl/src/nsUsageArrayHelper.cpp:135:16: error: unused variable 'errorString' [-Werror,-Wunused-variable] } https://tbpl.mozilla.org/php/getParsedLog.php?id=34094078&tree=Try Patch coming up to fix these.
Assignee | ||
Comment 1•10 years ago
|
||
We could just drop the 'srv' variables, but I'm assuming they're there so that we can inspect their values in a debugger (and to make it clearer that we know there's a return value, and we're ignoring it. There's an explicit comment to that effect in CreateCertErrorRunnable). I'm not 100% sure whether it's kosher to ignore it in the other spot (in nsNSSCertificate::GetChain), but that's what we're doing currently (and spamming a warning about). And for the 'errorString' one, we can just drop the variable entirely and inline the PR_ErrorToName call (as we do elsewhere in some other logging macros e.g. [1] [2] [3]) [1] http://mxr.mozilla.org/mozilla-central/source/netwerk/cache/nsDiskCacheStreams.cpp#147 [2] http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/ssl/ssl3con.c#7136 [3] http://mxr.mozilla.org/mozilla-central/source/storage/test/test_deadlock_detector.cpp#184
Attachment #8370926 -
Flags: review?(brian)
Assignee | ||
Updated•10 years ago
|
Blocks: buildwarning
Comment on attachment 8370926 [details] [diff] [review] fix v1 Review of attachment 8370926 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/SSLServerCertVerification.cpp @@ +533,5 @@ > // Either it is a failure, which is expected, and we'll process the > // verify log below. > // Or it is a success, then a domain mismatch is the only > // possible failure. > + unused << srv; My suggestion would be to move the comment explaining why we're ignoring the return value of certVerifier.VerifyCert to above the call, and then don't even declare/assign srv. ::: security/manager/ssl/src/nsNSSCertificate.cpp @@ +856,5 @@ > usage, PR_Now(), > nullptr, /*XXX fixme*/ > CertVerifier::FLAG_LOCAL_ONLY, > &nssChain); > + unused << srv; We completely ignore srv here - I don't think we even need to declare/assign it (really what we're looking for is if nssChain gets populated). This function is poorly specified and I intend to remove it/improve the interface in the relatively near future. ::: security/manager/ssl/src/nsUsageArrayHelper.cpp @@ +143,5 @@ > } > > PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, > ("error validating certificate for usage %s: %s (%d) -> %ud \n", > + typestr.get(), PR_ErrorToName(error), (int) error, (int) result)); LGTM.
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8370926 -
Flags: review?(brian) → review?(dkeeler)
Comment on attachment 8370926 [details] [diff] [review] fix v1 Review of attachment 8370926 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the changes I suggested, unless Brian disagrees.
Attachment #8370926 -
Flags: review?(dkeeler) → review+
Oh, Brian reassigned the review. I'll assume he's fine with what I said, then.
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to David Keeler (:keeler) from comment #2) > We completely ignore srv here - I don't think we even need to declare/assign > it (really what we're looking for is if nssChain gets populated). Cool; I'll drop those 'srv' values & move the comment as you suggested. (per comment 1, I was leaving it there in case it's useful to inspect with a debugger; but it sounds like it's not worth it.)
Assignee | ||
Comment 6•10 years ago
|
||
For reference, here's the updated patch that I intend to land, with "qdiff -w" to remove whitespace-only changes (from de-indenting subsequent lines of VerifyCert function-calls)
Assignee | ||
Comment 7•10 years ago
|
||
Try run: https://tbpl.mozilla.org/?tree=Try&rev=e4e130270e6f
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ff203ccf025 (after pushing, I noticed a minor typo in my just-landed commit message: "an inline" should say "and inline". But, not a big enough deal to backout & repush. :))
Flags: in-testsuite-
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6ff203ccf025
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in
before you can comment on or make changes to this bug.
Description
•