[1/2] extract file_load() function from rom_add_file() for reusing

Submitted by Olivia Yin on Aug. 14, 2012, 7:49 a.m.

Details

Message ID 1344930594-18911-1-git-send-email-hong-hua.yin@freescale.com
State New
Headers show

Commit Message

Olivia Yin Aug. 14, 2012, 7:49 a.m.
Sanity check in rom_add_file() could be reused by other image loaders.

Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
---
This patch is based on branch 'ppc-next' of Alex's upstream QEMU repo:
http://repo.or.cz/r/qemu/agraf.git

 hw/loader.c |   61 +++++++++++++++++++++++++++++++---------------------------
 1 files changed, 33 insertions(+), 28 deletions(-)

Comments

Yin Olivia-R63875 Aug. 16, 2012, 11:13 a.m.
Hi Avi & Ben,

I've got feedback from qemu-ppc list and revised this patch according to Blue's comments.
Could you please give any comments? 

Best Regards,
Olivia

> -----Original Message-----
> From: Yin Olivia-R63875
> Sent: Tuesday, August 14, 2012 3:50 PM
> To: qemu-ppc@nongnu.org; qemu-devel@nongnu.org
> Cc: Yin Olivia-R63875
> Subject: [PATCH 1/2] extract file_load() function from rom_add_file() for
> reusing
> 
> Sanity check in rom_add_file() could be reused by other image loaders.
> 
> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> ---
> This patch is based on branch 'ppc-next' of Alex's upstream QEMU repo:
> http://repo.or.cz/r/qemu/agraf.git
> 
>  hw/loader.c |   61 +++++++++++++++++++++++++++++++----------------------
> -----
>  1 files changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/hw/loader.c b/hw/loader.c
> index 33acc2f..f2099b6 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -86,6 +86,36 @@ int load_image(const char *filename, uint8_t *addr)
>      return size;
>  }
> 
> +static int file_load(const char *file, uint8_t **data) {
> +    int fd = -1;
> +    ssize_t rc, size;
> +
> +    fd = open(file, O_RDONLY | O_BINARY);
> +    if (fd == -1) {
> +        fprintf(stderr, "Could not open file '%s': %s\n",
> +                file, strerror(errno));
> +        return -1;
> +    }
> +
> +    size = lseek(fd, 0, SEEK_END);
> +    *data = g_malloc0(size);
> +    lseek(fd, 0, SEEK_SET);
> +    rc = read(fd, *data, size);
> +    if (rc != size) {
> +        fprintf(stderr, "file %-20s: read error: rc=%zd
> (expected %zd)\n",
> +                file, rc, size);
> +        goto err;
> +    }
> +    close(fd);
> +    return size;
> +err:
> +    if (fd != -1)
> +        close(fd);
> +    g_free(*data);
> +    return -1;
> +}
> +
>  /* read()-like version */
>  ssize_t read_targphys(const char *name,
>                        int fd, target_phys_addr_t dst_addr, size_t nbytes)
> @@ -568,38 +598,22 @@ int rom_add_file(const char *file, const char
> *fw_dir,
>                   target_phys_addr_t addr, int32_t bootindex)  {
>      Rom *rom;
> -    int rc, fd = -1;
>      char devpath[100];
> 
>      rom = g_malloc0(sizeof(*rom));
>      rom->name = g_strdup(file);
> +    rom->addr = addr;
>      rom->path = qemu_find_file(QEMU_FILE_TYPE_BIOS, rom->name);
>      if (rom->path == NULL) {
>          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);
> -    rom->data    = g_malloc0(rom->romsize);
> -    lseek(fd, 0, SEEK_SET);
> -    rc = read(fd, rom->data, rom->romsize);
> -    if (rc != rom->romsize) {
> -        fprintf(stderr, "rom: file %-20s: read error: rc=%d
> (expected %zd)\n",
> -                rom->name, rc, rom->romsize);
> -        goto err;
> -    }
> -    close(fd);
> +
> +    rom->romsize = file_load(rom->path, &rom->data);
>      rom_insert(rom);
>      if (rom->fw_file && fw_cfg) {
>          const char *basename;
> @@ -621,15 +635,6 @@ int rom_add_file(const char *file, const char
> *fw_dir,
> 
>      add_boot_device_path(bootindex, NULL, devpath);
>      return 0;
> -
> -err:
> -    if (fd != -1)
> -        close(fd);
> -    g_free(rom->data);
> -    g_free(rom->path);
> -    g_free(rom->name);
> -    g_free(rom);
> -    return -1;
>  }
> 
>  int rom_add_blob(const char *name, const void *blob, size_t len,
> --
> 1.7.1
Avi Kivity Aug. 16, 2012, 11:22 a.m.
On 08/16/2012 02:13 PM, Yin Olivia-R63875 wrote:
> Hi Avi & Ben,
> 
> I've got feedback from qemu-ppc list and revised this patch according to Blue's comments.
> Could you please give any comments? 


I don't really have any comments, those files are outside my area of
expertise.  They look okay to me, but that doesn't mean much as I'm not
familiar with ppc.
Peter Maydell Aug. 16, 2012, 11:36 a.m.
On 14 August 2012 08:49, Olivia Yin <hong-hua.yin@freescale.com> wrote:
> Sanity check in rom_add_file() could be reused by other image loaders.
>
> Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>
> ---
> This patch is based on branch 'ppc-next' of Alex's upstream QEMU repo:
> http://repo.or.cz/r/qemu/agraf.git
>
>  hw/loader.c |   61 +++++++++++++++++++++++++++++++---------------------------
>  1 files changed, 33 insertions(+), 28 deletions(-)
>
> diff --git a/hw/loader.c b/hw/loader.c
> index 33acc2f..f2099b6 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -86,6 +86,36 @@ int load_image(const char *filename, uint8_t *addr)
>      return size;
>  }
>
> +static int file_load(const char *file, uint8_t **data)
> +{
> +    int fd = -1;
> +    ssize_t rc, size;
> +
> +    fd = open(file, O_RDONLY | O_BINARY);
> +    if (fd == -1) {
> +        fprintf(stderr, "Could not open file '%s': %s\n",
> +                file, strerror(errno));
> +        return -1;
> +    }
> +
> +    size = lseek(fd, 0, SEEK_END);
> +    *data = g_malloc0(size);
> +    lseek(fd, 0, SEEK_SET);
> +    rc = read(fd, *data, size);
> +    if (rc != size) {
> +        fprintf(stderr, "file %-20s: read error: rc=%zd (expected %zd)\n",
> +                file, rc, size);
> +        goto err;
> +    }
> +    close(fd);
> +    return size;
> +err:
> +    if (fd != -1)
> +        close(fd);
> +    g_free(*data);
> +    return -1;
> +}

Isn't this function effectively a reimplementation of the glib
g_file_get_contents() function? It would probably be better to
just make the callers use that instead.

-- PMM
Yin Olivia-R63875 Aug. 17, 2012, 9:40 a.m.
Hi Peter,

Thanks for the reminder. I'll update the second patch to use g_file_get_contents().

Best Regards,
Olivia

> -----Original Message-----

> From: Peter Maydell [mailto:peter.maydell@linaro.org]

> Sent: Thursday, August 16, 2012 7:36 PM

> To: Yin Olivia-R63875

> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org

> Subject: Re: [Qemu-devel] [PATCH 1/2] extract file_load() function from

> rom_add_file() for reusing

> 

> On 14 August 2012 08:49, Olivia Yin <hong-hua.yin@freescale.com> wrote:

> > Sanity check in rom_add_file() could be reused by other image loaders.

> >

> > Signed-off-by: Olivia Yin <hong-hua.yin@freescale.com>

> > ---

> > This patch is based on branch 'ppc-next' of Alex's upstream QEMU repo:

> > http://repo.or.cz/r/qemu/agraf.git

> >

> >  hw/loader.c |   61 +++++++++++++++++++++++++++++++--------------------

> -------

> >  1 files changed, 33 insertions(+), 28 deletions(-)

> >

> > diff --git a/hw/loader.c b/hw/loader.c index 33acc2f..f2099b6 100644

> > --- a/hw/loader.c

> > +++ b/hw/loader.c

> > @@ -86,6 +86,36 @@ int load_image(const char *filename, uint8_t *addr)

> >      return size;

> >  }

> >

> > +static int file_load(const char *file, uint8_t **data) {

> > +    int fd = -1;

> > +    ssize_t rc, size;

> > +

> > +    fd = open(file, O_RDONLY | O_BINARY);

> > +    if (fd == -1) {

> > +        fprintf(stderr, "Could not open file '%s': %s\n",

> > +                file, strerror(errno));

> > +        return -1;

> > +    }

> > +

> > +    size = lseek(fd, 0, SEEK_END);

> > +    *data = g_malloc0(size);

> > +    lseek(fd, 0, SEEK_SET);

> > +    rc = read(fd, *data, size);

> > +    if (rc != size) {

> > +        fprintf(stderr, "file %-20s: read error: rc=%zd

> (expected %zd)\n",

> > +                file, rc, size);

> > +        goto err;

> > +    }

> > +    close(fd);

> > +    return size;

> > +err:

> > +    if (fd != -1)

> > +        close(fd);

> > +    g_free(*data);

> > +    return -1;

> > +}

> 

> Isn't this function effectively a reimplementation of the glib

> g_file_get_contents() function? It would probably be better to just make

> the callers use that instead.

> 

> -- PMM
Yin Olivia-R63875 Aug. 17, 2012, 9:42 a.m.
Hi Avi,

Thanks. Exactly the second patch is more important which saves the QEMU memory.

Best Regards,
Olivia

> -----Original Message-----
> From: Avi Kivity [mailto:avi@redhat.com]
> Sent: Thursday, August 16, 2012 7:23 PM
> To: Yin Olivia-R63875
> Cc: qemu-ppc@nongnu.org; qemu-devel@nongnu.org; blauwirbel@gmail.com;
> benh@kernel.crashing.org
> Subject: Re: [PATCH 1/2] extract file_load() function from rom_add_file()
> for reusing
> 
> On 08/16/2012 02:13 PM, Yin Olivia-R63875 wrote:
> > Hi Avi & Ben,
> >
> > I've got feedback from qemu-ppc list and revised this patch according
> to Blue's comments.
> > Could you please give any comments?
> 
> 
> I don't really have any comments, those files are outside my area of
> expertise.  They look okay to me, but that doesn't mean much as I'm not
> familiar with ppc.
> 
> --
> error compiling committee.c: too many arguments to function

Patch hide | download patch | download mbox

diff --git a/hw/loader.c b/hw/loader.c
index 33acc2f..f2099b6 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -86,6 +86,36 @@  int load_image(const char *filename, uint8_t *addr)
     return size;
 }
 
+static int file_load(const char *file, uint8_t **data)
+{
+    int fd = -1;
+    ssize_t rc, size;
+
+    fd = open(file, O_RDONLY | O_BINARY);
+    if (fd == -1) {
+        fprintf(stderr, "Could not open file '%s': %s\n",
+                file, strerror(errno));
+        return -1;
+    }
+
+    size = lseek(fd, 0, SEEK_END);
+    *data = g_malloc0(size);
+    lseek(fd, 0, SEEK_SET);
+    rc = read(fd, *data, size);
+    if (rc != size) {
+        fprintf(stderr, "file %-20s: read error: rc=%zd (expected %zd)\n",
+                file, rc, size);
+        goto err;
+    }
+    close(fd);
+    return size;
+err:
+    if (fd != -1)
+        close(fd);
+    g_free(*data);
+    return -1;
+}
+
 /* read()-like version */
 ssize_t read_targphys(const char *name,
                       int fd, target_phys_addr_t dst_addr, size_t nbytes)
@@ -568,38 +598,22 @@  int rom_add_file(const char *file, const char *fw_dir,
                  target_phys_addr_t addr, int32_t bootindex)
 {
     Rom *rom;
-    int rc, fd = -1;
     char devpath[100];
 
     rom = g_malloc0(sizeof(*rom));
     rom->name = g_strdup(file);
+    rom->addr = addr;
     rom->path = qemu_find_file(QEMU_FILE_TYPE_BIOS, rom->name);
     if (rom->path == NULL) {
         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);
-    rom->data    = g_malloc0(rom->romsize);
-    lseek(fd, 0, SEEK_SET);
-    rc = read(fd, rom->data, rom->romsize);
-    if (rc != rom->romsize) {
-        fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n",
-                rom->name, rc, rom->romsize);
-        goto err;
-    }
-    close(fd);
+
+    rom->romsize = file_load(rom->path, &rom->data);
     rom_insert(rom);
     if (rom->fw_file && fw_cfg) {
         const char *basename;
@@ -621,15 +635,6 @@  int rom_add_file(const char *file, const char *fw_dir,
 
     add_boot_device_path(bootindex, NULL, devpath);
     return 0;
-
-err:
-    if (fd != -1)
-        close(fd);
-    g_free(rom->data);
-    g_free(rom->path);
-    g_free(rom->name);
-    g_free(rom);
-    return -1;
 }
 
 int rom_add_blob(const char *name, const void *blob, size_t len,