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