Re: [Qemu-ppc] [PATCH 3/4] elf-loader: add more return codes

[Alexey Kardashevskiy]

On 01/22/2014 06:27 PM, Alexey Kardashevskiy wrote:
> On 01/22/2014 04:20 PM, Alexey Kardashevskiy wrote:
>> The existing load_elf() just returns -1 if it fails to load ELF. However
>> it could be smarter than this and tell more about the failure such as
>> wrong endianness or incompatible platform.
>>
>> This adds additional return codes for wrong architecture, wrong
>> endianness and if the image is not ELF at all.
>>
>> This fixes handling of what load_elf() returns for s390x and moxie
>> architectures, other callers just check the return value for <0 and
>> this remains unchanged.
>>
>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>> ---
>>  hw/core/loader.c     | 12 ++++++------
>>  hw/s390x/ipl.c       |  4 ++--
>>  include/hw/elf_ops.h | 19 ++++++++++++++-----
>>  include/hw/loader.h  |  5 +++++
>>  4 files changed, 27 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 0634bee..f510260 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -289,7 +289,7 @@ int load_elf(const char *filename, uint64_t 
>> (*translate_fn)(void *, uint64_t),
>>               void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
>>               uint64_t *highaddr, int big_endian, int elf_machine, int 
>> clear_lsb)
>>  {
>> -    int fd, data_order, target_data_order, must_swab, ret;
>> +    int fd, data_order, target_data_order, must_swab, ret = ELF_LOAD_FAILED;
>>      uint8_t e_ident[EI_NIDENT];
>>  
>>      fd = open(filename, O_RDONLY | O_BINARY);
>> @@ -302,8 +302,10 @@ int load_elf(const char *filename, uint64_t 
>> (*translate_fn)(void *, uint64_t),
>>      if (e_ident[0] != ELFMAG0 ||
>>          e_ident[1] != ELFMAG1 ||
>>          e_ident[2] != ELFMAG2 ||
>> -        e_ident[3] != ELFMAG3)
>> +        e_ident[3] != ELFMAG3) {
>> +        ret = ELF_LOAD_NOT_ELF;
>>          goto fail;
>> +    }
>>  #ifdef HOST_WORDS_BIGENDIAN
>>      data_order = ELFDATA2MSB;
>>  #else
>> @@ -317,6 +319,7 @@ int load_elf(const char *filename, uint64_t 
>> (*translate_fn)(void *, uint64_t),
>>      }
>>  
>>      if (target_data_order != e_ident[EI_DATA]) {
>> +        ret = ELF_LOAD_WRONG_ENDIAN;
>>          goto fail;
>>      }
>>  
>> @@ -329,12 +332,9 @@ int load_elf(const char *filename, uint64_t 
>> (*translate_fn)(void *, uint64_t),
>>                           pentry, lowaddr, highaddr, elf_machine, clear_lsb);
>>      }
>>  
>> -    close(fd);
>> -    return ret;
>> -
>>   fail:
>>      close(fd);
>> -    return -1;
>> +    return ret;
>>  }
>>  
>>  static void bswap_uboot_header(uboot_image_header_t *hdr)
>> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
>> index 1a6397b..cff77ad 100644
>> --- a/hw/s390x/ipl.c
>> +++ b/hw/s390x/ipl.c
>> @@ -97,10 +97,10 @@ static int s390_ipl_init(SysBusDevice *dev)
>>      } else {
>>          kernel_size = load_elf(ipl->kernel, NULL, NULL, NULL, NULL,
>>                                 NULL, 1, ELF_MACHINE, 0);
>> -        if (kernel_size == -1) {
>> +        if (kernel_size < 0) {
>>              kernel_size = load_image_targphys(ipl->kernel, 0, ram_size);
>>          }
>> -        if (kernel_size == -1) {
>> +        if (kernel_size < 0) {
>>              fprintf(stderr, "could not load kernel '%s'\n", ipl->kernel);
>>              return -1;
>>          }
>> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
>> index acc701e..b7e7b36 100644
>> --- a/include/hw/elf_ops.h
>> +++ b/include/hw/elf_ops.h
>> @@ -201,6 +201,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>>      uint64_t addr, low = (uint64_t)-1, high = 0;
>>      uint8_t *data = NULL;
>>      char label[128];
>> +    int ret = ELF_LOAD_FAILED;
>>  
>>      if (read(fd, &ehdr, sizeof(ehdr)) != sizeof(ehdr))
>>          goto fail;
>> @@ -210,23 +211,31 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>>  
>>      switch (elf_machine) {
>>          case EM_PPC64:
>> -            if (EM_PPC64 != ehdr.e_machine)
>> +            if (EM_PPC64 != ehdr.e_machine) {
>>                  if (EM_PPC != ehdr.e_machine)
> 
> A stupid bug here, "{" should go one line down :)
> 
> I'll repost if someone tells me that the whole idea makes any sense. Thanks.


Ping?





-- 
Alexey