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

Lukas Slebodnik lslebodn at redhat.com
Mon Oct 5 07:50:39 UTC 2015


On (05/10/15 09:35), Jakub Hrozek wrote:
>On Tue, Sep 08, 2015 at 04:11:26PM +0200, Michal Židek wrote:
>> 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.
>
>This thread has stalled, let's try to revive it. It would be nice to
>merge the patch.
>
>tl;dr I mostly agree with Michal and would prefer namespaced macro, even
>though it's a local one.

The main problem is a readability of code with such macro.
It's hard to find name of error code there.
>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" },
>

Try to propose something else.
The ideal would be something short (or lowecased) as with "_".
>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" },
>

LS


More information about the sssd-devel mailing list