The read() syscall is not guaranteed to return all data from a file. The
default ROM loader implementation currently does not take this into account,
instead failing if all bytes are not read at once. This change loads the ROM
using load_image_size() instead, which correctly reads all data using
multiple calls to read().

Signed-off-by: Gregor Haas <gregorhaas1997@gmail.com>
---
 hw/core/loader.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 2f8105d7de..8216781a75 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1115,14 +1115,13 @@ ssize_t 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);
+    close(fd);
+    rc = load_image_size(rom->path, rom->data, rom->datasize);
     if (rc != rom->datasize) {
         fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected %zd)\n",
                 rom->name, rc, rom->datasize);
         goto err;
     }
-    close(fd);
     rom_insert(rom);
     if (rom->fw_file && fw_cfg) {
         const char *basename;
-- 
2.45.2

On Thu, Jun 27, 2024 at 05:58:17PM -0700, Gregor Haas wrote:
> The read() syscall is not guaranteed to return all data from a file. The
> default ROM loader implementation currently does not take this into account,
> instead failing if all bytes are not read at once. This change loads the ROM
> using load_image_size() instead, which correctly reads all data using
> multiple calls to read().
> 
> Signed-off-by: Gregor Haas <gregorhaas1997@gmail.com>
> ---
>  hw/core/loader.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 2f8105d7de..8216781a75 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1115,14 +1115,13 @@ ssize_t 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);
> +    close(fd);
> +    rc = load_image_size(rom->path, rom->data, rom->datasize);
>      if (rc != rom->datasize) {
>          fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected %zd)\n",
>                  rom->name, rc, rom->datasize);
>          goto err;
>      }
> -    close(fd);

This method can be simplified much more.
All of the original 'open', lseek, g_malloc0, read & close, can be
replaced by something approximately like this (untested):

   g_autoptr(GError) gerr = NULL;
   if (!g_file_get_contents(file, &rom->data, &rom->datasize, &gerr)) {
       fprintf(stderr, "unable to load ROM '%s': %s", file, gerr->message);
       goto err;
   }
   

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

> -----Original Message-----
> From: Gregor Haas <gregorhaas1997@gmail.com>
> Sent: Friday, June 28, 2024 8:58 AM
> To: qemu-devel@nongnu.org
> Cc: Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>; Gregor Haas
> <gregorhaas1997@gmail.com>
> Subject: [PATCH v2] hw/core/loader: allow loading larger ROMs
> 
> The read() syscall is not guaranteed to return all data from a file. The
> default ROM loader implementation currently does not take this into account,
> instead failing if all bytes are not read at once. This change loads the ROM
> using load_image_size() instead, which correctly reads all data using
> multiple calls to read().
> 
> Signed-off-by: Gregor Haas <gregorhaas1997@gmail.com>
> ---
>  hw/core/loader.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 2f8105d7de..8216781a75 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1115,14 +1115,13 @@ ssize_t 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);
> +    close(fd);
> +    rc = load_image_size(rom->path, rom->data, rom->datasize);
LGTM.

I think we may get romsize by get_image_size() and delete the redundant code like below:
-    fd = open(rom->path, O_RDONLY | O_BINARY);
-    if (fd == -1) {
-        fprintf(stderr, "Could not open option rom '%s': %s\n",
-                rom->path, strerror(errno));
+    rom->romesize = get_image_size(rom->path);
+    if (rom->romsize == -1) {
+        fprintf(stderr, "rom: file %-20s: get size error: %s\n",
+                rom->name, strerror(errno));
         goto err;
     }

@@ -1106,16 +1106,9 @@ ssize_t rom_add_file(const char *file, const char *fw_dir,
         rom->fw_file = g_strdup(file);
     }
     rom->addr     = addr;
-    rom->romsize  = lseek(fd, 0, SEEK_END);
-    if (rom->romsize == -1) {
-        fprintf(stderr, "rom: file %-20s: get size error: %s\n",
-                rom->name, strerror(errno));
-        goto err;
-    }

     rom->datasize = rom->romsize;
     rom->data     = g_malloc0(rom->datasize);
-    lseek(fd, 0, SEEK_SET);

>      if (rc != rom->datasize) {
>          fprintf(stderr, "rom: file %-20s: read error: rc=%zd (expected %zd)\n",
>                  rom->name, rc, rom->datasize);
>          goto err;
>      }
> -    close(fd);
>      rom_insert(rom);
>      if (rom->fw_file && fw_cfg) {
>          const char *basename;
> --
> 2.45.2