Re: [XEN PATCH] ioreq: include arch-specific ioreq header in <xen/ioreq.h>

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.09.2023 09:13, Nicola Vetrini wrote:
> On 28/08/2023 09:59, Jan Beulich wrote:
>> On 25.08.2023 17:02, Nicola Vetrini wrote:
>>> The common header file for ioreq should include the arch-specific one.
>>
>> To be honest I'm not convinced of "should" here. There are two aspects
>> to consider: On one hand it is good practice to do what you say. Otoh 
>> it
>> introduces a needless dependency, and it'll require new arch-es 
>> (RISC-V,
>> PPC at present) to introduce another dummy header just to satisfy the
>> xen/ioreq.h use in common/memory.c. Therefore, _if_ we want to go this
>> route, besides a wording change here I think ...
>>
> 
> Since including both <xen/ioreq.h> and the arch-specific one was 
> requested to be changed
> in previous series, I was trying to apply the same pattern here.
> If you prefer not to change the current layout then the fix is even 
> simpler: I'll just
> include <asm/ioreq.h> in xen/arch/arm/ioreq.c, which is where it's 
> missing.
> 
>>> --- a/xen/include/xen/ioreq.h
>>> +++ b/xen/include/xen/ioreq.h
>>> @@ -20,6 +20,7 @@
>>>  #define __XEN_IOREQ_H__
>>>
>>>  #include <xen/sched.h>
>>> +#include <asm/ioreq.h>
>>
>> ... this #include wants to be conditional upon CONFIG_IOREQ_SERVER 
>> being
>> defined. (I'm actually surprised that there's no respective #ifdef in
>> that header, meaning types defined there are available even when the
>> functionality was turned off.)
> 
> The two functions in question are actually guarded by an #ifdef 
> CONFIG_IOREQ_SERVER
> in arch/arm/include/asm/ioreq.h (in the #else branch some stubs are 
> defined)

Well, I don't see how an #ifdef there helps with the aspect mentioned
earlier (new arch-es needing to needlessly provide such a header as long
as the #include here is unconditional).

Jan