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

Michal Židek mzidek at redhat.com
Tue Sep 8 14:11:26 UTC 2015


On 09/08/2015 03:31 PM, Lukas Slebodnik wrote:
> 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.

Ah, yes sorry, I misunderstood your text.

>>>
>>>>> 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.

Well, I still think that it is better to namespace macros even if they
are local. Unlike functions, there are ways to propagate macros
by other means than header files so the collision may happen
in some cases. But yes that would require someone to use
not namespaced macro on place where it would be obviously
dangerous (like in Makefile) so we would catch it. This makes
option b) acceptable.

Please rebase the patches and if you do not decide to go
with option a) (which I would prefer) I can accept the
option b) as well.

> Option c) is the most readable, but there is a collision with gnu gettext.

Option c) is not readable because it makes people think "What
is gettext macro doing here?". The confusion is even bigger because
we use human readable text nearby.

> So it cannot be used.
>
> LS



More information about the sssd-devel mailing list