[PATCH v4] hw/core/loader: allow loading larger ROMs

[PATCH v4] hw/core/loader: allow loading larger ROMs

[Gregor Haas]

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 g_file_get_contents() instead, which correctly reads all data using
multiple calls to read() while also returning the loaded ROM size.

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

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 2f8105d7de..44444a5714 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1075,8 +1075,7 @@ ssize_t rom_add_file(const char *file, const char *fw_dir,
 {
     MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
     Rom *rom;
-    ssize_t rc;
-    int fd = -1;
+    g_autoptr(GError) gerr = NULL;
     char devpath[100];
 
     if (as && mr) {
@@ -1094,35 +1093,19 @@ ssize_t rom_add_file(const char *file, const char *fw_dir,
         rom->path = g_strdup(file);
     }
 
-    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));
-        goto err;
-    }
-
     if (fw_dir) {
         rom->fw_dir  = g_strdup(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));
+    if (!g_file_get_contents(rom->path, (gchar **) &rom->data,
+                             &rom->romsize, &gerr)) {
+        fprintf(stderr, "rom: file %-20s: error %s\n",
+                rom->name, gerr->message);
         goto err;
     }
 
     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=%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;
@@ -1159,9 +1142,6 @@ ssize_t rom_add_file(const char *file, const char *fw_dir,
     return 0;
 
 err:
-    if (fd != -1)
-        close(fd);
-
     rom_free(rom);
     return -1;
 }
-- 
2.45.2

Re: [PATCH v4] hw/core/loader: allow loading larger ROMs

[Daniel P. Berrangé]

On Fri, Jun 28, 2024 at 11:27:06AM -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 g_file_get_contents() instead, which correctly reads all data using
> multiple calls to read() while also returning the loaded ROM size.
> 
> Signed-off-by: Gregor Haas <gregorhaas1997@gmail.com>
> ---
>  hw/core/loader.c | 30 +++++-------------------------
>  1 file changed, 5 insertions(+), 25 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


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 :|

RE: [PATCH v4] hw/core/loader: allow loading larger ROMs

[Xingtao Yao (Fujitsu) via]

Reviewed-by: Xingtao Yao <yaoxt.fnst@fujitsu.com>

> -----Original Message-----
> From: Gregor Haas <gregorhaas1997@gmail.com>
> Sent: Saturday, June 29, 2024 2:27 AM
> To: qemu-devel@nongnu.org
> Cc: berrange@redhat.com; Yao, Xingtao/姚 幸涛 <yaoxt.fnst@fujitsu.com>;
> Gregor Haas <gregorhaas1997@gmail.com>
> Subject: [PATCH v4] 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 g_file_get_contents() instead, which correctly reads all data using
> multiple calls to read() while also returning the loaded ROM size.
> 
> Signed-off-by: Gregor Haas <gregorhaas1997@gmail.com>
> ---
>  hw/core/loader.c | 30 +++++-------------------------
>  1 file changed, 5 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 2f8105d7de..44444a5714 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1075,8 +1075,7 @@ ssize_t rom_add_file(const char *file, const char *fw_dir,
>  {
>      MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine());
>      Rom *rom;
> -    ssize_t rc;
> -    int fd = -1;
> +    g_autoptr(GError) gerr = NULL;
>      char devpath[100];
> 
>      if (as && mr) {
> @@ -1094,35 +1093,19 @@ ssize_t rom_add_file(const char *file, const char
> *fw_dir,
>          rom->path = g_strdup(file);
>      }
> 
> -    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));
> -        goto err;
> -    }
> -
>      if (fw_dir) {
>          rom->fw_dir  = g_strdup(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));
> +    if (!g_file_get_contents(rom->path, (gchar **) &rom->data,
> +                             &rom->romsize, &gerr)) {
> +        fprintf(stderr, "rom: file %-20s: error %s\n",
> +                rom->name, gerr->message);
>          goto err;
>      }
> 
>      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=%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;
> @@ -1159,9 +1142,6 @@ ssize_t rom_add_file(const char *file, const char *fw_dir,
>      return 0;
> 
>  err:
> -    if (fd != -1)
> -        close(fd);
> -
>      rom_free(rom);
>      return -1;
>  }
> --
> 2.45.2

Re: [PATCH v4] hw/core/loader: allow loading larger ROMs

[Philippe Mathieu-Daudé]

On 28/6/24 20:27, 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 g_file_get_contents() instead, which correctly reads all data using
> multiple calls to read() while also returning the loaded ROM size.
> 
> Signed-off-by: Gregor Haas <gregorhaas1997@gmail.com>
> ---
>   hw/core/loader.c | 30 +++++-------------------------
>   1 file changed, 5 insertions(+), 25 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

and queued, thanks!