Re: [PATCH V4 14/24] arm/ioreq: Introduce arch specific bits for IOREQ/DM features

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.01.2021 13:22, Oleksandr wrote:
> On 27.01.21 12:24, Jan Beulich wrote:
>> On 21.01.2021 09:50, Oleksandr wrote:
>>> On 20.01.21 17:50, Julien Grall wrote:
>>>> On 17/01/2021 18:52, Oleksandr wrote:
>>>>> error2.txt - when add #include <public/hvm/dm_op.h> to xen/ioreq.h
>>>> It looks like the error is happening in dm.c rather than xen/dm.h. Any
>>>> reason to not include <public/hvm/dm_op.h> in dm.c directly?
>>> Including it directly doesn't solve build issue.
>>> If I am not mistaken in order to follow requirements how to include
>>> headers (alphabetic order, public* should be included after xen* and
>>> asm* ones, etc)
>>> the dm.h gets included the first in dm.c, and dm_op.h gets included the
>>> last. We can avoid build issue if we change inclusion order a bit,
>>> what I mean is to include dm.h after hypercall.h at least (because
>>> hypercall.h already includes dm_op.h). But this breaks the requirements
>>> and is not way to go.
>>> Now I am in doubt how to overcome this.
>> First, violating the alphabetic ordering rule is perhaps less
>> of a problem than putting seemingly stray #include-s anywhere.
>> However, as soon as ordering starts mattering, this is
>> indicative of problems with the headers: Either the (seemingly)
>> too early included one lacks some #include-s, or you've run
>> into a circular dependency. In the former case the needed
>> #include-s should be added, and all ought to be fine. In the
>> latter case, however, disentangling may be a significant
>> effort, and hence it may be sensible and acceptable to instead
>> use unusual ordering of #include-s in the one place where it
>> matters (suitably justified in the description). Ideally such
>> would come with a promise to try to sort this later on, when
>> time is less constrained.
> Thank you for the explanation. I think, I am facing the former case (too 
> early included one lacks some #include-s),
> actually both common/dm.c and arch/arm/dm.c suffer from that.
> It works for me if I add the following to both files (violating the 
> alphabetic ordering rule):
> 
> +#include <xen/types.h>
> +#include <public/hvm/dm_op.h>
> +
>   #include <xen/dm.h>
> 
> 
> So, if I got your point correctly, we could include these both headers 
> by dm.h) Would it be appropriate (with suitable justification of course)?

Perhaps - this is a header you introduce aiui, so it's up to
you to arrange for it to include all further headers it
depends upon. In such a case (new header) you don't need to
explicitly justify what you include, but of course you don't
want to include excessive ones, or you risk getting back
"Why?" from reviewers.

Jan