[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



>>> 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.

>>>>>> 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
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).

> 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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.