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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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 <aik@ozlabs.ru>
> ---
>  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.


-- 
Alexey