qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [PATCH 1/2] QEMU: extract file_load() function from rom_a


From: Yin Olivia-R63875
Subject: Re: [Qemu-ppc] [PATCH 1/2] QEMU: extract file_load() function from rom_add_file() for reusing
Date: Tue, 14 Aug 2012 08:09:21 +0000

Thanks for the comments.

Fix will be added into next revision.

Best Regards,
Olivia

-----Original Message-----
From: Blue Swirl [mailto:address@hidden 
Sent: Tuesday, August 14, 2012 4:16 AM
To: Yin Olivia-R63875
Cc: address@hidden
Subject: Re: [Qemu-ppc] [PATCH 1/2] QEMU: extract file_load() function from 
rom_add_file() for reusing

On Mon, Aug 13, 2012 at 11:00 AM, Olivia Yin <address@hidden> wrote:
> Sanity check in rom_add_file() could be reused by other image loaders.
>
> Signed-off-by: Olivia Yin <address@hidden>
> ---
> 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..8475850 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 rc, fd = -1;
> +    int size;

I know this and the below problems stem from the original code, but please fix 
them while refactoring. rc should be ssize_t and size off_t or ssize_t.

> +
> +    fd = open(file, O_RDONLY | O_BINARY);
> +    if (fd == -1) {
> +        fprintf(stderr, "Could not open file '%s': %s\n",
> +                file, strerror(errno));
> +        goto err;

In this case, *data should not be g_free()d since the pointer has not been 
initialized yet.

> +    }
> +
> +    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=%d (expected 
> + %zd)\n",

I'd remove -20. %d will need to be changed to %zd to match rc's ssize_t.

> +                file, rc, size);
> +        goto err;
> +    }
> +    close(fd);
> +    return size;
> +err:
> +    if (fd != -1)
> +        close(fd);

Please add missing braces to match CODING_STYLE.

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


reply via email to

[Prev in Thread] Current Thread [Next in Thread]