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

Lukas Slebodnik lslebodn at redhat.com
Fri May 22 08:28:22 UTC 2015


On (21/05/15 17:26), Pavel Reichl wrote:
>
>
>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 at redhat.com>
>>>>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.
>
sss_cli.h should be included just in client code and should not be mixed
with responder/data_provider code. The same applies to nss_mc.h.

This is a reason why there are more typedefs.

>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

>From 16bee39c2cc7a734abc5a19d6069c80eed2500cc Mon Sep 17 00:00:00 2001
>From: Pavel Reichl <preichl at redhat.com>
>Date: Tue, 19 May 2015 07:35:11 -0400
>Subject: [PATCH] util: remove needless header files
                        ^^^^^^^^^^^^^^^
                That's not true.

>
>sss_cli.h is already included in authtok-utils.h, sysdb.h,
>data_provider.h, responder.h, responder_packet.h
>---
> src/responder/sudo/sudosrv_cmd.c | 1 -
> src/tests/cmocka/test_pam_srv.c  | 1 -
> src/tools/tools_mc_util.c        | 1 -
> src/util/authtok.c               | 1 +
> src/util/authtok.h               | 2 --
> 5 files changed, 1 insertion(+), 5 deletions(-)
>
>diff --git a/src/responder/sudo/sudosrv_cmd.c b/src/responder/sudo/sudosrv_cmd.c
>index dd636e949200dd49c1422a5789e9328dc4b25fb0..8d838babd0fbadcf9f1167cdeba4ec3855c246d5 100644
>--- a/src/responder/sudo/sudosrv_cmd.c
>+++ b/src/responder/sudo/sudosrv_cmd.c
>@@ -27,7 +27,6 @@
> #include "responder/common/responder_packet.h"
> #include "responder/sudo/sudosrv_private.h"
> #include "db/sysdb_sudo.h"
>-#include "sss_client/sss_cli.h"
> #include "responder/common/negcache.h"

sss_client/sss_cli.h defines SSS_GET_VERSION which is used in this file.



> static errno_t sudosrv_cmd_send_reply(struct sudo_cmd_ctx *cmd_ctx,
>diff --git a/src/tests/cmocka/test_pam_srv.c b/src/tests/cmocka/test_pam_srv.c
>index 03faea249a31359d721e82cfd5bc2f9f375d0592..ac9da69e3610578ae267ce5c0d63a832fb0822bd 100644
>--- a/src/tests/cmocka/test_pam_srv.c
>+++ b/src/tests/cmocka/test_pam_srv.c
>@@ -30,7 +30,6 @@
> #include "responder/pam/pamsrv.h"
> #include "responder/pam/pam_helpers.h"
> #include "sss_client/pam_message.h"
>-#include "sss_client/sss_cli.h"
> 
> #include "util/crypto/nss/nss_util.h"
>
SSS_PAM_AUTHENTICATE is defined sss_client/sss_cli.h which is used in this file

>diff --git a/src/tools/tools_mc_util.c b/src/tools/tools_mc_util.c
>index 9634b86e248a1937f68939c5a8d8a6301780a566..d54653abae26145a0aaa660b7ee3704b16f81a8d 100644
>--- a/src/tools/tools_mc_util.c
>+++ b/src/tools/tools_mc_util.c
>@@ -26,7 +26,6 @@
> #include "util/util.h"
> #include "tools/tools_util.h"
> #include "util/mmap_cache.h"
>-#include "sss_client/sss_cli.h"
SSS_NSS_GETPWNAM is defined sss_client/sss_cli.h which is used in this file

> /* This is a copy of sss_mc_set_recycled present in
>  * src/responder/nss/nsssrv_mmap_cache.c. If you modify this function,
>diff --git a/src/util/authtok.h b/src/util/authtok.h
>index cb366270832852281a222018f8e27feb1500ff01..55f080e85806b7c241c6287a42a2cfa501a8ee19 100644
>--- a/src/util/authtok.h
>+++ b/src/util/authtok.h
>@@ -20,9 +20,7 @@
> #ifndef __AUTHTOK_H__
> #define __AUTHTOK_H__
> 
>-#include "util/util.h"
> #include "util/authtok-utils.h"
>-#include "sss_client/sss_cli.h"

As I mentioned in previous mail; we *should not* rely on the order of header
files or included header fiels from other hedaer files.
In most cases we do not care about it (if code compiles without problem)
but on the other hand it's not a reason for micro-ptimalisation
to remove includes as in your patch. There still posibility that function
will be moved to another file(removed, ...) and the header file will need
to be included again.

All header files are guarded with defines. It's not a problem
when the same header files are included in header file and implementation
module. The only reason for removing lines with include will be if header file
is not used at all in module.

LS



More information about the sssd-devel mailing list