cisco / libsrtp

Library for SRTP (Secure Realtime Transport Protocol)
Other
1.21k stars 474 forks source link

logging is way too verbose #110

Closed jnoring closed 7 years ago

jnoring commented 9 years ago

We run a media server that uses libsrtp (see https://github.com/ging/licode), and we'd like to turn on debugging but it's far too verbose to be of any value. I'd really like to see log levels (two would suffice) were we could request verbose/debug logging, or just error logging.

I'm happy to make this change in the library if someone can point me in the general direction that maintainers would prefer?

jnoring commented 9 years ago

Reading more code...there's actually most of the plumbing necessary for this already in place. libsrtp has a notion of a "module" with an on-off setting for the module, and also some notion of log levels:

typedef enum {
  err_level_emergency = 0,
  err_level_alert,
  err_level_critical,
  err_level_error,
  err_level_warning,
  err_level_notice,
  err_level_info,
  err_level_debug,
  err_level_none
} err_reporting_level_t;

The problem is by default logging is turned off in srtp.c (mod_srtp is set to "off"), and there is no method exposed to tweak the error reporting level, nor is there a method exposed to actually turn logging on for a given module.

So a lot of the plumbing is actually there--it's just not particularly coherent. What I'd like to see is the ability to set a global level, and then also set individual module levels (get rid of the on/off switch for modules, and have them use err_reporting_level_t as well).

There is also a compile error if ERR_REPORTING_SYSLOG is enabled (missing a closing param), and a spurrious if(1) call that can be removed.

Lastly, the implementation for err_level_none is unclear to me; does this mean it should print everything, or nothing? Currently the code will print everything if err_level is set to err_level_none because of that entry's position in the enum.

jfigus commented 9 years ago

Agreed, it appears someone had good intentions by defining a wide variety of logging levels. But it was never fully implemented. For a library this size, 9 levels of logging is probably overkill. We can probably reduce this list down to four levels: None, Verbose, Warning and Error.

I would think err_level_none would report nothing.

ibc commented 8 years ago

IMHO a low level library must not log anything. Instead provide an event/listeners based API for the library user to handle debugging info.

jfigus commented 8 years ago

But aren't the debugs disabled by default? The --enable-console option is used to enable logging to the console.

pabuhler commented 7 years ago

Closing this issue as nothing specific will be done based on it. If there are any logging feature requests then please add them to #230