Patchwork [3/4] loader: Add rom_add_file_buf for adding roms from a buffer

login
register
mail settings
Submitter jordan.l.justen@intel.com
Date Oct. 17, 2011, 7:16 p.m.
Message ID <1318878968-18090-3-git-send-email-jordan.l.justen@intel.com>
Download mbox | patch
Permalink /patch/120289/
State New
Headers show

Comments

jordan.l.justen@intel.com - Oct. 17, 2011, 7:16 p.m.
rom_add_file_buf is similar to rom_add_file, except the rom's
contents are provided in a buffer.

rom_add_file is modified to call rom_add_file_buf after
reading the rom's contents from the file.

Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
---
 hw/loader.c |   71 +++++++++++++++++++++++++++++++++++++++-------------------
 hw/loader.h |    5 ++++
 2 files changed, 53 insertions(+), 23 deletions(-)
Blue Swirl - Oct. 18, 2011, 6:05 p.m.
On Mon, Oct 17, 2011 at 7:16 PM, Jordan Justen
<jordan.l.justen@intel.com> wrote:
> rom_add_file_buf is similar to rom_add_file, except the rom's
> contents are provided in a buffer.
>
> rom_add_file is modified to call rom_add_file_buf after
> reading the rom's contents from the file.
>
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> ---
>  hw/loader.c |   71 +++++++++++++++++++++++++++++++++++++++-------------------
>  hw/loader.h |    5 ++++
>  2 files changed, 53 insertions(+), 23 deletions(-)
>
> diff --git a/hw/loader.c b/hw/loader.c
> index 5676c18..d1a4a98 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -557,11 +557,11 @@ static void rom_insert(Rom *rom)
>     QTAILQ_INSERT_TAIL(&roms, rom, next);
>  }
>
> -int rom_add_file(const char *file, const char *fw_dir,
> -                 target_phys_addr_t addr, int32_t bootindex)
> +int rom_add_file_buf(const char *file, const void *data, size_t size,
> +                     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));
> @@ -571,28 +571,16 @@ int 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);
> +    rom->romsize = size;
>     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);
> +
> +    memcpy(rom->data, data, rom->romsize);

This is not optimal, instead the data should be used directly. That
way also mmap()ed, deduplicated ROM files are possible.

> +
>     rom_insert(rom);
>     if (rom->fw_file && fw_cfg) {
>         const char *basename;
> @@ -614,14 +602,51 @@ int rom_add_file(const char *file, const char *fw_dir,
>
>     add_boot_device_path(bootindex, NULL, devpath);
>     return 0;
> +}
> +
> +int rom_add_file(const char *file, const char *fw_dir,
> +                 target_phys_addr_t addr, int32_t bootindex)
> +{
> +    char *filename;
> +    void *data = NULL;
> +    size_t size;
> +    int rc, fd = -1;
> +
> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, file);
> +    if (filename == NULL) {
> +        filename = g_strdup(file);
> +    }
> +
> +    fd = open(filename, O_RDONLY | O_BINARY);
> +    if (fd == -1) {
> +        fprintf(stderr, "Could not open option rom '%s': %s\n",
> +                filename, strerror(errno));
> +        goto err;
> +    }
> +
> +    size = lseek(fd, 0, SEEK_END);
> +    data = g_malloc0(size);
> +    lseek(fd, 0, SEEK_SET);
> +    rc = read(fd, data, size);

It should be easy to replace this with mmap(), maybe later.

> +    if (rc != size) {
> +        fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n",
> +                filename, rc, size);
> +        goto err;
> +    }
> +    close(fd);
> +
> +    rc = rom_add_file_buf(filename, data, size, fw_dir, addr, bootindex);
> +    if (rc != 0) {
> +        goto err;
> +    }
> +
> +    g_free(data);
> +    return 0;
>
>  err:
>     if (fd != -1)
>         close(fd);
> -    g_free(rom->data);
> -    g_free(rom->path);
> -    g_free(rom->name);
> -    g_free(rom);
> +    g_free(data);
>     return -1;
>  }
>
> diff --git a/hw/loader.h b/hw/loader.h
> index fc6bdff..9efe64a 100644
> --- a/hw/loader.h
> +++ b/hw/loader.h
> @@ -21,6 +21,9 @@ void pstrcpy_targphys(const char *name,
>                       const char *source);
>
>
> +int rom_add_file_buf(const char *file, const void *data, size_t size,
> +                     const char *fw_dir,
> +                     target_phys_addr_t addr, int32_t bootindex);
>  int rom_add_file(const char *file, const char *fw_dir,
>                  target_phys_addr_t addr, int32_t bootindex);
>  int rom_add_blob(const char *name, const void *blob, size_t len,
> @@ -31,6 +34,8 @@ int rom_copy(uint8_t *dest, target_phys_addr_t addr, size_t size);
>  void *rom_ptr(target_phys_addr_t addr);
>  void do_info_roms(Monitor *mon);
>
> +#define rom_add_file_buf_fixed(_f, _d, _s, _a, _i)          \
> +    rom_add_file_buf(_f, _d, _s, NULL, _a, _i)
>  #define rom_add_file_fixed(_f, _a, _i)          \
>     rom_add_file(_f, NULL, _a, _i)
>  #define rom_add_blob_fixed(_f, _b, _l, _a)      \
> --
> 1.7.1
>
>
>
Jordan Justen - Oct. 18, 2011, 9:17 p.m.
On Tue, Oct 18, 2011 at 11:05, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Mon, Oct 17, 2011 at 7:16 PM, Jordan Justen
> <jordan.l.justen@intel.com> wrote:
>> rom_add_file_buf is similar to rom_add_file, except the rom's
>> contents are provided in a buffer.
>>
>> rom_add_file is modified to call rom_add_file_buf after
>> reading the rom's contents from the file.
>>
>> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
>> ---
>>  hw/loader.c |   71 +++++++++++++++++++++++++++++++++++++++-------------------
>>  hw/loader.h |    5 ++++
>>  2 files changed, 53 insertions(+), 23 deletions(-)
>>
>> diff --git a/hw/loader.c b/hw/loader.c
>> index 5676c18..d1a4a98 100644
>> --- a/hw/loader.c
>> +++ b/hw/loader.c
>> @@ -557,11 +557,11 @@ static void rom_insert(Rom *rom)
>>     QTAILQ_INSERT_TAIL(&roms, rom, next);
>>  }
>>
>> -int rom_add_file(const char *file, const char *fw_dir,
>> -                 target_phys_addr_t addr, int32_t bootindex)
>> +int rom_add_file_buf(const char *file, const void *data, size_t size,
>> +                     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));
>> @@ -571,28 +571,16 @@ int 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);
>> +    rom->romsize = size;
>>     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);
>> +
>> +    memcpy(rom->data, data, rom->romsize);
>
> This is not optimal, instead the data should be used directly. That
> way also mmap()ed, deduplicated ROM files are possible.

In my 4th patch I use a buffer from a memory region via
memory_region_get_ram_ptr.  Comments for memory_region_get_ram_ptr say
'Use with care'.

So, would the best thing be for me to allocate a new buffer in my 4th
patch, do the memcpy there, and then use the pointer directly here?

Thanks,

-Jordan

>
>> +
>>     rom_insert(rom);
>>     if (rom->fw_file && fw_cfg) {
>>         const char *basename;
>> @@ -614,14 +602,51 @@ int rom_add_file(const char *file, const char *fw_dir,
>>
>>     add_boot_device_path(bootindex, NULL, devpath);
>>     return 0;
>> +}
>> +
>> +int rom_add_file(const char *file, const char *fw_dir,
>> +                 target_phys_addr_t addr, int32_t bootindex)
>> +{
>> +    char *filename;
>> +    void *data = NULL;
>> +    size_t size;
>> +    int rc, fd = -1;
>> +
>> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, file);
>> +    if (filename == NULL) {
>> +        filename = g_strdup(file);
>> +    }
>> +
>> +    fd = open(filename, O_RDONLY | O_BINARY);
>> +    if (fd == -1) {
>> +        fprintf(stderr, "Could not open option rom '%s': %s\n",
>> +                filename, strerror(errno));
>> +        goto err;
>> +    }
>> +
>> +    size = lseek(fd, 0, SEEK_END);
>> +    data = g_malloc0(size);
>> +    lseek(fd, 0, SEEK_SET);
>> +    rc = read(fd, data, size);
>
> It should be easy to replace this with mmap(), maybe later.
>
>> +    if (rc != size) {
>> +        fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n",
>> +                filename, rc, size);
>> +        goto err;
>> +    }
>> +    close(fd);
>> +
>> +    rc = rom_add_file_buf(filename, data, size, fw_dir, addr, bootindex);
>> +    if (rc != 0) {
>> +        goto err;
>> +    }
>> +
>> +    g_free(data);
>> +    return 0;
>>
>>  err:
>>     if (fd != -1)
>>         close(fd);
>> -    g_free(rom->data);
>> -    g_free(rom->path);
>> -    g_free(rom->name);
>> -    g_free(rom);
>> +    g_free(data);
>>     return -1;
>>  }
>>
>> diff --git a/hw/loader.h b/hw/loader.h
>> index fc6bdff..9efe64a 100644
>> --- a/hw/loader.h
>> +++ b/hw/loader.h
>> @@ -21,6 +21,9 @@ void pstrcpy_targphys(const char *name,
>>                       const char *source);
>>
>>
>> +int rom_add_file_buf(const char *file, const void *data, size_t size,
>> +                     const char *fw_dir,
>> +                     target_phys_addr_t addr, int32_t bootindex);
>>  int rom_add_file(const char *file, const char *fw_dir,
>>                  target_phys_addr_t addr, int32_t bootindex);
>>  int rom_add_blob(const char *name, const void *blob, size_t len,
>> @@ -31,6 +34,8 @@ int rom_copy(uint8_t *dest, target_phys_addr_t addr, size_t size);
>>  void *rom_ptr(target_phys_addr_t addr);
>>  void do_info_roms(Monitor *mon);
>>
>> +#define rom_add_file_buf_fixed(_f, _d, _s, _a, _i)          \
>> +    rom_add_file_buf(_f, _d, _s, NULL, _a, _i)
>>  #define rom_add_file_fixed(_f, _a, _i)          \
>>     rom_add_file(_f, NULL, _a, _i)
>>  #define rom_add_blob_fixed(_f, _b, _l, _a)      \
>> --
>> 1.7.1
>>
>>
>>
>
>
Blue Swirl - Oct. 23, 2011, 11:27 a.m.
On Tue, Oct 18, 2011 at 21:17, Jordan Justen <jljusten@gmail.com> wrote:
> On Tue, Oct 18, 2011 at 11:05, Blue Swirl <blauwirbel@gmail.com> wrote:
>> On Mon, Oct 17, 2011 at 7:16 PM, Jordan Justen
>> <jordan.l.justen@intel.com> wrote:
>>> rom_add_file_buf is similar to rom_add_file, except the rom's
>>> contents are provided in a buffer.
>>>
>>> rom_add_file is modified to call rom_add_file_buf after
>>> reading the rom's contents from the file.
>>>
>>> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
>>> ---
>>>  hw/loader.c |   71 +++++++++++++++++++++++++++++++++++++++-------------------
>>>  hw/loader.h |    5 ++++
>>>  2 files changed, 53 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/hw/loader.c b/hw/loader.c
>>> index 5676c18..d1a4a98 100644
>>> --- a/hw/loader.c
>>> +++ b/hw/loader.c
>>> @@ -557,11 +557,11 @@ static void rom_insert(Rom *rom)
>>>     QTAILQ_INSERT_TAIL(&roms, rom, next);
>>>  }
>>>
>>> -int rom_add_file(const char *file, const char *fw_dir,
>>> -                 target_phys_addr_t addr, int32_t bootindex)
>>> +int rom_add_file_buf(const char *file, const void *data, size_t size,
>>> +                     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));
>>> @@ -571,28 +571,16 @@ int 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);
>>> +    rom->romsize = size;
>>>     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);
>>> +
>>> +    memcpy(rom->data, data, rom->romsize);
>>
>> This is not optimal, instead the data should be used directly. That
>> way also mmap()ed, deduplicated ROM files are possible.
>
> In my 4th patch I use a buffer from a memory region via
> memory_region_get_ram_ptr.  Comments for memory_region_get_ram_ptr say
> 'Use with care'.
>
> So, would the best thing be for me to allocate a new buffer in my 4th
> patch, do the memcpy there, and then use the pointer directly here?

No, instead of memcpy just do
rom->data = data;

Then also the corresponding g_free(data) below should be removed.

The line g_free(rom->data) in the error path would be a problem for
the future mmap() case though. Should be solvable with with some
refactoring then, we'd need to be able to munmap() anyway.

> Thanks,
>
> -Jordan
>
>>
>>> +
>>>     rom_insert(rom);
>>>     if (rom->fw_file && fw_cfg) {
>>>         const char *basename;
>>> @@ -614,14 +602,51 @@ int rom_add_file(const char *file, const char *fw_dir,
>>>
>>>     add_boot_device_path(bootindex, NULL, devpath);
>>>     return 0;
>>> +}
>>> +
>>> +int rom_add_file(const char *file, const char *fw_dir,
>>> +                 target_phys_addr_t addr, int32_t bootindex)
>>> +{
>>> +    char *filename;
>>> +    void *data = NULL;
>>> +    size_t size;
>>> +    int rc, fd = -1;
>>> +
>>> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, file);
>>> +    if (filename == NULL) {
>>> +        filename = g_strdup(file);
>>> +    }
>>> +
>>> +    fd = open(filename, O_RDONLY | O_BINARY);
>>> +    if (fd == -1) {
>>> +        fprintf(stderr, "Could not open option rom '%s': %s\n",
>>> +                filename, strerror(errno));
>>> +        goto err;
>>> +    }
>>> +
>>> +    size = lseek(fd, 0, SEEK_END);
>>> +    data = g_malloc0(size);
>>> +    lseek(fd, 0, SEEK_SET);
>>> +    rc = read(fd, data, size);
>>
>> It should be easy to replace this with mmap(), maybe later.
>>
>>> +    if (rc != size) {
>>> +        fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n",
>>> +                filename, rc, size);
>>> +        goto err;
>>> +    }
>>> +    close(fd);
>>> +
>>> +    rc = rom_add_file_buf(filename, data, size, fw_dir, addr, bootindex);
>>> +    if (rc != 0) {
>>> +        goto err;
>>> +    }
>>> +
>>> +    g_free(data);
>>> +    return 0;
>>>
>>>  err:
>>>     if (fd != -1)
>>>         close(fd);
>>> -    g_free(rom->data);
>>> -    g_free(rom->path);
>>> -    g_free(rom->name);
>>> -    g_free(rom);
>>> +    g_free(data);
>>>     return -1;
>>>  }
>>>
>>> diff --git a/hw/loader.h b/hw/loader.h
>>> index fc6bdff..9efe64a 100644
>>> --- a/hw/loader.h
>>> +++ b/hw/loader.h
>>> @@ -21,6 +21,9 @@ void pstrcpy_targphys(const char *name,
>>>                       const char *source);
>>>
>>>
>>> +int rom_add_file_buf(const char *file, const void *data, size_t size,
>>> +                     const char *fw_dir,
>>> +                     target_phys_addr_t addr, int32_t bootindex);
>>>  int rom_add_file(const char *file, const char *fw_dir,
>>>                  target_phys_addr_t addr, int32_t bootindex);
>>>  int rom_add_blob(const char *name, const void *blob, size_t len,
>>> @@ -31,6 +34,8 @@ int rom_copy(uint8_t *dest, target_phys_addr_t addr, size_t size);
>>>  void *rom_ptr(target_phys_addr_t addr);
>>>  void do_info_roms(Monitor *mon);
>>>
>>> +#define rom_add_file_buf_fixed(_f, _d, _s, _a, _i)          \
>>> +    rom_add_file_buf(_f, _d, _s, NULL, _a, _i)
>>>  #define rom_add_file_fixed(_f, _a, _i)          \
>>>     rom_add_file(_f, NULL, _a, _i)
>>>  #define rom_add_blob_fixed(_f, _b, _l, _a)      \
>>> --
>>> 1.7.1
>>>
>>>
>>>
>>
>>
>
Jordan Justen - Oct. 24, 2011, 10:19 p.m.
On Sun, Oct 23, 2011 at 04:27, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Tue, Oct 18, 2011 at 21:17, Jordan Justen <jljusten@gmail.com> wrote:
>> On Tue, Oct 18, 2011 at 11:05, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> On Mon, Oct 17, 2011 at 7:16 PM, Jordan Justen
>>> <jordan.l.justen@intel.com> wrote:
>>>> rom_add_file_buf is similar to rom_add_file, except the rom's
>>>> contents are provided in a buffer.
>>>>
>>>> rom_add_file is modified to call rom_add_file_buf after
>>>> reading the rom's contents from the file.
>>>>
>>>> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
>>>> ---
>>>>  hw/loader.c |   71 +++++++++++++++++++++++++++++++++++++++-------------------
>>>>  hw/loader.h |    5 ++++
>>>>  2 files changed, 53 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/hw/loader.c b/hw/loader.c
>>>> index 5676c18..d1a4a98 100644
>>>> --- a/hw/loader.c
>>>> +++ b/hw/loader.c
>>>> @@ -557,11 +557,11 @@ static void rom_insert(Rom *rom)
>>>>     QTAILQ_INSERT_TAIL(&roms, rom, next);
>>>>  }
>>>>
>>>> -int rom_add_file(const char *file, const char *fw_dir,
>>>> -                 target_phys_addr_t addr, int32_t bootindex)
>>>> +int rom_add_file_buf(const char *file, const void *data, size_t size,
>>>> +                     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));
>>>> @@ -571,28 +571,16 @@ int 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);
>>>> +    rom->romsize = size;
>>>>     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);
>>>> +
>>>> +    memcpy(rom->data, data, rom->romsize);
>>>
>>> This is not optimal, instead the data should be used directly. That
>>> way also mmap()ed, deduplicated ROM files are possible.
>>
>> In my 4th patch I use a buffer from a memory region via
>> memory_region_get_ram_ptr.  Comments for memory_region_get_ram_ptr say
>> 'Use with care'.
>>
>> So, would the best thing be for me to allocate a new buffer in my 4th
>> patch, do the memcpy there, and then use the pointer directly here?
>
> No, instead of memcpy just do
> rom->data = data;
>
> Then also the corresponding g_free(data) below should be removed.
>
> The line g_free(rom->data) in the error path would be a problem for
> the future mmap() case though. Should be solvable with with some
> refactoring then, we'd need to be able to munmap() anyway.

I was discussing this change with Alex, and his opinion was that I
should not need to add the rom_add_file_buf function because the
pflash device is being used.  So, I plan to drop patches 3 & 4 from
this changeset.

Thanks for the suggestion though, and I'll keep it in mind for future changes.

-Jordan

>>>
>>>> +
>>>>     rom_insert(rom);
>>>>     if (rom->fw_file && fw_cfg) {
>>>>         const char *basename;
>>>> @@ -614,14 +602,51 @@ int rom_add_file(const char *file, const char *fw_dir,
>>>>
>>>>     add_boot_device_path(bootindex, NULL, devpath);
>>>>     return 0;
>>>> +}
>>>> +
>>>> +int rom_add_file(const char *file, const char *fw_dir,
>>>> +                 target_phys_addr_t addr, int32_t bootindex)
>>>> +{
>>>> +    char *filename;
>>>> +    void *data = NULL;
>>>> +    size_t size;
>>>> +    int rc, fd = -1;
>>>> +
>>>> +    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, file);
>>>> +    if (filename == NULL) {
>>>> +        filename = g_strdup(file);
>>>> +    }
>>>> +
>>>> +    fd = open(filename, O_RDONLY | O_BINARY);
>>>> +    if (fd == -1) {
>>>> +        fprintf(stderr, "Could not open option rom '%s': %s\n",
>>>> +                filename, strerror(errno));
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    size = lseek(fd, 0, SEEK_END);
>>>> +    data = g_malloc0(size);
>>>> +    lseek(fd, 0, SEEK_SET);
>>>> +    rc = read(fd, data, size);
>>>
>>> It should be easy to replace this with mmap(), maybe later.
>>>
>>>> +    if (rc != size) {
>>>> +        fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected %zd)\n",
>>>> +                filename, rc, size);
>>>> +        goto err;
>>>> +    }
>>>> +    close(fd);
>>>> +
>>>> +    rc = rom_add_file_buf(filename, data, size, fw_dir, addr, bootindex);
>>>> +    if (rc != 0) {
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    g_free(data);
>>>> +    return 0;
>>>>
>>>>  err:
>>>>     if (fd != -1)
>>>>         close(fd);
>>>> -    g_free(rom->data);
>>>> -    g_free(rom->path);
>>>> -    g_free(rom->name);
>>>> -    g_free(rom);
>>>> +    g_free(data);
>>>>     return -1;
>>>>  }
>>>>
>>>> diff --git a/hw/loader.h b/hw/loader.h
>>>> index fc6bdff..9efe64a 100644
>>>> --- a/hw/loader.h
>>>> +++ b/hw/loader.h
>>>> @@ -21,6 +21,9 @@ void pstrcpy_targphys(const char *name,
>>>>                       const char *source);
>>>>
>>>>
>>>> +int rom_add_file_buf(const char *file, const void *data, size_t size,
>>>> +                     const char *fw_dir,
>>>> +                     target_phys_addr_t addr, int32_t bootindex);
>>>>  int rom_add_file(const char *file, const char *fw_dir,
>>>>                  target_phys_addr_t addr, int32_t bootindex);
>>>>  int rom_add_blob(const char *name, const void *blob, size_t len,
>>>> @@ -31,6 +34,8 @@ int rom_copy(uint8_t *dest, target_phys_addr_t addr, size_t size);
>>>>  void *rom_ptr(target_phys_addr_t addr);
>>>>  void do_info_roms(Monitor *mon);
>>>>
>>>> +#define rom_add_file_buf_fixed(_f, _d, _s, _a, _i)          \
>>>> +    rom_add_file_buf(_f, _d, _s, NULL, _a, _i)
>>>>  #define rom_add_file_fixed(_f, _a, _i)          \
>>>>     rom_add_file(_f, NULL, _a, _i)
>>>>  #define rom_add_blob_fixed(_f, _b, _l, _a)      \
>>>> --
>>>> 1.7.1
>>>>
>>>>
>>>>
>>>
>>>
>>
>

Patch

diff --git a/hw/loader.c b/hw/loader.c
index 5676c18..d1a4a98 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -557,11 +557,11 @@  static void rom_insert(Rom *rom)
     QTAILQ_INSERT_TAIL(&roms, rom, next);
 }
 
-int rom_add_file(const char *file, const char *fw_dir,
-                 target_phys_addr_t addr, int32_t bootindex)
+int rom_add_file_buf(const char *file, const void *data, size_t size,
+                     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));
@@ -571,28 +571,16 @@  int 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);
+    rom->romsize = size;
     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);
+
+    memcpy(rom->data, data, rom->romsize);
+
     rom_insert(rom);
     if (rom->fw_file && fw_cfg) {
         const char *basename;
@@ -614,14 +602,51 @@  int rom_add_file(const char *file, const char *fw_dir,
 
     add_boot_device_path(bootindex, NULL, devpath);
     return 0;
+}
+
+int rom_add_file(const char *file, const char *fw_dir,
+                 target_phys_addr_t addr, int32_t bootindex)
+{
+    char *filename;
+    void *data = NULL;
+    size_t size;
+    int rc, fd = -1;
+
+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, file);
+    if (filename == NULL) {
+        filename = g_strdup(file);
+    }
+
+    fd = open(filename, O_RDONLY | O_BINARY);
+    if (fd == -1) {
+        fprintf(stderr, "Could not open option rom '%s': %s\n",
+                filename, strerror(errno));
+        goto err;
+    }
+
+    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, "rom: file %-20s: read error: rc=%d (expected %zd)\n",
+                filename, rc, size);
+        goto err;
+    }
+    close(fd);
+
+    rc = rom_add_file_buf(filename, data, size, fw_dir, addr, bootindex);
+    if (rc != 0) {
+        goto err;
+    }
+
+    g_free(data);
+    return 0;
 
 err:
     if (fd != -1)
         close(fd);
-    g_free(rom->data);
-    g_free(rom->path);
-    g_free(rom->name);
-    g_free(rom);
+    g_free(data);
     return -1;
 }
 
diff --git a/hw/loader.h b/hw/loader.h
index fc6bdff..9efe64a 100644
--- a/hw/loader.h
+++ b/hw/loader.h
@@ -21,6 +21,9 @@  void pstrcpy_targphys(const char *name,
                       const char *source);
 
 
+int rom_add_file_buf(const char *file, const void *data, size_t size,
+                     const char *fw_dir,
+                     target_phys_addr_t addr, int32_t bootindex);
 int rom_add_file(const char *file, const char *fw_dir,
                  target_phys_addr_t addr, int32_t bootindex);
 int rom_add_blob(const char *name, const void *blob, size_t len,
@@ -31,6 +34,8 @@  int rom_copy(uint8_t *dest, target_phys_addr_t addr, size_t size);
 void *rom_ptr(target_phys_addr_t addr);
 void do_info_roms(Monitor *mon);
 
+#define rom_add_file_buf_fixed(_f, _d, _s, _a, _i)          \
+    rom_add_file_buf(_f, _d, _s, NULL, _a, _i)
 #define rom_add_file_fixed(_f, _a, _i)          \
     rom_add_file(_f, NULL, _a, _i)
 #define rom_add_blob_fixed(_f, _b, _l, _a)      \