On Mon, 2011-08-08 at 07:51 -0400, Pavel Brezina wrote:
https://fedorahosted.org/sssd/ticket/925
[PATCH 1/2] There are several new macros in util/util.h:
- SSSDBG_* to reflect a debug level (same names as in the ticket) -
please, don't use magic numbers anymore
- DEBUG_MSG(level, function, message) which will format the debug
message like "(time) [prg_name] [function] (level): message\n"
- DEBUG_IS_SET(level) that you should use to check if the level is
allowed to be logged You can use it like: if
(DEBUG_IS_SET(SSSDBG_TRACE_LIBS)) {...}
For the most part, this patch is good (one minor nitpick: the comment
around DEBUG_MSG identifies the DEBUG macro, not the DEBUG_MSG macro)
However, it is very difficult to review. Please break this patch up into
several:
1) Creation of the new levels and the macros for converting the old
versions to them.
2) Modification of the DEBUG() macro and creation of the DEBUG_MSG()
macro
3) Conversion of the old DEBUG macros to the new levels
4) The unit tests.
This will make it much more digestible for the reviewer.
[PATCH 2/2] Changes unresolved debug level value (SSSDBG_UNRESOLVED)
from -1 to 0 so DEBUG macro could be reduced by one condition. Anyway,
it has a minor effect, every time you want to load debug_level from
command line parameters, you have to use following pattern:
/* Set debug level to invalid value so we can deside if -d 0 was
used. */ debug_level = SSSDBG_INVALID;
pc = poptGetContext(argv[0], argc, argv, long_options, 0);
while((opt = poptGetNextOpt(pc)) != -1) { ... }
debug_level = debug_level != SSSDBG_INVALID ?
debug_convert_old_level(debug_level) :
SSSDBG_UNRESOLVED; /* Debug level should be loaded from config file. */
I don't actually see any value to this patch. Also, wouldn't this
conflict with compatibility with old settings if 'debug_level = 0' had
been explicitly set?