968338 - SSLServerCertVerification.cpp:284:21: warning: private field 'mFdForLogging' is not used [Wunused-private-field]

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Daniel Holbert [:dholbert]]

Clang warning that we're currently hitting on opt Mac TBPL builds (which use clang):
{ 
security/manager/ssl/src/SSLServerCertVerification.cpp:284:21: warning: private field 'mFdForLogging' is not used [Wunused-private-field]
}

This mFdForLogging variable is only used in a PR_LOG statement, which is #defined away in opt builds, leaving the variable unused.

We could play some tricks by #ifdeffing around the variable, but given that there's another variable of the same name in the same file which *is* used in opt builds (by passing it as a function-arg), I think it's probably easier/saner to just pass this unused version to 'unused <<' (alongside the PR_LOG call, so that we'll notice to drop the unused << and the variable if we ever drop the PR_LOG).

Patch coming up to do that.

Comment 1

[Daniel Holbert [:dholbert]]

(FWIW, here's a log from a Try run, with this directory marked as FAIL_ON_WARNINGS in a separate local patch, tripping over this build warning:
 https://tbpl.mozilla.org/php/getParsedLog.php?id=34095391&tree=Try)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8370912 [details] [diff] [review]
fix v1

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

Honestly, I'm not even sure mFdForLogging is useful. I say we just remove it.

Comment 4

[Daniel Holbert [:dholbert]]

Just this CertErrorRunnable::mFdForLogging, or the other one too? (SSLServerCertVerificationJob::mFdForLogging)

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

mFdForLogging is useful for reading logs to see which certificate verification is tied to which connection. Please don't remove it.

Can't we use DebugOnly<T> for this?

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #5)
> Can't we use DebugOnly<T> for this?

No. DebugOnly is #ifdef DEBUG, whereas the usages of this variable are #ifdef PR_LOGGING.

If you'd like, I can mark this variable as #ifdef PR_LOGGING though.

(I had an earlier patch to do that, and then I tried applying it to SSLServerCertVerificationJob's variable as well, and got a build error, because SSLServerCertVerificationJob passes its mFdForLogging as a function-arg in at least one spot. So rather than having two classes one of which has an #ifdeffed variable and the other has a non-#ifdeffed variable, I opted for this unused << solution.)

Comment 7

[Daniel Holbert [:dholbert]]

(TBPL would be happy with a DebugOnly<> solution, because our debug builds have PR_LOGGING defined and our opt builds do not. That'd just be luck, though, and we'd run into trouble if anyone tried to turn on logging in an opt build.)

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8370912 [details] [diff] [review]
fix v1

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

Thanks for the explanation.

Comment 9

[Daniel Holbert [:dholbert]]

Thanks for the review!

I noticed mercurial wanted to touch a few additional lines when I was preparing to push this, and it turned out it was just auto-fixing DOS newlines in this file. (might be due to something in my .hgrc, not sure)  Rather than letting it auto-lump those fixes into this cset, I did a quick "fromdos" run to fix those lines on their own:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/4dc3ca14787f
and then landed this bug's actual patch:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/214c6b4f03c7

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Thanks for the newline fixes. Unfortunately I use MS Visual Studio as an editor, which works great except it doesn't have a "Unix Newlines By Default" option. I will figure out a solution to reduce the chances of me adding more DOS newlines into my patches.

Comment 11

[Daniel Holbert [:dholbert]]

(http://stackoverflow.com/questions/3802406/configure-visual-studio-with-unix-end-of-lines has a suggestion for that, though maybe it's more subtle than that.)

Also: I could've sworn this file already had an #include for "unused.h", but apparently it doesn't (and hence comment 9 busted inbound).  Oops. Pushed a followup to add the #include and fix the bustage:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/f0eb0b8c20d0

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #11)
> Also: I could've sworn this file already had an #include for "unused.h", but
> apparently it doesn't (and hence comment 9 busted inbound).  Oops.

Aha! I'm not crazy. I *did* have an #include for "unused.h" **in my local copy of this file**, because I initially stacked this on top of the first version of my fix for bug 968348 (which added this #include, to mark an unrelated local variable as unused).

The problem was that I wrote the patches in that order, but then pushed this one first. :) In any case, all's well now.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/214c6b4f03c7
https://hg.mozilla.org/mozilla-central/rev/f0eb0b8c20d0