On (19/01/16 16:47), Michal Židek wrote:
On 01/19/2016 04:28 PM, Simo Sorce wrote:
>On Tue, 2016-01-19 at 02:54 +0100, Michal Židek wrote:
>>On 01/19/2016 12:03 AM, Simo Sorce wrote:
>>>Found this while working on another patch.
>>>
>>>It is not evident by this patch alone but ... "trust me" :-)
>>>(I'll send the other patch next, try to apply just that one and see what
>>>I mean if you want)
>>>
>>>Simo.
>>>
>>>
>>
>>Hi Simo!
>>
>>I wonder if including config.h indirectly through
>>util.h is a good thing. It may be better
>>to simply include config.h at the beginning of every
>>.c file (after license) as a rule of thumb. This way
>>even if we do not need util.h, we will have the same
>>beginning of file and it will be more difficult to forget
>>config.h. What do you think?
>
>I a ok with such a rule, I can change the patch to explicitly add
>config.h as first in all files I touched, are the other cleanups I did
>also ok ?
>Or do we want a patch that only adds config.h as the first header and
>touches nothing else ?
Yes, adding just config.h (or moving it to the top)
is probably the best thing to do. It is minimal change
and it will solve the immediate problem. And Lukas
will also agree with this IMO (see the explanation
below).
>
>>That being said, I know you made this patch in order
>>to move work on another patch, so I definitely do not
>>want to block the review.
>>
>>CI passed:
>>http://sssd-ci.duckdns.org/logs/job/35/72/summary.html
>>
>>But I will wait with the ack until you respond to the
>>question above.
>>
>>Btw. we have quite a lot of files that do not use
>>util.h that probably already use this rule (I did
>>not check all of them, just did the grep). I think
>>the rule would add consistency to the code.
>
>Yes a lot of files do the "right" thing (between quotes because Lukas
>apparently has a different opinion), just not all of them.
I think there is a misunderstanding. Lukas is right that
it is a common practice to include system headers first
and after that your own headers. However config.h is
an exception. It is a special header that set's up the
environment. Btw. the reason why system headers should be
added before our own headers is similar to why config.h should
be the first header. They sometimes also change environment
(by environment I mean, they define special macros that
change behavior of some functions etc.)
If I undestood it correctly Lukas did not protest against
including config.h as the first header, but he did not like
the util.h as first (because it is our own header).
I read/replied to this patch before seeing a problematic patch
"[PATCH] Abstract and improve connection credential handling"
There would not be a problem to include "config.h" in every
implentation module which includes "src/responder/common/responder.h"
But we would not fix a problem. The same issue would be in
any new implementation module for new responder.
I think I proposed better solution in reply to the problematic patch.
If we move conditional definition of struct cli_creds or
(UCRED_CTX, SELINUX_CTX) to the implenetation module we will solve
order of included header file only on one place andnot in 27+ files
LS