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

Pavel Reichl preichl at redhat.com
Mon Oct 12 12:06:07 UTC 2015



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?



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