On 02.06.2025 16:30, Alejandro Vallejo wrote:
> On Mon Jun 2, 2025 at 9:53 AM CEST, Jan Beulich wrote:
>> On 30.05.2025 14:02, Alejandro Vallejo wrote:> ---
>> a/xen/include/xen/grant_table.h
>>> +++ b/xen/include/xen/grant_table.h
>>> @@ -27,7 +27,7 @@
>>> #include <xen/rwlock.h>
>>> #include <public/grant_table.h>
>>>
>>> -#ifdef CONFIG_GRANT_TABLE
>>> +#if __has_include("asm/grant_table.h")
>>> #include <asm/grant_table.h>
>>> #endif
>>
>> This change looks wrong (or otherwise is lacking justification): With
>> GRANT_TABLE=n
>> the arch header isn't supposed to be included.
>
> It's not equivalent to the previous code; but that's a feature, not a bug.
>
> Not including the header with GRANT_TABLE=n was the best we could with
> the older toolchains in order to not try to include a header that might not
> exist. The high number of sequential inclusions of xen/grant_table.h and
> asm/grant_table.h seem to attest to that.
>
> I can ammend the commit message to be clearer, but IMO this is what it was
> always
> meant to be. I can replace the current commit message with:
>
> "The previous toolchain base version didn't provide __has_include(), which
> allows conditional inclusion based on a header's existence. Lacking that
> feature the inclusion was guarded by the GRANT_TABLE option being present
> but even then sometimes the arch-specific header is required even when
> the option is not selected. This causes inclusion sites to needlessly
> include both asm/grant_table.h and xen/grant_table.h.
>
> Using __has_include() removes this requirement at inclusion sites."
>
> Thoughts?
So why would we include a header we don't need when GRANT_TABLE=n? Overall
we're trying to reduce dependency trees rather than growing them further.
Jan
