[SSSD] [PATCH] UTIL: Alternative way for debug message initialisation

Lukas Slebodnik lslebodn at redhat.com
Tue Sep 8 13:31:45 UTC 2015


On (08/09/15 15:21), Michal Židek wrote:
>On 09/08/2015 03:09 PM, Lukas Slebodnik wrote:
>>On (08/09/15 14:57), Michal Židek wrote:
>>>On 08/27/2015 12:52 PM, Lukas Slebodnik wrote:
>>>>On (19/08/15 15:57), Michal Židek wrote:
>>>>>On 08/12/2015 01:11 PM, Lukas Slebodnik wrote:
>>>>>>ehlo,
>>>>>>
>>>>>>attached patch shoul avoid situation as in commit 7c69221077c780e62f6c536e78675f2dc1c131bc
>>>>>>The comments does not guarantee anything.
>>>>>>
>>>>>>Author: Michal Zidek <mzidek at redhat.com>
>>>>>>Date:   Tue Mar 10 17:30:48 2015 +0100
>>>>>>
>>>>>>     DEBUG: Add missing strings for error messages
>>>>>>
>>>>>>     We had more error codes than corresponding
>>>>>>     messages. Also order of two messages was wrong.
>>>>>>
>>>>>>Unfortunatelly, the index of enum do not start from 0
>>>>>>and need to be converted using SSSD_ERR_IDX. I used shorter version of
>>>>>>macro "idx". Feel free to propose nicer name.
>>>>>
>>>>>I would use the SSSD_ERR_IDX and not create an alias for it.
>>>>>I think it is always better to have properly namespaced macros
>>>>>even if the are a little longer.
>>>>>
>>>>a) the macro is not in public header.
>>>
>>>It is int util_errors.h
>>>
>>>git grep '#include .*util_errors.h'
>>>src/providers/dp_refresh.c:#include "util/util_errors.h"
>>>src/providers/ipa/ipa_dyndns.h:#include "util/util_errors.h"
>>>src/providers/krb5/krb5_child.c:#include "src/util/util_errors.h"
>>>src/tests/sbus_codegen_tests.c:#include "util/util_errors.h"
>>>src/tests/sbus_tests.c:#include "util/util_errors.h"
>>>src/util/util.h:#include "util/util_errors.h"
>>>        ^^^^^^^^
>>>So it is almost everywhere with the exception of the
>>>client code.
>>>
>>I meant that the local version of SSSD_ERR_IDX is not im public
>>header. So it could not colide with other files.
>>
>>>>b) SSSD_ERR_IDX significantly decrease readability
>>>
>>>Not properly namespacing macros can bite us in the
>>>long term. I do not think that SSSD_ERR_IDX decreases
>>>readability, but even if it was the case I would say
>>>it is simply necessary. Both SSSD_ and ERR_ prefixes are
>>>important here.
>>>
>>
>>OK,
>>try to compare few followinf versions and choose the one which
>>can be easily proceed/understood.
>>
>>a)
>>struct err_string error_to_str[ SSSD_ERR_IDX(ERR_LAST) + 1 ] = {
>>     [SSSD_ERR_IDX(ERR_INVALID) ] = { "Invalid Error" },
>>     [SSSD_ERR_IDX(ERR_INTERNAL) ] = { "Internal Error" },
>>     [SSSD_ERR_IDX(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" },
>>     [SSSD_ERR_IDX(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" },
>>     [SSSD_ERR_IDX(ERR_NO_CREDS) ] = { "No credentials available" },
>>     [SSSD_ERR_IDX(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
>>
>>
>>b)
>>struct err_string error_to_str[ idx(ERR_LAST) + 1 ] = {
>>     [ idx(ERR_INVALID) ] = { "Invalid Error" },
>>     [ idx(ERR_INTERNAL) ] = { "Internal Error" },
>>     [ idx(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" },
>>     [ idx(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" },
>>     [ idx(ERR_NO_CREDS) ] = { "No credentials available" },
>>     [ idx(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
>>
>>
>>c)
>>struct err_string error_to_str[ _(ERR_LAST) + 1 ] = {
>>     [ _(ERR_INVALID) ] = { "Invalid Error" },
>>     [ _(ERR_INTERNAL) ] = { "Internal Error" },
>>     [ _(ERR_ACCOUNT_UNKNOWN) ] = { "Account Unknown" },
>>     [ _(ERR_INVALID_CRED_TYPE) ] = { "Invalid credential type" },
>>     [ _(ERR_NO_CREDS) ] = { "No credentials available" },
>>     [ _(ERR_CREDS_EXPIRED) ] = { "Credentials are expired" },
>>
>>
>>d)
>>struct err_string error_to_str[] = {
>>     { "Invalid Error" },            /* ERR_INVALID */
>>     { "Internal Error" },           /* ERR_INTERNAL */
>>     { "Account Unknown" },          /* ERR_ACCOUNT_UNKNOWN */
>>     { "Invalid credential type" },  /* ERR_INVALID_CRED_TYPE */
>>     { "No credentials available" }, /* ERR_NO_CREDS */
>>     { "Credentials are expired" },  /* ERR_CREDS_EXPIRED */
>>
>>
>>IMHO, the last one is the best, but there is not a compile time check.
>>
>>LS
>
>I would choose a). It may be less readable than d) but has
>compile time checks. Both b) and c) are not properly namespaced.
>
I would rather used option d) than option a). (Do not change anything)

Option b) does not require namespacing, because it is a local macro.
Option c) is the most readable, but there is a collision with gnu gettext.
So it cannot be used.

LS


More information about the sssd-devel mailing list