Re: [SSSD] [PATCH] util: don't include util.h in src/util/authtok.h

[Pavel Reichl]

On 05/19/2015 02:25 PM, Pavel Reichl wrote:
>  On 05/19/2015 02:15 PM, Lukas Slebodnik wrote:
> > On (19/05/15 13:59), Pavel Reichl wrote:
> >> Hello,
> >>
> >> please see attached simple patch. I believe that nested header files 
> >> are
> >> sometimes necessary evil, but they should be avoided when possible. 
> >> It makes
> >> difficult figuring out on which header files source file actually 
> >> depend on
> >> and can increase compilation time.
> >>
> >> Thanks.
> > >From d11f9e4538cbb6d247f43e427347d59e1ca6c98f Mon Sep 17 00:00:00 2001
> >> From: Pavel Reichl <preichl(a)redhat.com&gt;
> >> Date: Tue, 19 May 2015 07:35:11 -0400
> >> Subject: [PATCH] util: don't include util.h in src/util/authtok.h
> >>
> >> ---
> >> src/util/authtok.c | 1 +
> >> src/util/authtok.h | 1 -
> >> 2 files changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/src/util/authtok.c b/src/util/authtok.c
> >> index 
> >>
> 45761df80175fded8a6c6e5dac8a90180b11d225..784512c0bd21128bd16d3663bf460a286e8b3eed 
> >> 100644
> >> --- a/src/util/authtok.c
> >> +++ b/src/util/authtok.c
> >> @@ -17,6 +17,7 @@
> >>     along with this program.  If not, see 
> >> <http://www.gnu.org/licenses/>;.
> >> */
> >>
> >> +#include "util/util.h"
> >> #include "authtok.h"
> >>
> >> struct sss_auth_token {
> >> diff --git a/src/util/authtok.h b/src/util/authtok.h
> >> index 
> >>
> cb366270832852281a222018f8e27feb1500ff01..d82168f5823e188e5c04fa418cb0ff52f6a52aac 
> >> 100644
> >> --- a/src/util/authtok.h
> >> +++ b/src/util/authtok.h
> >> @@ -20,7 +20,6 @@
> >> #ifndef __AUTHTOK_H__
> >> #define __AUTHTOK_H__
> >>
> >> -#include "util/util.h"
> >> #include "util/authtok-utils.h"
> >> #include "sss_client/sss_cli.h"
> > This is not right solution. Even thought everything compiles without any
> > problem.
> >
> > "util/util.h" was included because errnot_t is used in prototypes of 
> > functions.
> >
> > sh$ grep -n errno_t src/util/authtok.h
> > 70:errno_t sss_authtok_get_password(struct sss_auth_token *tok,
> > 85:errno_t sss_authtok_set_password(struct sss_auth_token *tok,
> > 101:errno_t sss_authtok_get_ccfile(struct sss_auth_token *tok,
> > 115:errno_t sss_authtok_set_ccfile(struct sss_auth_token *tok,
> > 140:errno_t sss_authtok_set(struct sss_auth_token *tok,
> > 154:errno_t sss_authtok_copy(struct sss_auth_token *src,
> > 198:errno_t sss_authtok_set_2fa(struct sss_auth_token *tok,
> > 222:errno_t sss_authtok_get_2fa(struct sss_auth_token *tok,
> >
> >
> > and with your patch "errno_t" has to be defined before including 
> > authtok.h
> > It's better to do not rely on the order of included header files.
>  Yes, you are right, thanks.
>
>  I suppose that correct solution would be then include just 
>  'util/util_errors.h'  in src/util/authtok.h. But I suppose we have 
>  bigger fish to fry...or do you encourage me to send such a patch? 
I had another
peak at the patch, I found out that we actually redefine 
errno_t in 3 different header files (util_errors.h, sss_cli.h, nss_mc.h) 
which is bad.

Attached patch removes already included header files...although I'm not 
sure if it would not be better to actually get rid of as much nested 
header files (header files included in header files) as possible and 
rather include header files directly.

What do you think Lukas?
> >
> > LS
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel(a)lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
>  _______________________________________________
>  sssd-devel mailing list
>  sssd-devel(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel