[CRIU] [PATCH] Move headers around to fix issues on custom-built glibc

[Nikolay Borisov]

On Tue, Aug 16, 2016 at 7:54 PM, Dmitry Safonov <dsafonov at virtuozzo.com> wrote:
> On 08/16/2016 07:25 PM, Nikolay Borisov wrote:
>>
>> On Tue, Aug 16, 2016 at 7:09 PM, Dmitry Safonov <dsafonov at virtuozzo.com>
>> wrote:
>>>
>>> On 08/16/2016 06:52 PM, Dmitry Safonov wrote:
>>>>
>>>>
>>>> Hi,
>>>>
>>>> On 08/16/2016 06:13 PM, Nikolay Borisov wrote:
>>>>>
>>>>>
>>>>> diff --git a/criu/arch/x86/crtools.c b/criu/arch/x86/crtools.c
>>>>> index 51eab133f269..54ccf8465f7f 100644
>>>>> --- a/criu/arch/x86/crtools.c
>>>>> +++ b/criu/arch/x86/crtools.c
>>>>> @@ -11,9 +11,9 @@
>>>>>
>>>>>  #include "cr_options.h"
>>>>>  #include "compiler.h"
>>>>> +#include "restorer.h"
>>>>>  #include "ptrace.h"
>>>>>  #include "parasite-syscall.h"
>>>>> -#include "restorer.h"
>>>>>  #include "log.h"
>>>>>  #include "util.h"
>>>>>  #include "cpu.h"
>>>>>
>>>>
>>>> Hmm, from my POV, one should avoid hard include seq-dependencies as
>>>> much as possible.
>>>> As F_{S,G}ETPIPE_SZ are defined both in fcntl.h and config-base.h,
>>>> can we do something like that? Am I missing something?
>>>>
>>>
>>> I replaced system includes with a wrapper only for users of
>>> F_{S,G}ETPIPE_SZ -- maybe we should even convert *all* includes of
>>> system header to wrapper?
>>> It would be a noisy patch, but for a long perspective it may come
>>> as a better solution than manually checking order of includes.
>>>
>>
>> I think switching to CRIU"s provided fcntl.h is identical to what I
>> did. Because in it there are ifdef guards but in the system-provided
>> fcntl there isn't. So in case "fcntl.h" is included before the system
>> one, it will define F_SETPIPE_SZ etc and when the system header is
>> included it will cause gcc warnings.
>>
>
> Yep, but this is only for the crtools.c file fix.
> And if some other file has this problem? Than we should follow the same
> order of includes, which isn't convenient. We should document it then
> somewhere or something...
> And then we could meet the cycle-dependencies where one file should be
> included before the other by some reason, but that other should be first
> by a different reason.
> I don't mind for your fix here, but the proper solution would be
> destroying the include-dependecy.

You are correct about proliferation of the include order. However,
what I meant was that your fix doesn't eliminate the order dependency.
Maybe the proper way to fix this is add a 'configure' step which will
check what is defined and what not and based on that set some defines
in turn so that when criu is compiled it will now whether to include
its own definitions. But that would be significantly more work than
this simple fix here.


>
> --
>              Dmitry