On (23/11/15 07:30), Petr Cech wrote:
On 11/20/2015 06:28 PM, Lukas Slebodnik wrote:
>On (20/11/15 16:10), Petr Cech wrote:
>>Hi,
>>
>>there is patch resolving
>>https://fedorahosted.org/sssd/ticket/2808
>>
>>Regards
>>
>>Petr
>
>>From 576ca8774302e90af4bc3bdfca73d4f91cef034e Mon Sep 17 00:00:00 2001
>>From: Petr Cech <pcech(a)redhat.com>
>>Date: Fri, 20 Nov 2015 10:06:56 -0500
>>Subject: [PATCH] DEBUG: Add text tag after hexadecimal debug level
>>
>>This patch adds text tag after debug level in logs.
>>
>>Resolves:
>>https://fedorahosted.org/sssd/ticket/2808
>>---
>>src/tests/debug-tests.c | 15 +++++++------
>>src/util/debug.c | 57 +++++++++++++++++++++++++++++++++++++++++++------
>>src/util/util.h | 1 +
>>3 files changed, 61 insertions(+), 12 deletions(-)
>>
>>diff --git a/src/util/debug.c b/src/util/debug.c
>>index
a8eea32740155ec3daf6be71ef9a8af6592f74a9..059f38bb568d1eee1eebc05cc57317e21743bde6 100644
>>--- a/src/util/debug.c
>>+++ b/src/util/debug.c
>>@@ -46,6 +46,48 @@ int debug_to_stderr = 0;
>>const char *debug_log_file = "sssd";
>>FILE *debug_file = NULL;
>>
>>+const char *str_debug_level(int level) {
>This function is not tested.
>We need to ensure that new string will be added if
>new debug level will be created.
Hi Lukas,
yes, there is no CI test for this function. The SSSDBG_* levels are constants
defined at src/util/util.h. If somebody add something new there is no way how
to trace it. I welcome your advice!
Debug levels are constants with type int.
They are not ordinary constants. They are constants with only one bit set.
One bit per constant.
>
>>+ const char *message = NULL;
>>+
>>+ switch(level) {
>>+ case SSSDBG_FATAL_FAILURE:
>>+ message = "ERR_FATAL_FAILURE";
>>+ break;
>>+ case SSSDBG_CRIT_FAILURE:
>>+ message = "ERR_CRIT_FAILURE";
>>+ break;
>>+ case SSSDBG_OP_FAILURE:
>>+ message = "ERR_OP_FAILURE";
>>+ break;
>>+ case SSSDBG_MINOR_FAILURE:
>>+ message = "ERR_MINOR_FAILURE";
>>+ break;
>>+ case SSSDBG_CONF_SETTINGS:
>>+ message = "INFO_CONF_SETTINGS";
>>+ break;
>>+ case SSSDBG_FUNC_DATA:
>>+ message = "INFO_FUNC_DATA";
>>+ break;
>>+ case SSSDBG_TRACE_FUNC:
>>+ message = "TRACE_FUNC";
>>+ break;
>>+ case SSSDBG_TRACE_LIBS:
>>+ message = "TRACE_LIBS";
>>+ break;
>>+ case SSSDBG_TRACE_INTERNAL:
>>+ message = "TRACE_INTERNAL";
>>+ break;
>>+ case SSSDBG_TRACE_ALL:
>>+ message = "TRACE_ALL";
>>+ break;
>>+ case SSSDBG_BE_FO:
>>+ message = "FAIL_OVER";
>>+ break;
>>+ }
Default case could be there, with log message, that there is something new.
Is it enough?
No.
It should be covered in unit test and not in code itself.
>>+
>>+ return message;
>>+}
>>+
>>errno_t set_debug_file_from_fd(const int fd)
>>{
>> FILE *dummy;
>>@@ -252,18 +294,21 @@ void debug_fn(const char *file,
>> memcpy(datetime, ctime(&tv.tv_sec), 19);
>> datetime[19] = '\0';
>> if (debug_microseconds) {
>>- debug_printf("(%s:%.6ld %d) [%s] [%s] (%#.4x): ",
>>+ debug_printf("(%s:%.6ld %d) [%s] [%s] (%#.4x | %s): ",
> ^^^^^^^^^^^^
> This change would complicate
> filtering of messages based on hexadecmal number.
> The expectation was that debug level was between
> round brackets.
Right. I can rewrite it to form
(%#.4x) (%s)
And I need to say I am not still happy with that.
Me neither.
IMHO the tesxt representation of debuglevel does not improve anything.
Reasons:
a) approximately 50% of codes were converted from nemerical value to constants.
So most of messages does not correspond with debug constant
b) String representation of debug_level decrease readability of log.
Currently all debug messages starts at the same column. After your change
they will not. If the purpose of this patch is to simplify reading of logs
by human then the result will be opposite
c) the size of logs will be increased.
There is another thread on
list about it.
So, in the future it might be better to wait till the resolution in
another
thread and do not send patch without it.
LS