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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 01/09/2023 09:36, Jan Beulich wrote:
> 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).
As far as I can tell, including the asm header in the arm implementation 
file does not imply
that new archs will need such an header. Of course, if the solution 
proposed in the patch is
chosen then I agree with you.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)