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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
