[PATCH] UTIL: Alternative way for debug message initialisation

[Lukas Slebodnik]

ehlo,

attached patch shoul avoid situation as in commit
7c69221077c780e62f6c536e78675f2dc1c131bc
The comments does not guarantee anything.

Author: Michal Zidek <mzidek(a)redhat.com&gt;
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.

LS

[Michal Židek]

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(a)redhat.com&gt;
>  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.

>  LS
>
>

[Lukas Slebodnik]

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(a)redhat.com&gt;
> >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.
b) SSSD_ERR_IDX significantly decrease readability

LS

[Michal Židek]

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(a)redhat.com&gt;
> >> 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.

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

Michal

>  LS

[Lukas Slebodnik]

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(a)redhat.com&gt;
> >>>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

[Michal Židek]

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(a)redhat.com&gt;
> >>>> 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.

Michal

[Lukas Slebodnik]

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(a)redhat.com&gt;
> >>>>>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

[Petr Cech]

On 09/08/2015 03:31 PM, Lukas Slebodnik wrote:
>  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 
Only my 2 cents:
Isn't compile time check really important feature?
Petr

[Michal Židek]

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(a)redhat.com&gt;
> >>>>>> 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

[Jakub Hrozek]

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(a)redhat.com&gt;
>  >>>>>>>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.

[Lukas Slebodnik]

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(a)redhat.com&gt;
> > >>>>>>>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

[Jakub Hrozek]

On Mon, Oct 05, 2015 at 09:50:39AM +0200, Lukas Slebodnik wrote:
>  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(a)redhat.com&gt;
>  >> >>>>>>>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" },
>  > 
Looks fine to me.

>  
>  Try to propose something else.
>  The ideal would be something short (or lowecased) as with "_". 
S_ERR_IDX?

>  >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
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (05/10/15 09:57), Jakub Hrozek wrote:
> On Mon, Oct 05, 2015 at 09:50:39AM +0200, Lukas Slebodnik wrote:
> > 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(a)redhat.com&gt;
> > >> >>>>>>>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"
> },
> > >
>
> Looks fine to me.
>
> > 
> > Try to propose something else.
> > The ideal would be something short (or lowecased) as with "_".
>
> S_ERR_IDX?
>  
It seems that you don't prefer lowercased macro for improving visibility.

I think it would be better if all active developers vote for prefered
version. It would affect all of us and we would need to backport it to
1.13 to avoid conflicts with backporting.

If most of developers prefer SSSD_ERR_IDX I will prepare new version.

BTW what about ("__" or prefixed version "sss__")


LS

[Jakub Hrozek]

On Mon, Oct 05, 2015 at 10:25:20AM +0200, Lukas Slebodnik wrote:
>  On (05/10/15 09:57), Jakub Hrozek wrote:
>  >On Mon, Oct 05, 2015 at 09:50:39AM +0200, Lukas Slebodnik wrote:
>  >> 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(a)redhat.com&gt;
>  >> >> >>>>>>>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" },
>  >> >
>  >
>  >Looks fine to me.
>  >
>  >> 
>  >> Try to propose something else.
>  >> The ideal would be something short (or lowecased) as with "_".
>  >
>  >S_ERR_IDX?
>  >
>  It seems that you don't prefer lowercased macro for improving visibility. 
>  
>  I think it would be better if all active developers vote for prefered
>  version. It would affect all of us and we would need to backport it to
>  1.13 to avoid conflicts with backporting.
>  
>  If most of developers prefer SSSD_ERR_IDX I will prepare new version.
>  
>  BTW what about ("__" or prefixed version "sss__") 
I don't care about case[*], I care about namespace prefix and knowing what
the macro does when I read the name. Namespaced lowecarcase sss_err_i or
even s_err_i are fine by me..

[*] My general rule is that macros with side-effects like changing
variables or jumping around the code should be uppercase.