Re: [RESEND][PATCH v2 5/7] xen: include xen/guest_access.h rather than asm/guest_access.h

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.08.2020 10:58, Julien Grall wrote:
> 
> 
> On 18/08/2020 09:50, Jan Beulich wrote:
>> On 14.08.2020 21:07, Julien Grall wrote:
>>> Hi Jan,
>>>
>>> On 31/07/2020 12:36, Jan Beulich wrote:
>>>> On 30.07.2020 20:18, Julien Grall wrote:
>>>>> From: Julien Grall <jgrall@xxxxxxxxxx>
>>>>>
>>>>> Only a few places are actually including asm/guest_access.h. While this
>>>>> is fine today, a follow-up patch will want to move most of the helpers
>>>>> from asm/guest_access.h to xen/guest_access.h.
>>>>>
>>>>> To prepare the move, everyone should include xen/guest_access.h rather
>>>>> than asm/guest_access.h.
>>>>>
>>>>> Interestingly, asm-arm/guest_access.h includes xen/guest_access.h. The
>>>>> inclusion is now removed as no-one but the latter should include the
>>>>> former.
>>>>>
>>>>> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
>>>>
>>>> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>>
>>>> Is there any chance you could take measures to avoid new inclusions
>>>> of asm/guest_access.h to appear?
>>>
>>> It should be possible.
>>>
>>> How about this:
>>>
>>> diff --git a/xen/include/asm-arm/guest_access.h 
>>> b/xen/include/asm-arm/guest_access.h
>>> index b9a89c495527..d8dbc7c973b4 100644
>>> --- a/xen/include/asm-arm/guest_access.h
>>> +++ b/xen/include/asm-arm/guest_access.h
>>> @@ -1,3 +1,7 @@
>>> +#ifndef ALLOW_INCLUDE_ASM_GUEST_ACCESS_H
>>> +#error "asm/guest_access.h should not be included directly"
>>> +#endif
>>> +
>>>   #ifndef __ASM_ARM_GUEST_ACCESS_H__
>>>   #define __ASM_ARM_GUEST_ACCESS_H__
>>>
>>> diff --git a/xen/include/asm-x86/guest_access.h 
>>> b/xen/include/asm-x86/guest_access.h
>>> index 369676f31ac3..e665ca3a27af 100644
>>> --- a/xen/include/asm-x86/guest_access.h
>>> +++ b/xen/include/asm-x86/guest_access.h
>>> @@ -4,6 +4,10 @@
>>>    * Copyright (c) 2006, K A Fraser
>>>    */
>>>
>>> +#ifndef ALLOW_INCLUDE_ASM_GUEST_ACCESS_H
>>> +#error "asm/guest_access.h should not be included directly"
>>> +#endif
>>> +
>>>   #ifndef __ASM_X86_GUEST_ACCESS_H__
>>>   #define __ASM_X86_GUEST_ACCESS_H__
>>>
>>> diff --git a/xen/include/xen/guest_access.h b/xen/include/xen/guest_access.h
>>> index 75103d30c8be..814e31329de9 100644
>>> --- a/xen/include/xen/guest_access.h
>>> +++ b/xen/include/xen/guest_access.h
>>> @@ -7,7 +7,9 @@
>>>   #ifndef __XEN_GUEST_ACCESS_H__
>>>   #define __XEN_GUEST_ACCESS_H__
>>>
>>> +#define ALLOW_INCLUDE_ASM_GUEST_ACCESS_H
>>>   #include <asm/guest_access.h>
>>> +#undef ALLOW_INCLUDE_ASM_GUEST_ACCESS_H
>>>   #include <xen/types.h>
>>>   #include <public/xen.h>
>>
>> One option. Personally I'd prefer to avoid introduction of yet another
>> constant, by leveraging __XEN_GUEST_ACCESS_H__ instead.
> 
> I thought about it but it doesn't prevent new inclusions of 
> asm/guest_access.h. For instance, the following would still compile:
> 
> #include <xen/guest_access.h>
> 
> [...]
> 
> #include <asm/guest_access.h>

But where's the problem with this? The first #include will already
have resulted in the inclusion of asm/guest_access.h, so the second
#include is simply a no-op.

Jan