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
