Closed
Bug 864982
Opened 12 years ago
Closed 12 years ago
Remove legacy err_msg/buginf/notice_msg logging in SIPCC
Categories
(Core :: WebRTC: Signaling, defect)
Core
WebRTC: Signaling
Tracking
()
RESOLVED
FIXED
mozilla23
People
(Reporter: abr, Assigned: abr)
References
Details
(Whiteboard: [WebRTC] [blocking-webrtc-] [qa-])
Attachments
(1 file, 1 obsolete file)
1.16 MB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
This is a follow-on for Bug 855335 and Bug 848173 comment 5. It removes all uses of buginf, buginf_msg, err_msg, and notice_msg in the core code, replacing them with the corresponding CSFLog* macros. The key benefit of doing so is that log messages now emit the correct line numbers. This patch also includes an __attribute__ declaration on the CSFLog function that asks GCC to validate the printf-style format strings in log messages against their parameters, so as to prevent any log message format regressions. Consqeuent to this change, several of the old buginf/buginf_msg/err_msg/notice_msg format strings that weren't fixed in Bug 855335 have been updated.
Comment 2•12 years ago
|
||
Comment on attachment 741010 [details] [diff] [review] Remove legacy err_msg/buginf/notice_msg logging Review of attachment 741010 [details] [diff] [review]: ----------------------------------------------------------------- ehugg's on vacation. The \n's are a nice-to-remove, not mandatory. The message dumps at one line per byte.... I'd rather seem them turned off or put on a higher level (MSGDEBUG = 7), or replaced with something dump that dumps them on a small number of lines (even if it prints trailing garbage). Or snprintf() them into a buffer and log that. I'll r+, but I'd love to see them at least moved to another debug level so one can reasonably set signaling:6 logging ::: media/webrtc/signaling/src/sipcc/core/ccapp/ccprovider.c @@ +2086,5 @@ > thread_ended(THREADMON_CCAPP); > destroy_ccapp_thread(); > break; > default: > + APP_ERR_MSG("CCApp_Task: Error: Unknown message %d msg =%p\n", remove \n's? ::: media/webrtc/signaling/src/sipcc/core/gsm/ccapi.c @@ +74,2 @@ > } > + CSFLogDebug("gsm", "%02x ", *pData++); God, no, please.... :-) One debug per byte. Dump it as a block of %02x's and dump the size, and let a human ignore trailing 0's if need be. Or something. Anything. :-) ::: media/webrtc/signaling/src/sipcc/core/gsm/subapi.c @@ +38,2 @@ > } > + CSFLogDebug("gsm", "%02x ", *pData++); Ditto: please not one line per debug
Attachment #741010 -
Flags: review?(ethanhugg) → review+
Updated•12 years ago
|
Whiteboard: [WebRTC] [blocking-webrtc-]
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #741010 -
Attachment is obsolete: true
Assignee | ||
Comment 4•12 years ago
|
||
Comment on attachment 741394 [details] [diff] [review] Remove legacy err_msg/buginf/notice_msg logging I've removed somewhere just shy of 2000 trailing "\n"'s from the logging messages. There are somewhere between 500 and 1000 such instances remaining in the code, but they aren't as easy to fix in an automated fashion, and I think the improvements are hitting a level of diminishing returns as compared to the effort required to fix them. The interesting changes here (and the ones I'm asking for re-review on) are in ccapi.c and subapi.c (along with some supporting changes in CSFLog.h). In a nutshell, we're formatting the bytes into lines before emitting them, and we've pushed them down to a new level 7 ("Obnoxious") to keep them out of the way of normal debug logging.
Attachment #741394 -
Flags: review?(rjesup)
Comment 5•12 years ago
|
||
Comment on attachment 741394 [details] [diff] [review] Remove legacy err_msg/buginf/notice_msg logging Review of attachment 741394 [details] [diff] [review]: ----------------------------------------------------------------- r+ modulo the DEBUG/etc issue, and how often. We can also discusse with Ethan later. ::: media/webrtc/signaling/src/sipcc/core/gsm/ccapi.c @@ +78,2 @@ > } > + CSFLogObnoxious("gsm", "%04X %s",row * BYTES_PER_LINE, buffer); So, this code is controlled by sipccLoggingMask in CallControlManagerImpl.cpp (very indirectly) which is 0xFFFFFFFF. That means that this will always format the messages into hex bytes, and then call CSFLogObnoxious, and then not print them out. If this is only in DEBUG builds it's not a (big) issue, but if this is in opt/release builds I'm concerned. Also, the volume of messages is an issue - if it's one or two at startup, that's one thing. If you're pretty sure there's no major issue (for opt/release builds) here, lets land this and maybe consider somethign smarter later.
Attachment #741394 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 6•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9defb9661f84
Comment 7•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9defb9661f84
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Updated•12 years ago
|
Whiteboard: [WebRTC] [blocking-webrtc-] → [WebRTC] [blocking-webrtc-] [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•