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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 01/09/2023 11:44, Jan Beulich wrote:
> On 01.09.2023 11:13, Nicola Vetrini wrote:
> > 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.
> Hmm, maybe then I misunderstood your earlier reply: I thought you were
> arguing against the change I had suggested.
I'm fine with either way, but since the one proposed in the patch may 
result in an inconvenience
for other architectures, then perhaps the other is better.

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