Re: [Xen-devel] [v7][RFC][PATCH 04/13] hvmloader/util: get reserved device memory maps

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2014/10/29 17:05, Jan Beulich wrote:
> > > > On 29.10.14 at 07:54, <tiejun.chen@xxxxxxxxx> wrote:
> > On 2014/10/28 17:48, Jan Beulich wrote:
> > > > > > On 28.10.14 at 06:21, <tiejun.chen@xxxxxxxxx> wrote:
> > > > On 2014/10/27 17:45, Jan Beulich wrote:
> > > > > > > > On 27.10.14 at 04:12, <tiejun.chen@xxxxxxxxx> wrote:
> > > > > > On 2014/10/24 22:22, Jan Beulich wrote:
> > > > > > > > > > On 24.10.14 at 09:34, <tiejun.chen@xxxxxxxxx> wrote:
> > > > > > > > --- a/tools/firmware/hvmloader/util.h
> > > > > > > > +++ b/tools/firmware/hvmloader/util.h
> > > > > > > > @@ -241,6 +241,12 @@ int build_e820_table(struct e820entry *e820,
> > > > > > > >                           unsigned int bios_image_base);
> > > > > > > >      void dump_e820_table(struct e820entry *e820, unsigned int nr);
> > > > > > > >
> > > > > > > > +#include <xen/memory.h>
> > > > > > > > +#define ENOBUFS     105 /* No buffer space available */
> > > > > > > This is a joke I hope? The #include belongs at the top (albeit afaict
> > > > > > > you don't really need it here), and the #define is completely
> > > > > > If without this line, #include <xen/memory.h>,
> > > > > >
> > > > > > In file included from build.c:25:0:
> > > > > > ../util.h:246:70: error: array type has incomplete element type
> > > > > >      int get_reserved_device_memory_map(struct xen_reserved_device_memory
> > > > > > entries[],
> > > > > >                                                                           ^
> > > > > > make[8]: *** [build.o] Error 1
> > > > > So just forward declare the structure ahead of the function
> > > > > declaration.
> > > > tools/firmware/hvmloader/pci.c:28:#include <xen/memory.h>
> > > > tools/firmware/hvmloader/ovmf.c:36:#include <xen/memory.h>
> > > >
> > > > So any reason I can't do such a same thing?
> > > You can, but it's undesirable. You're wanting this in a header, i.e.
> > > you'll make everyone consuming that header also implicitly depend
> > > on the new header you would include. We shouldn't pointlessly
> > > add build dependencies (and we should really try to reduce them
> > > where possible).
> > Looks I can remove those stuff from util.h and just add 'extern' to them
> > when we really need them.
> Please stop thinking this way. Declarations for things defined in .c
> files are to be present in headers, and the defining .c file has to
> include that header (making sure declaration and definition are and
> remain in sync). I hate having to again repeat my remark that you
> shouldn't forget it's not application code that you're modifying.
> Robust and maintainable code are a requirement in the hypervisor
> (and, as said it being an extension of it, hvmloader). Which - just
> to avoid any misunderstanding - isn't to say that this shouldn't also
> apply to application code. It's just that in the hypervisor and kernel
> (and certain other code system components) the consequences of
> being lax are much more severe.
Okay. But currently, the pci.c file already include 'util.h' and 
'<xen/memory.h>,
#include "util.h"
...
#include <xen/memory.h>

We can't redefine struct xen_reserved_device_memory in util.h.

> > > > > > > misplaced here. While I generally wouldn't recommend doing this, I
> > > > > > > think in the case here including the hypervisor header that defines
> > > > > > > them would be okay. Perhaps not via relative path, but via having
> > So is the following is a way "via having the Makefile symlink the
> > hypervisor header here."?
> >
> > --- a/tools/include/Makefile
> > +++ b/tools/include/Makefile
> > @@ -17,6 +17,7 @@ xen/.dir:
> >           ln -sf ../xen-sys/$(XEN_OS) xen/sys
> >           ln -sf $(addprefix $(XEN_ROOT)/xen/include/xen/,libelf.h 
> > elfstructs.h) xen/libelf/
> >           ln -s ../xen-foreign xen/foreign
> > +       ln -sf $(XEN_ROOT)/xen/include/xen/errno.h xen
> Along those lines at least. Consult with the tools maintainers as to
> whether this would better go into a special subdirectory (and
> perhaps not even under tools/include, but somewhere beneath
> tools/firmware/hvmloader/, since it's not meant to be used by
Agree with you.

> anyone else - as one of the ones de facto looking after hvmloader
> irrespective of what ./MAINTAINERS says, I'd strongly recommend
> limiting the visibility scope of this header as much as possible).
So this time this email is Toed the following maintainers

Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Ian Campbell <ian.campbell@xxxxxxxxxx>
Wei Liu <wei.liu2@xxxxxxxxxx>

So all tools maintainers,

Could you give us such a comment?

'ln -sf $(XEN_ROOT)/xen/include/xen/errno.h xen' versus
'ln -sf $(XEN_ROOT)/xen/include/xen/errno.h 
$(XEN_ROOT)/tools/firmware/hvmloader/,
Thanks
Tiejun

> > Then we just need include this in util.c:
> >
> > --- a/tools/firmware/hvmloader/util.c
> > +++ b/tools/firmware/hvmloader/util.c
> > @@ -26,6 +26,7 @@
> >    #include <xen/xen.h>
> >    #include <xen/memory.h>
> >    #include <xen/sched.h>
> > +#include <xen/errno.h>
> Exactly.
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel