Re: [PATCH 1/9] xen/common: Add missing #includes treewide

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.08.2023 01:02, Shawn Anastasio wrote:
> A few files treewide depend on defininitions in headers that they
> don't include. This works when arch headers end up including the
> required headers by chance, but broke on ppc64 with only minimal/stub
> arch headers.
> 
> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>

I'm okay with the changes in principle, but I'd like to ask a question
nevertheless, perhaps also for other REST maintainers (whom you should
have Cc-ed, btw) to chime in.

> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -28,6 +28,7 @@
>  #include <asm/current.h>
>  #include <asm/hardirq.h>
>  #include <asm/p2m.h>
> +#include <asm/page.h>
>  #include <public/memory.h>
>  #include <xsm/xsm.h>

I realize there are several asm/*.h being included here already. Yet
generally I think common .c files would better not include any of
them directly; only xen/*.h ones should (and even there one might see
possible restrictions on what's "legitimate"). Do you recall what it
was that's needed from asm/page.h here ...

> --- a/xen/common/xmalloc_tlsf.c
> +++ b/xen/common/xmalloc_tlsf.c
> @@ -27,6 +27,7 @@
>  #include <xen/mm.h>
>  #include <xen/pfn.h>
>  #include <asm/time.h>
> +#include <asm/page.h>

... and here?

> --- a/xen/include/xen/domain.h
> +++ b/xen/include/xen/domain.h
> @@ -4,6 +4,7 @@
>  
>  #include <xen/types.h>
>  
> +#include <public/domctl.h>
>  #include <public/xen.h>

While following our sorting guidelines, this still looks a little odd.
We typically would include public/xen.h first, but then almost all other
public headers include it anyway. So I'm inclined to suggest to replace
(rather than amend) the existing #include here.

Then again I wonder why this include is needed. xen/domain.h is
effectively included everywhere, yet I would have hoped public/domctl.h
isn't.

Jan