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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.10.14 at 06:55, <tiejun.chen@xxxxxxxxx> wrote:
> 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.

Redefine? I said forward declare.

> 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/,

I think if it is (as suggested) to be limited to hvmloader, the
symlinking also should be done in its Makefile rather than along
with other tools stuff.

Jan


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