On Tue, May 31, 2022 at 12:59 AM Philippe Mathieu-Daudé via <qemu-devel@nongnu.org> wrote: > > Hi Peter, > > On 28/4/22 01:07, Peter Collingbourne wrote: > > Currently the loader uses int as the return type for various APIs > > that deal with file sizes, which leads to an error if the file > > size is >=2GB, as it ends up being interpreted as a negative error > > code. Furthermore, we do not tolerate short reads, which are possible > > at least on Linux when attempting to read such large files in one > > syscall. > > > > Fix the first problem by switching to 64-bit types for file sizes, > > and fix the second by introducing a loop around the read syscall. > > Hmm maybe worth rebasing on this patch from Jamie which also > fixes the format on 32-bit hosts: > https://lore.kernel.org/qemu-devel/20211111141141.3295094-2-ja...@nuviainc.com/ > > (Personally I prefer to read ssize_t while reviewing instead > of int64_t).
I agree with ssize_t as well, I have applied the patch from Jamie which had fallen through the cracks. If you can rebase this on top of the RISC-V queue and re-send it I'll apply the other changes Alistair > > While here, please have a look at this other patch from Jamie: > https://lore.kernel.org/qemu-devel/20211111141141.3295094-3-ja...@nuviainc.com/ > > Also, Cc'ing the maintainer: > > $ ./scripts/get_maintainer.pl -f hw/core/generic-loader.c > Alistair Francis <alist...@alistair23.me> (maintainer:Generic Loader) > > > Signed-off-by: Peter Collingbourne <p...@google.com> > > --- > > hw/core/generic-loader.c | 2 +- > > hw/core/loader.c | 44 ++++++++++++++++++++++++---------------- > > include/hw/loader.h | 13 ++++++------ > > 3 files changed, 34 insertions(+), 25 deletions(-) > > > > diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c > > index c666545aa0..0891fa73c3 100644 > > --- a/hw/core/generic-loader.c > > +++ b/hw/core/generic-loader.c > > @@ -67,7 +67,7 @@ static void generic_loader_realize(DeviceState *dev, > > Error **errp) > > GenericLoaderState *s = GENERIC_LOADER(dev); > > hwaddr entry; > > int big_endian; > > - int size = 0; > > + int64_t size = 0; > > > > s->set_pc = false; > > > > diff --git a/hw/core/loader.c b/hw/core/loader.c > > index ca2f2431fb..d07c79c400 100644 > > --- a/hw/core/loader.c > > +++ b/hw/core/loader.c > > @@ -115,17 +115,17 @@ ssize_t read_targphys(const char *name, > > return did; > > } > > > > -int load_image_targphys(const char *filename, > > - hwaddr addr, uint64_t max_sz) > > +int64_t load_image_targphys(const char *filename, > > + hwaddr addr, uint64_t max_sz) > > { > > return load_image_targphys_as(filename, addr, max_sz, NULL); > > } > > > > /* return the size or -1 if error */ > > -int load_image_targphys_as(const char *filename, > > - hwaddr addr, uint64_t max_sz, AddressSpace *as) > > +int64_t load_image_targphys_as(const char *filename, > > + hwaddr addr, uint64_t max_sz, AddressSpace > > *as) > > { > > - int size; > > + int64_t size; > > > > size = get_image_size(filename); > > if (size < 0 || size > max_sz) { > > @@ -139,9 +139,9 @@ int load_image_targphys_as(const char *filename, > > return size; > > } > > > > -int load_image_mr(const char *filename, MemoryRegion *mr) > > +int64_t load_image_mr(const char *filename, MemoryRegion *mr) > > { > > - int size; > > + int64_t size; > > > > if (!memory_access_is_direct(mr, false)) { > > /* Can only load an image into RAM or ROM */ > > @@ -963,7 +963,8 @@ int rom_add_file(const char *file, const char *fw_dir, > > { > > MachineClass *mc = MACHINE_GET_CLASS(qdev_get_machine()); > > Rom *rom; > > - int rc, fd = -1; > > + int fd = -1; > > + size_t bytes_read = 0; > > char devpath[100]; > > > > if (as && mr) { > > @@ -1003,11 +1004,17 @@ int rom_add_file(const char *file, const char > > *fw_dir, > > 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=%d (expected > > %zd)\n", > > - rom->name, rc, rom->datasize); > > - goto err; > > + while (bytes_read < rom->datasize) { > > + ssize_t rc = > > + read(fd, rom->data + bytes_read, rom->datasize - bytes_read); > > + if (rc <= 0) { > > + fprintf(stderr, > > + "rom: file %-20s: read error: rc=%zd at position %zd " > > + "(expected size %zd)\n", > > + rom->name, rc, bytes_read, rom->datasize); > > + goto err; > > + } > > + bytes_read += rc; > > } > > close(fd); > > rom_insert(rom); > > @@ -1671,7 +1678,7 @@ typedef struct { > > HexLine line; > > uint8_t *bin_buf; > > hwaddr *start_addr; > > - int total_size; > > + int64_t total_size; > > uint32_t next_address_to_write; > > uint32_t current_address; > > uint32_t current_rom_index; > > @@ -1767,8 +1774,8 @@ static int handle_record_type(HexParser *parser) > > } > > > > /* return size or -1 if error */ > > -static int parse_hex_blob(const char *filename, hwaddr *addr, uint8_t > > *hex_blob, > > - size_t hex_blob_size, AddressSpace *as) > > +static int64_t parse_hex_blob(const char *filename, hwaddr *addr, uint8_t > > *hex_blob, > > + size_t hex_blob_size, AddressSpace *as) > > { > > bool in_process = false; /* avoid re-enter and > > * check whether record begin with ':' */ > > @@ -1832,11 +1839,12 @@ out: > > } > > > > /* return size or -1 if error */ > > -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace > > *as) > > +int64_t load_targphys_hex_as(const char *filename, hwaddr *entry, > > + AddressSpace *as) > > { > > gsize hex_blob_size; > > gchar *hex_blob; > > - int total_size = 0; > > + int64_t total_size = 0; > > > > if (!g_file_get_contents(filename, &hex_blob, &hex_blob_size, NULL)) { > > return -1; > > diff --git a/include/hw/loader.h b/include/hw/loader.h > > index 5572108ba5..7b09705940 100644 > > --- a/include/hw/loader.h > > +++ b/include/hw/loader.h > > @@ -40,8 +40,8 @@ ssize_t load_image_size(const char *filename, void *addr, > > size_t size); > > * > > * Returns the size of the loaded image on success, -1 otherwise. > > */ > > -int load_image_targphys_as(const char *filename, > > - hwaddr addr, uint64_t max_sz, AddressSpace *as); > > +int64_t load_image_targphys_as(const char *filename, > > + hwaddr addr, uint64_t max_sz, AddressSpace > > *as); > > > > /**load_targphys_hex_as: > > * @filename: Path to the .hex file > > @@ -53,14 +53,15 @@ int load_image_targphys_as(const char *filename, > > * > > * Returns the size of the loaded .hex file on success, -1 otherwise. > > */ > > -int load_targphys_hex_as(const char *filename, hwaddr *entry, AddressSpace > > *as); > > +int64_t load_targphys_hex_as(const char *filename, hwaddr *entry, > > + AddressSpace *as); > > > > /** load_image_targphys: > > * Same as load_image_targphys_as(), but doesn't allow the caller to > > specify > > * an AddressSpace. > > */ > > -int load_image_targphys(const char *filename, hwaddr, > > - uint64_t max_sz); > > +int64_t load_image_targphys(const char *filename, hwaddr, > > + uint64_t max_sz); > > > > /** > > * load_image_mr: load an image into a memory region > > @@ -73,7 +74,7 @@ int load_image_targphys(const char *filename, hwaddr, > > * If the file is larger than the memory region's size the call will fail. > > * Returns -1 on failure, or the size of the file. > > */ > > -int load_image_mr(const char *filename, MemoryRegion *mr); > > +int64_t load_image_mr(const char *filename, MemoryRegion *mr); > > > > /* This is the limit on the maximum uncompressed image size that > > * load_image_gzipped_buffer() and load_image_gzipped() will read. It > > prevents > >
