| Message ID | 1344930594-18911-1-git-send-email-hong-hua.yin@freescale.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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
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
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,
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(-)