Re: [Qemu-devel] [PATCH v1 15/17] loader: add API to load elf header

[Peter Crosthwaite]

On Tue, Jan 19, 2016 at 9:50 AM, Peter Maydell  wrote:
> On 18 January 2016 at 07:12, Peter Crosthwaite
>  wrote:
>> Add an API to load an elf header header from a file. Populates a
>> buffer with the header contents, as well as a boolean for whether the
>> elf is 64b or not. Both arguments are optional.
>>
>> Signed-off-by: Peter Crosthwaite 
>> ---
>>
>>  hw/core/loader.c| 48 
>>  include/hw/loader.h |  1 +
>>  2 files changed, 49 insertions(+)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 6b69852..28da8e2 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -331,6 +331,54 @@ const char *load_elf_strerror(int error)
>>  }
>>  }
>>
>> +void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp)
>> +{
>> +int fd;
>> +uint8_t e_ident[EI_NIDENT];
>> +size_t hdr_size, off = 0;
>> +bool is64l;
>> +
>> +fd = open(filename, O_RDONLY | O_BINARY);
>> +if (fd < 0) {
>> +error_setg_errno(errp, errno, "Fail to open file");
>
> "Failed" (also below).
>

Fixed (x2).

> I don't think we end up with the filename anywhere in the
> error message; it would be helpful if we could include it.
>

Fixed (x4)

>> +return;
>> +}
>> +if (read(fd, e_ident, sizeof(e_ident)) != sizeof(e_ident)) {
>> +error_setg_errno(errp, errno, "Fail to read file");
>> +goto fail;
>> +}
>> +if (e_ident[0] != ELFMAG0 ||
>> +e_ident[1] != ELFMAG1 ||
>> +e_ident[2] != ELFMAG2 ||
>> +e_ident[3] != ELFMAG3) {
>> +error_setg(errp, "Bad ELF magic");
>> +goto fail;
>> +}
>> +
>> +is64l = e_ident[EI_CLASS] == ELFCLASS64;
>> +hdr_size = is64l ? sizeof(Elf64_Ehdr) : sizeof(Elf32_Ehdr);
>> +if (is64) {
>> +*is64 = is64l;
>> +}
>> +
>> +lseek(fd, 0, SEEK_SET);
>
> You're not checking this lseek for failure (and you don't
> need it anyway, because you could just copy the magic bytes
> into *hdr and read four fewer bytes).
>

OK, so I have optimised it away. What I am doing now is always reading
to straight to hdr[], and if the caller passes hdr == NULL, then hdr
is set to a local buffer (and the full header read is still skipped as
per current logic).

>> +while (hdr && off < hdr_size) {
>> +size_t br = read(fd, hdr + off, hdr_size - off);
>> +switch (br) {
>> +case 0:
>> +error_setg(errp, "File too short");
>> +goto fail;
>> +case -1:
>> +error_setg_errno(errp, errno, "Failed to read file");
>> +goto fail;
>> +}
>> +off += br;
>> +}
>> +
>> +fail:
>> +close(fd);
>> +}
>> +
>>  /* return < 0 if error, otherwise the number of bytes loaded in memory */
>>  int load_elf(const char *filename, uint64_t (*translate_fn)(void *, 
>> uint64_t),
>>   void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
>> diff --git a/include/hw/loader.h b/include/hw/loader.h
>> index f7b43ab..33067f8 100644
>> --- a/include/hw/loader.h
>> +++ b/include/hw/loader.h
>> @@ -36,6 +36,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);
>> +void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error 
>> **errp);
>
> Doc comment, please.
>

Added:

+
+/** load_elf_hdr:
+ * @filename: Path of ELF file
+ * @hdr: Buffer to populate with header data. Header data will not be
+ * filled if set to NULL.
+ * @is64: Set to true if the ELF is 64bit. Ignored if set to NULL
+ * @errp: Populated with an error in failure cases
+ *
+ * Inspect as ELF file's header. Read its full header contents into a
+ * buffer and/or determine if the ELF is 64bit.
+ */


Regards,
Peter

>>  int load_aout(const char *filename, hwaddr addr, int max_sz,
>>int bswap_needed, hwaddr target_page_size);
>>  int load_uimage(const char *filename, hwaddr *ep,
>> --
>> 1.9.1
>
> thanks
> -- PMM

Re: [Qemu-devel] [PATCH v1 15/17] loader: add API to load elf header

[Peter Maydell]

On 18 January 2016 at 07:12, Peter Crosthwaite
 wrote:
> Add an API to load an elf header header from a file. Populates a
> buffer with the header contents, as well as a boolean for whether the
> elf is 64b or not. Both arguments are optional.
>
> Signed-off-by: Peter Crosthwaite 
> ---
>
>  hw/core/loader.c| 48 
>  include/hw/loader.h |  1 +
>  2 files changed, 49 insertions(+)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 6b69852..28da8e2 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -331,6 +331,54 @@ const char *load_elf_strerror(int error)
>  }
>  }
>
> +void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp)
> +{
> +int fd;
> +uint8_t e_ident[EI_NIDENT];
> +size_t hdr_size, off = 0;
> +bool is64l;
> +
> +fd = open(filename, O_RDONLY | O_BINARY);
> +if (fd < 0) {
> +error_setg_errno(errp, errno, "Fail to open file");

"Failed" (also below).

I don't think we end up with the filename anywhere in the
error message; it would be helpful if we could include it.

> +return;
> +}
> +if (read(fd, e_ident, sizeof(e_ident)) != sizeof(e_ident)) {
> +error_setg_errno(errp, errno, "Fail to read file");
> +goto fail;
> +}
> +if (e_ident[0] != ELFMAG0 ||
> +e_ident[1] != ELFMAG1 ||
> +e_ident[2] != ELFMAG2 ||
> +e_ident[3] != ELFMAG3) {
> +error_setg(errp, "Bad ELF magic");
> +goto fail;
> +}
> +
> +is64l = e_ident[EI_CLASS] == ELFCLASS64;
> +hdr_size = is64l ? sizeof(Elf64_Ehdr) : sizeof(Elf32_Ehdr);
> +if (is64) {
> +*is64 = is64l;
> +}
> +
> +lseek(fd, 0, SEEK_SET);

You're not checking this lseek for failure (and you don't
need it anyway, because you could just copy the magic bytes
into *hdr and read four fewer bytes).

> +while (hdr && off < hdr_size) {
> +size_t br = read(fd, hdr + off, hdr_size - off);
> +switch (br) {
> +case 0:
> +error_setg(errp, "File too short");
> +goto fail;
> +case -1:
> +error_setg_errno(errp, errno, "Failed to read file");
> +goto fail;
> +}
> +off += br;
> +}
> +
> +fail:
> +close(fd);
> +}
> +
>  /* return < 0 if error, otherwise the number of bytes loaded in memory */
>  int load_elf(const char *filename, uint64_t (*translate_fn)(void *, 
> uint64_t),
>   void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
> diff --git a/include/hw/loader.h b/include/hw/loader.h
> index f7b43ab..33067f8 100644
> --- a/include/hw/loader.h
> +++ b/include/hw/loader.h
> @@ -36,6 +36,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);
> +void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);

Doc comment, please.

>  int load_aout(const char *filename, hwaddr addr, int max_sz,
>int bswap_needed, hwaddr target_page_size);
>  int load_uimage(const char *filename, hwaddr *ep,
> --
> 1.9.1

thanks
-- PMM

[Qemu-devel] [PATCH v1 15/17] loader: add API to load elf header

[Peter Crosthwaite]

Add an API to load an elf header header from a file. Populates a
buffer with the header contents, as well as a boolean for whether the
elf is 64b or not. Both arguments are optional.

Signed-off-by: Peter Crosthwaite 
---

 hw/core/loader.c| 48 
 include/hw/loader.h |  1 +
 2 files changed, 49 insertions(+)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 6b69852..28da8e2 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -331,6 +331,54 @@ const char *load_elf_strerror(int error)
 }
 }
 
+void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp)
+{
+int fd;
+uint8_t e_ident[EI_NIDENT];
+size_t hdr_size, off = 0;
+bool is64l;
+
+fd = open(filename, O_RDONLY | O_BINARY);
+if (fd < 0) {
+error_setg_errno(errp, errno, "Fail to open file");
+return;
+}
+if (read(fd, e_ident, sizeof(e_ident)) != sizeof(e_ident)) {
+error_setg_errno(errp, errno, "Fail to read file");
+goto fail;
+}
+if (e_ident[0] != ELFMAG0 ||
+e_ident[1] != ELFMAG1 ||
+e_ident[2] != ELFMAG2 ||
+e_ident[3] != ELFMAG3) {
+error_setg(errp, "Bad ELF magic");
+goto fail;
+}
+
+is64l = e_ident[EI_CLASS] == ELFCLASS64;
+hdr_size = is64l ? sizeof(Elf64_Ehdr) : sizeof(Elf32_Ehdr);
+if (is64) {
+*is64 = is64l;
+}
+
+lseek(fd, 0, SEEK_SET);
+while (hdr && off < hdr_size) {
+size_t br = read(fd, hdr + off, hdr_size - off);
+switch (br) {
+case 0:
+error_setg(errp, "File too short");
+goto fail;
+case -1:
+error_setg_errno(errp, errno, "Failed to read file");
+goto fail;
+}
+off += br;
+}
+
+fail:
+close(fd);
+}
+
 /* return < 0 if error, otherwise the number of bytes loaded in memory */
 int load_elf(const char *filename, uint64_t (*translate_fn)(void *, uint64_t),
  void *translate_opaque, uint64_t *pentry, uint64_t *lowaddr,
diff --git a/include/hw/loader.h b/include/hw/loader.h
index f7b43ab..33067f8 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -36,6 +36,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);
+void load_elf_hdr(const char *filename, void *hdr, bool *is64, Error **errp);
 int load_aout(const char *filename, hwaddr addr, int max_sz,
   int bswap_needed, hwaddr target_page_size);
 int load_uimage(const char *filename, hwaddr *ep,
-- 
1.9.1