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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

>>>> 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
>>>
>>> Seems we just need to include this,
>>>
>>> #include <errno.h>
>>
>> You shouldn't include system headers here - what if the build system's
>> -E... values differ from Xen's? Please remember that what your making
> 
> tools/firmware/hvmloader/xenbus.c:30:#include <errno.h>

This is a completely different case: For one, no-one really looks at
the error codes generated here. And even if someone would, these
would be error value purely internal to hvmloader. Whereas in your
case you want to interpret a value you get back from the hypervisor.

> And why will Xen define this different?

Why would Linux, *BSD, Solaris, and whatever else OS usable as a
build host for building Xen all be required to use exactly the same
-E... definitions when already Linux has variations for some of them
depending on host architecture (i.e. when doing cross builds you'd
risk running into problems even on Linux)?

Once again - please always keep in mind that you're modifying
hypervisor code, not some simple user mode application.

Jan


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