[SSSD] [PATCH] SDAP: add missing header files

Pavel Reichl preichl at redhat.com
Mon Oct 12 12:45:37 UTC 2015



On 10/12/2015 02:36 PM, Lukas Slebodnik wrote:
> On (12/10/15 14:06), Pavel Reichl wrote:
>>
>>
>> On 10/12/2015 01:52 PM, Lukas Slebodnik wrote:
>>> On (12/10/15 13:05), Pavel Reichl wrote:
>>>>
>>>>
>>>> On 10/12/2015 07:32 AM, Lukas Slebodnik wrote:
>>>>> On (11/10/15 23:11), Pavel Reichl wrote:
>>>>>> Hello,
>>>>>>
>>>>>> while messing around with some code analyses tool I noticed that some header files are not including directly header files which they use and thus are dependent on being included in particular order.
>>>>>>
>>>>>> Please see simple patch attached that fixes some of such files (but definitely not all).
>>>>>
>>>>> >From 12c273af1c8ffe727efc526c9e993240904bcbf1 Mon Sep 17 00:00:00 2001
>>>>>> From: Pavel Reichl <reichl.pavel at gmail.com>
>>>>>> Date: Sun, 11 Oct 2015 22:29:44 +0200
>>>>>> Subject: [PATCH 1/4] SDAP: add missing header files
>>>>>>
>>>>>> ---
>>>>>> src/providers/ldap/ldap_auth.h       | 4 ++++
>>>>>> src/providers/ldap/sdap_async_enum.h | 6 ++++++
>>>>>> src/providers/ldap/sdap_autofs.h     | 6 ++++++
>>>>>> src/providers/ldap/sdap_id_op.h      | 4 ++++
>>>>>> src/providers/ldap/sdap_sudo.h       | 6 ++++++
>>>>>> src/providers/ldap/sdap_users.h      | 4 ++++
>>>>>> 6 files changed, 30 insertions(+)
>>>>>>
>>>>>> diff --git a/src/providers/ldap/ldap_auth.h b/src/providers/ldap/ldap_auth.h
>>>>>> index 5fbddd7087dc65ab8bd1df5fb57492d2fc26d0bb..1848af5de19796c8287117b9afe5c6d3e1a248ec 100644
>>>>>> --- a/src/providers/ldap/ldap_auth.h
>>>>>> +++ b/src/providers/ldap/ldap_auth.h
>>>>>> @@ -22,6 +22,10 @@
>>>>>>
>>>>>> #include "config.h"
>>>>>>
>>>>>> +#include <talloc.h>
>>>>>> +
>>>>>> +#include "util/util.h"
>>>>>> +
>>>>> util.h already includes talloc.h.
>>>>> Could you explain why do we need to include both of them?
>>>>
>>>> Thanks for noticing that. This seems to be quite common in our code base:
>>>> git grep -l 'util.h' -- '*.[ch]' | xargs grep 'talloc.h' | wc -l
>>>> 88
>>>>
>>>> But it's glitch in the patch nonetheless.
>>>>
>>>>>
>>>>>
>>>>>
>>>>>> enum pwexpire {
>>>>>>      PWEXPIRE_NONE = 0,
>>>>>>      PWEXPIRE_LDAP_PASSWORD_POLICY,
>>>>>> diff --git a/src/providers/ldap/sdap_async_enum.h b/src/providers/ldap/sdap_async_enum.h
>>>>>> index 2da38f988913fa0d6f252697925e50e05eb794a6..7d4ff82c5e003ffbd14aca76f321a8a0fc41b3d6 100644
>>>>>> --- a/src/providers/ldap/sdap_async_enum.h
>>>>>> +++ b/src/providers/ldap/sdap_async_enum.h
>>>>>> @@ -26,6 +26,12 @@
>>>>>> #ifndef _SDAP_ASYNC_ENUM_H_
>>>>>> #define _SDAP_ASYNC_ENUM_H_
>>>>>>
>>>>>> +#include "config.h"
>>>>>> +
>>>>>> +#include <talloc.h>
>>>>>> +
>>>>>> +#include "util/util.h"
>>>>>> +
>>>>>
>>>>> util.h already includes config.h and talloc.h.
>>>>> Could you explain why we need to include all mentioned header files?
>>>>
>>>> Same as above. Thanks for noticing.
>>>>
>>>> git grep -l 'util.h' -- '*.[ch]' | xargs grep -l 'talloc.h' | xargs grep 'config.h' | wc -l
>>>> 23
>>>>
>>>> Please see updated patch.
>>>
>>> I'm sorry but I still miss an explanation why do we need to include these
>>> header files. You just updated patches but the explanation is neither in mail
>>> nor in commit message.
>>>
>>> LS
>>
>> In first mail I wrote:
>>
>>>>>>   I noticed that some header files are not including directly header files which they use and thus are dependent on being included in particular order.
>>
>> If we have header file A that uses declarations from other header file B and
>> B is not directly included from A then compilation of source file that
>> includes header file A will fail unless header file B is included before
>> header A. This is not desired behavior. I don't think this is a big problem
>> and I don't think we should spent time actively fixing this. Still I hit this
>> problem and I fixed it in the files that failed for me. I don't see any
>> reason why not to merge such improvements. Do you?
>>
> I'm not stupid I know why we need to include missing header file.
I'm sorry if I offended you. It was not my intention.

> But it's not cleaner which header file need to be included and why?

OK, I'll add explanation to commit messages.
>
> So could you be more concreate?
>
> e.g.: We need to include talloc.h because talloc memory context are used in
> header file.
>
> BTW the main problem with including util.h is that include many header files,
> which is not necessary in many cases. And header files should not include
> unnecessary header files. It significantly slow down compilation on slow
> processors (arm development boards e.g. raspberry pi, ...)

Yes I agree, so if header file is missing just declaration of errno_t then including just 'util/util_errors.h' is fine with you?

BTW should every header file contain as first header file 'config.h'?

Thanks!

>
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>


More information about the sssd-devel mailing list