242457 - NTLM should use nspr logging instead of printf with a special ifdef

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: like this

I had to rewrite the hexdump function (PrintBuf); nspr would have written each
char on its own line...

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

("rewrite" as in "make slightly less trivial changes than to the rest of the file")

Comment 3

[Darin Fisher]

Comment on attachment 147652 [details] [diff] [review]
like this

>Index: nsNTLMAuthModule.cpp

>+#ifdef MOZ_LOGGING
>+#define FORCE_PR_LOG
>+#endif
>+#include "prlog.h"

I don't know that this needs to be enabled in release builds.  Do you
know of cases where it would be helpful to have this in release builds?

I believe HTTP logging logs the WWW-Authenticate challenges, and so we
can always use netwerk/test/ReadNTLM to generate the same kind of output
that this file produces.

Therefore, my recommendation is to not define FORCE_PR_LOG.


>+#ifdef PR_LOGGING
>+PRLogModuleInfo *gNTLMLog = PR_NewLogModule("NTLM");

If it is indeed valid to instantiate these at file scope, then maybe we
should be doing that everywhere else too.  What do you think?


>+#else
>+#define LOG(x)
>+#define LOG_ENABLED() PR_FALSE
> #endif

I don't think you should need to define LOG_ENABLED outside of PR_LOGGING.
If you do need LOG_ENABLED outside of PR_LOGGING, then it probably means
that you should wrap the conditional in a #ifdef PR_LOGGING anyways.


> static void PrintFlags(PRUint32 flags)
> PrintBuf(const char *tag, const PRUint8 *buf, PRUint32 bufLen)
> static void PrintToken(const char *name, const void *token, PRUint32 tokenLen)

s/Print/Log/ perhaps?  I think these would be better as Log*


r+sr=darin with those changes.	Thanks for writing this patch.

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #3)
> I don't know that this needs to be enabled in release builds.  Do you
> know of cases where it would be helpful to have this in release builds?

sorry... this is a leftover from testing this patch.

> >+#ifdef PR_LOGGING
> >+PRLogModuleInfo *gNTLMLog = PR_NewLogModule("NTLM");
> 
> If it is indeed valid to instantiate these at file scope, then maybe we
> should be doing that everywhere else too.  What do you think?

hm... several modules do this already:
http://lxr.mozilla.org/seamonkey/search?string=PR_NewLogModule

e.g. cookieservice, jpeg decoder, stopwatch.h, parts of content, parts of gfx,
webshell...

> I don't think you should need to define LOG_ENABLED outside of PR_LOGGING.
> If you do need LOG_ENABLED outside of PR_LOGGING, then it probably means
> that you should wrap the conditional in a #ifdef PR_LOGGING anyways.

hm, looks like I only use it inside an ifdef PR_LOGGING block. ok, I'll remove
that define.

> > static void PrintFlags(PRUint32 flags)
> > PrintBuf(const char *tag, const PRUint8 *buf, PRUint32 bufLen)
> > static void PrintToken(const char *name, const void *token, PRUint32 tokenLen)
> 
> s/Print/Log/ perhaps?  I think these would be better as Log*

ok, will do that.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in nsNTLMAuthModule.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNTLMAuthModule.cpp,v  <-- 
nsNTLMAuthModule.cpp
new revision: 1.9; previous revision: 1.8
done

Comment 6

[benc]

V:
(you can verify your own "code change" bugs when you include the checkin info...)