qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] loader: support loading large files (>=2GB)

From: Philippe Mathieu-Daudé
Subject: Re: [PATCH] loader: support loading large files (>=2GB)
Date: Mon, 30 May 2022 16:58:06 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 
Hi Peter,

On 28/4/22 01:07, Peter Collingbourne wrote:

Currently the loader uses int as the return type for various APIs
that deal with file sizes, which leads to an error if the file
size is >=2GB, as it ends up being interpreted as a negative error
code. Furthermore, we do not tolerate short reads, which are possible
at least on Linux when attempting to read such large files in one
syscall.

Fix the first problem by switching to 64-bit types for file sizes,
and fix the second by introducing a loop around the read syscall.


Hmm maybe worth rebasing on this patch from Jamie which also
fixes the format on 32-bit hosts:
https://lore.kernel.org/qemu-devel/20211111141141.3295094-2-jamie@nuviainc.com/

(Personally I prefer to read ssize_t while reviewing instead
of int64_t).

While here, please have a look at this other patch from Jamie:
https://lore.kernel.org/qemu-devel/20211111141141.3295094-3-jamie@nuviainc.com/

Also, Cc'ing the maintainer:

$ ./scripts/get_maintainer.pl -f hw/core/generic-loader.c
Alistair Francis <alistair@alistair23.me> (maintainer:Generic Loader)


Signed-off-by: Peter Collingbourne <pcc@google.com>
---
  hw/core/generic-loader.c |  2 +-
  hw/core/loader.c         | 44 ++++++++++++++++++++++++----------------
  include/hw/loader.h      | 13 ++++++------
  3 files changed, 34 insertions(+), 25 deletions(-)

diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
index c666545aa0..0891fa73c3 100644
--- a/hw/core/generic-loader.c
+++ b/hw/core/generic-loader.c
@@ -67,7 +67,7 @@ static void generic_loader_realize(DeviceState *dev, Error 
**errp)
      GenericLoaderState *s = GENERIC_LOADER(dev);
      hwaddr entry;
      int big_endian;
-    int size = 0;
+    int64_t size = 0;
s->set_pc = false; diff --git a/hw/core/loader.c b/hw/core/loader.c 
index ca2f2431fb..d07c79c400 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -115,17 +115,17 @@ ssize_t read_targphys(const char *name,
      return did;
  }
-int load_image_targphys(const char *filename, 
-                        hwaddr addr, uint64_t max_sz)
+int64_t load_image_targphys(const char *filename,
+                            hwaddr addr, uint64_t max_sz)
  {
      return load_image_targphys_as(filename, addr, max_sz, NULL);
  }
/* return the size or -1 if error */ 
-int load_image_targphys_as(const char *filename,
-                           hwaddr addr, uint64_t max_sz, AddressSpace *as)
+int64_t load_image_targphys_as(const char *filename,
+                               hwaddr addr, uint64_t max_sz, AddressSpace *as)
  {
-    int size;
+    int64_t size;
size = get_image_size(filename); 
      if (size < 0 || size > max_sz) {
@@ -139,9 +139,9 @@ int load_image_targphys_as(const char *filename,
      return size;
  }
-int load_image_mr(const char *filename, MemoryRegion *mr) 
+int64_t load_image_mr(const char *filename, MemoryRegion *mr)
  {
-    int size;
+    int64_t size;
if (!memory_access_is_direct(mr, false)) { 
          /* Can only load an image into RAM or ROM */
@@ -963,7 +963,8 @@ int rom_add_file(const char *file, const char *fw_dir,
  {
      MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
      Rom *rom;
-    int rc, fd = -1;
+    int fd = -1;
+    size_t bytes_read = 0;
      char devpath[100];
if (as && mr) { 
@@ -1003,11 +1004,17 @@ int rom_add_file(const char *file, const char *fw_dir,
      rom->datasize = rom->romsize;
      rom->data     = g_malloc0(rom->datasize);
      lseek(fd, 0, SEEK_SET);
-    rc = read(fd, rom->data, rom->datasize);
-    if (rc != rom->datasize) {
-        fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n",
-                rom->name, rc, rom->datasize);
-        goto err;
+    while (bytes_read < rom->datasize) {
+        ssize_t rc =
+            read(fd, rom->data + bytes_read, rom->datasize - bytes_read);
+        if (rc <= 0) {
+            fprintf(stderr,
+                    "rom: file %-20s: read error: rc=%zd at position %zd "
+                    "(expected size %zd)\n",
+                    rom->name, rc, bytes_read, rom->datasize);
+            goto err;
+        }
+        bytes_read += rc;
      }
      close(fd);
      rom_insert(rom);
@@ -1671,7 +1678,7 @@ typedef struct {
      HexLine line;
      uint8_t *bin_buf;
      hwaddr *start_addr;
-    int total_size;
+    int64_t total_size;
      uint32_t next_address_to_write;
      uint32_t current_address;
      uint32_t current_rom_index;
@@ -1767,8 +1774,8 @@ static int handle_record_type(HexParser *parser)
  }
/* return size or -1 if error */ 
-static int parse_hex_blob(const char *filename, hwaddr *addr, uint8_t 
*hex_blob,
-                          size_t hex_blob_size, AddressSpace *as)
+static int64_t parse_hex_blob(const char *filename, hwaddr *addr, uint8_t 
*hex_blob,
+                              size_t hex_blob_size, AddressSpace *as)
  {
      bool in_process = false; /* avoid re-enter and
                                * check whether record begin with ':' */
@@ -1832,11 +1839,12 @@ out:
  }
/* return size or -1 if error */ 
-int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace *as)
+int64_t load_targphys_hex_as(const char *filename, hwaddr *entry,
+                             AddressSpace *as)
  {
      gsize hex_blob_size;
      gchar *hex_blob;
-    int total_size = 0;
+    int64_t total_size = 0;
if (!g_file_get_contents(filename, &hex_blob, &hex_blob_size, NULL)) { 
          return -1;
diff --git a/include/hw/loader.h b/include/hw/loader.h
index 5572108ba5..7b09705940 100644
--- a/include/hw/loader.h
+++ b/include/hw/loader.h
@@ -40,8 +40,8 @@ ssize_t load_image_size(const char *filename, void *addr, 
size_t size);
   *
   * Returns the size of the loaded image on success, -1 otherwise.
   */
-int load_image_targphys_as(const char *filename,
-                           hwaddr addr, uint64_t max_sz, AddressSpace *as);
+int64_t load_image_targphys_as(const char *filename,
+                               hwaddr addr, uint64_t max_sz, AddressSpace *as);
/**load_targphys_hex_as: 
   * @filename: Path to the .hex file
@@ -53,14 +53,15 @@ int load_image_targphys_as(const char *filename,
   *
   * Returns the size of the loaded .hex file on success, -1 otherwise.
   */
-int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace 
*as);
+int64_t load_targphys_hex_as(const char *filename, hwaddr *entry,
+                             AddressSpace *as);
/** load_image_targphys: 
   * Same as load_image_targphys_as(), but doesn't allow the caller to specify
   * an AddressSpace.
   */
-int load_image_targphys(const char *filename, hwaddr,
-                        uint64_t max_sz);
+int64_t load_image_targphys(const char *filename, hwaddr,
+                            uint64_t max_sz);
/** 
   * load_image_mr: load an image into a memory region
@@ -73,7 +74,7 @@ int load_image_targphys(const char *filename, hwaddr,
   * If the file is larger than the memory region's size the call will fail.
   * Returns -1 on failure, or the size of the file.
   */
-int load_image_mr(const char *filename, MemoryRegion *mr);
+int64_t load_image_mr(const char *filename, MemoryRegion *mr);
/* This is the limit on the maximum uncompressed image size that 
   * load_image_gzipped_buffer() and load_image_gzipped() will read. It prevents
reply via email to
[Prev in Thread] Current Thread [Next in Thread]