Re: [Xen-devel] [PATCH 04/18] xen/x86: add multiboot2 protocol support

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.03.15 at 13:22, <daniel.kiper@xxxxxxxxxx> wrote:
> On Fri, Mar 27, 2015 at 11:20:10AM +0000, Jan Beulich wrote:
>> >>> On 27.03.15 at 11:56, <daniel.kiper@xxxxxxxxxx> wrote:
>> > On Fri, Feb 20, 2015 at 04:06:26PM +0000, Jan Beulich wrote:
>> >> >>> On 30.01.15 at 18:54, <daniel.kiper@xxxxxxxxxx> wrote:
>> >> > @@ -31,7 +38,16 @@ asm (
>> >> >      );
>> >> >
>> >> >  typedef unsigned int u32;
>> >> > +typedef unsigned long long u64;
>> >> > +
>> >> > +#include "../../../include/xen/compiler.h"
>> >>
>> >> Why?
>> >
>> > static multiboot_info_t __used *reloc(void *mbi_in, u32 mb_magic)
>> > Because of this ------> ^^^^^^
>>
>> And what keeps you from leaving out the "static", making the
>> __used unnecessary?
> 
> This func is clearly static. Why do not mark it as is and use __used.
> What is wrong with that?

#include-s of the above kind generally indicate badness, so we
should try to limit them to the absolute minimum.

>> >> > +
>> >> > +typedef struct {
>> >> > +    u32 type;
>> >> > +    u32 size;
>> >> > +    u32 mem_lower;
>> >> > +    u32 mem_upper;
>> >> > +} multiboot2_tag_basic_meminfo_t;
>> >> > +
>> >> > +typedef struct __packed {
>> >>
>> >> Why __packed when all the others aren't?
>> >
>> > Ha! This thing was taken from GRUB2 source.
>> > I was asking this question myself many times.
>> > I have not found real explanation for this
>> > but if you wish I can dive into code and
>> > try to find one (if it exists).
>>
>> Or even better just drop the __packed.
> 
> Should not we be in line with GRUB2 source?
> Even it sounds strange. Anyway, I will check
> GRUB2 source and maybe we can also remove __packed
> from it. This way everything will be OK.

Now that's the right course of action.

Jan


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