864982 - Remove legacy err_msg/buginf/notice_msg logging in SIPCC

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Adam Roach [:abr]]

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 1

[Adam Roach [:abr]]

Attachment: Remove legacy err_msg/buginf/notice_msg logging

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 3

[Adam Roach [:abr]]

Attachment: Remove legacy err_msg/buginf/notice_msg logging

Comment 4

[Adam Roach [:abr]]

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.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 6

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9defb9661f84

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9defb9661f84