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