Re: [Xen-devel] [PATCH v3 for-4.8] libelf: fix symtab/strtab loading for 32bit domains

2016-10-14 Thread Jan Beulich 
>>> On 13.10.16 at 15:25,  wrote:
> On 13/10/16 13:48, Roger Pau Monne wrote:
>> diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
>> index 90bd8cb..70abbaf 100644
>> --- a/xen/include/xen/libelf.h
>> +++ b/xen/include/xen/libelf.h
>> @@ -432,6 +432,16 @@ struct elf_dom_parms {
>>  uint64_t virt_kend;
>>  };
>>  
>> +/* Number of section header needed in order to fit the SYMTAB and STRTAB. 
> */
>> +#define ELF_BSDSYM_SECTIONS 3
>> +struct elf_sym_header {
>> +uint32_t size;
>> +struct {
>> +elf_ehdr header;
>> +elf_shdr section[ELF_BSDSYM_SECTIONS];
>> +} elf_header;
>> +} __attribute__((packed));
> 
> __packed please, rather than opencoding it.  Also, should be between
> struct and the structure name.

There's no __packed in libxc afaics, and the code here is shared.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 for-4.8] libelf: fix symtab/strtab loading for 32bit domains

2016-10-13 Thread Andrew Cooper 
On 13/10/16 13:48, Roger Pau Monne wrote:
> diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h
> index 90bd8cb..70abbaf 100644
> --- a/xen/include/xen/libelf.h
> +++ b/xen/include/xen/libelf.h
> @@ -432,6 +432,16 @@ struct elf_dom_parms {
>  uint64_t virt_kend;
>  };
>  
> +/* Number of section header needed in order to fit the SYMTAB and STRTAB. */
> +#define ELF_BSDSYM_SECTIONS 3
> +struct elf_sym_header {
> +uint32_t size;
> +struct {
> +elf_ehdr header;
> +elf_shdr section[ELF_BSDSYM_SECTIONS];
> +} elf_header;
> +} __attribute__((packed));

__packed please, rather than opencoding it.  Also, should be between
struct and the structure name.

~Andrew

___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH v3 for-4.8] libelf: fix symtab/strtab loading for 32bit domains

2016-10-13 Thread Jan Beulich 
>>> On 13.10.16 at 14:48,  wrote:
> @@ -174,8 +171,8 @@ void elf_parse_bsdsyms(struct elf_binary *elf, uint64_t 
> pstart)
>  /* Space to store the size of the elf image */
>  sz = sizeof(uint32_t);
>  
> -/* Space for the elf and elf section headers */
> -sz += elf_uval(elf, elf->ehdr, e_ehsize) +
> +/* Space for the elf header and elf section headers */
> +sz += offsetof(struct elf_sym_header, elf_header.section) +
>ELF_BSDSYM_SECTIONS * elf_uval(elf, elf->ehdr, e_shentsize);

You've retained the inconsistency which I had asked to eliminate
when commenting on v2.

> --- a/xen/include/xen/libelf.h
> +++ b/xen/include/xen/libelf.h
> @@ -432,6 +432,16 @@ struct elf_dom_parms {
>  uint64_t virt_kend;
>  };
>  
> +/* Number of section header needed in order to fit the SYMTAB and STRTAB. */
> +#define ELF_BSDSYM_SECTIONS 3
> +struct elf_sym_header {
> +uint32_t size;
> +struct {
> +elf_ehdr header;
> +elf_shdr section[ELF_BSDSYM_SECTIONS];
> +} elf_header;
> +} __attribute__((packed));

This doesn't belong here - it's still an internal structure. At most this
might go into libelf-private.h, but I think best would be to keep in the
C file, just moving it up (and out of any function) there. And if you
were to move it into _any_ header, the comment would need
adjustment to make clear what part of the loader this actually is
relevant for.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel