On Tue, 2016-01-19 at 20:20 +0100, Lukas Slebodnik wrote: > On (19/01/16 11:30), Simo Sorce wrote: > >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 ? > > > At the moment, there is not a implentation module(*.c) which requires > defined GNU extensions and does not have included config.h > > I fixed few of them in past > 5389b3714be747f1a11ac51beb0c5988cfb6c240 > b3007e32fa5d6b722f3aaaf9fe7593103cb443c3 > 1ecb16023779f912ba1c4e9e418acb59296ce8bc > > >The fact we see this problem only with my other patch does not mean the > >current situation with the headers is ok. > > > We cannot see such issues in past because most of files does not > depend on defined GNU extensions in system header files. > And that's fine + more portable. > > The main issue in different patch was that you introduced > hard dependency on defined GNU extensions into responder.h > and it cannot be solved in the header file responder.h > because it depends on previously included header files. > > If we make "struct cli_creds" opaque then we will > reduce dependency on defined GNU extensions and > this patch would not be needed. > > I do not think that we need include "config.h" and thus > enable GNU extensions in every file (*.c)
Ok, I do not have intereset in this battle, please drop this patch and close the bug "wontfix" 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
