Re: [Xen-devel] [PATCH v2 1/5] xen/x86: Introduce MultiBoot Data (MBD) type

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Thu, Sep 25, 2014 at 10:22:16AM +0100, Jan Beulich wrote:
> >>> On 24.09.14 at 20:40, <andrew.cooper3@xxxxxxxxxx> wrote:
> > On 24/09/14 18:19, Daniel Kiper wrote:

[...]

> >> +typedef unsigned char u8;
> >> +typedef unsigned short u16;
> >> +typedef unsigned long u32;
> >> +typedef unsigned long long u64;
> >> +
> >> +#include "../../../include/xen/compiler.h"
> >> +#include "../../../include/xen/multiboot.h"
> >> +
> >> +#include "../../../include/asm/mbd.h"
> >
> > How about playing with -I for this file in the Makefile to allow
> > #include <xen/compiler.h> and so ?
>
> Including these here is bogus anyway, even if for the ones above it's

Hmmm... Why it is bogus?

> perhaps acceptable. But expressing it to be bogus via the ../../../
> prefix is quite desirable imo.

I have been thinking about that since I saw this first time. Why we could
not use -I gcc option here? Could you enlighten me?

> >> +
> >> +/*
> >> + * __HERE__ IS ENTRY POINT!!!
> >
> > I am still of the firm opinion that anyone capable of editing this file
> > ought to know understand the _start symbol, making this comment useless.
>
> Indeed.

We know this right now. However, I spent some time to discover this at
the beginning of this work. This file is full of magic so I think that
this comment helps a bit to understand what is going on. So, please
do me a favor and let's leave it here. If you wish I can use lowercase
and remove underscore. Additionally, I removed similar comment for
__reloc() (as you requested) which does not make sense if it is
prefixed with static.

Daniel

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