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