On Tue, 2016-01-19 at 17:06 +0100, Lukas Slebodnik wrote: > 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
Lukas, are you saying you are opposed to add config.h as the top header in all files that miss it ? The fact we see this problem only with my other patch does not mean the current situation with the headers is ok. If I wanted to fix just the cli_creds issues I would have proposed a much smaller patch to start with. Please comment on this patch and how to proceed or if you think we should drop it entirely. Simo. -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
