Re: [Xen-devel] [PATCH v4 01/16] xen: Relocate mem_access and mem_event into common.

[Tamas K Lengyel <tamas.lengyel@xxxxxxxxxxxx>]

On Fri, Sep 5, 2014 at 1:26 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 05.09.14 at 12:52, <tamas.lengyel@xxxxxxxxxxxx> wrote:
> On Fri, Sep 5, 2014 at 12:48 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>
>> >>> On 05.09.14 at 12:36, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> > On Fri, Sep 5, 2014 at 12:07 PM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>> >
>> >> >>> On 05.09.14 at 11:45, <tamas.lengyel@xxxxxxxxxxxx> wrote:
>> >> > On Fri, Sep 5, 2014 at 11:14 AM, Jan Beulich <JBeulich@xxxxxxxx>
>> wrote:
>> >> >
>> >> >> >>> On 05.09.14 at 10:58, <tklengyel@xxxxxxxxxxxxx> wrote:
>> >> >> > --- a/xen/arch/x86/x86_64/mm.c
>> >> >> > +++ b/xen/arch/x86/x86_64/mm.c
>> >> >> > @@ -35,9 +35,9 @@
>> >> >> >  #include <asm/msr.h>
>> >> >> >  #include <asm/setup.h>
>> >> >> >  #include <asm/numa.h>
>> >> >> > -#include <asm/mem_event.h>
>> >> >> > +#include <xen/mem_event.h>
>> >> >> >  #include <asm/mem_sharing.h>
>> >> >> > -#include <asm/mem_access.h>
>> >> >> > +#include <xen/mem_access.h>
>> >> >> >  #include <public/memory.h>
>> >> >>
>> >> >> This is not the only place, but a specifically bad example: I'm
>> pretty
>> >> >> sure I asked you before to not make a mess by mixing asm/ and
>> >> >> xen/ included - move the now xen/ ones to the other ones already
>> >> >> coming from that directory. And do so consistently throughout the
>> >> >> patch.
>> >> >>
>> >> >>
>> >> > Sure. Is this a style-thing or does it have some other effect on the
>> >> code?
>> >>
>> >> A style thing that is not supposed to have an effect (we should
>> >> strive for headers to include their dependencies rather than relying
>> >> on their users doing so up front).
>> >>
>> >> >> Looking at this again I wonder though
>> >> >> whether we really need these - the use sites could easily check
>> >> >> whether p2m_is_{paging,shared} is defined instead.
>> >> >
>> >> >  I would still need to wrap them in CONFIG_X86 blocks as
>> common/memory.c
>> >> > does, so ultimately I'm not sure there is much difference.
>> >>
>> >> I don't understand why - ARM doesn't define p2m_is_paging()
>> >> or p2m_is_shared().
>> >
>> > Hm, I guess I could do that, just define them to return 0 and hope the
>> > compiler just drops the always-dead if blocks.
>>
>> No, that's not what I was asking you to do, as that may require you
>> to defined further stubs used inside the conditionals. Instead I asked
>> that you replace "#ifdef CONFIG_..." with "#ifdef p2m_is_...".
>>

> Oh, OK. Isn't that a bit hack-ish looking though?

Some may say it is, but I personally don't think so. After all there's
a clear connection between the #ifdef and the subsequent code.

Jan

I agree I just don't see such an approach used anywhere else. Let's wait to hear from others what their thoughts are on this.

Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel