| Message ID | 20181130151712.2312-6-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Remove deprecated load_image() function | expand |
On 11/30/18 9:17 AM, Peter Maydell wrote: > The load_image() function is deprecated, as it does not let the > caller specify how large the buffer to read the file into is. > Use the glib g_file_get_contents() function instead, which does > the whole "allocate memory for the file and read it in" operation. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/i386/pc.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > +++ b/hw/i386/pc.c > @@ -839,10 +839,9 @@ static void load_linux(PCMachineState *pcms, > { > uint16_t protocol; > int setup_size, kernel_size, cmdline_size; > - int64_t initrd_size = 0; > int dtb_size, setup_data_offset; > uint32_t initrd_max; > - uint8_t header[8192], *setup, *kernel, *initrd_data; > + uint8_t header[8192], *setup, *kernel; Unrelated - but 'header' at 8k is larger than I like for an auto variable. Some OSs put guard pages at only 4k granularity, so this much stack allocation can miss stack overflow. > - initrd_size = get_image_size(initrd_filename); > - if (initrd_size < 0) { > + if (!g_file_get_contents(initrd_filename, &initrd_data, > + &initrd_size, &gerr)) { > fprintf(stderr, "qemu: error reading initrd %s: %s\n", > - initrd_filename, strerror(errno)); > + initrd_filename, gerr->message); > exit(1); > - } else if (initrd_size >= initrd_max) { > + } > + if (initrd_size >= initrd_max) { > fprintf(stderr, "qemu: initrd is too large, cannot support." > - "(max: %"PRIu32", need %"PRId64")\n", initrd_max, initrd_size); > + "(max: %"PRIu32", need %"PRId64")\n", > + initrd_max, initrd_size); > exit(1); You're exiting anyway, so it doesn't matter, but free'ing initrd_data might satisfy a lint-checker. Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Fri, 30 Nov 2018 at 15:31, Peter Maydell <peter.maydell@linaro.org> wrote: > > The load_image() function is deprecated, as it does not let the > caller specify how large the buffer to read the file into is. > Use the glib g_file_get_contents() function instead, which does > the whole "allocate memory for the file and read it in" operation. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > - initrd_size = get_image_size(initrd_filename); > - if (initrd_size < 0) { > + if (!g_file_get_contents(initrd_filename, &initrd_data, > + &initrd_size, &gerr)) { > fprintf(stderr, "qemu: error reading initrd %s: %s\n", > - initrd_filename, strerror(errno)); > + initrd_filename, gerr->message); > exit(1); > - } else if (initrd_size >= initrd_max) { > + } > + if (initrd_size >= initrd_max) { > fprintf(stderr, "qemu: initrd is too large, cannot support." > - "(max: %"PRIu32", need %"PRId64")\n", initrd_max, initrd_size); > + "(max: %"PRIu32", need %"PRId64")\n", > + initrd_max, initrd_size); patchew reports that this introduces a format string error on 32-bit hosts: /tmp/qemu-test/src/hw/i386/pc.c: In function 'load_linux': /tmp/qemu-test/src/hw/i386/pc.c:983:29: error: format '%lld' expects argument of type 'long long int', but argument 4 has type 'gsize {aka unsigned int}' [-Werror=format=] fprintf(stderr, "qemu: initrd is too large, cannot support." ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ The underlying problem here is glib's weird insistance on using its own types, which then interact badly with everything else that's using standard POSIX types :-( thanks -- PMM
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index f095725dbab..067d23a992a 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -839,10 +839,9 @@ static void load_linux(PCMachineState *pcms, { uint16_t protocol; int setup_size, kernel_size, cmdline_size; - int64_t initrd_size = 0; int dtb_size, setup_data_offset; uint32_t initrd_max; - uint8_t header[8192], *setup, *kernel, *initrd_data; + uint8_t header[8192], *setup, *kernel; hwaddr real_addr, prot_addr, cmdline_addr, initrd_addr = 0; FILE *f; char *vmode; @@ -965,27 +964,30 @@ static void load_linux(PCMachineState *pcms, /* load initrd */ if (initrd_filename) { + gsize initrd_size; + gchar *initrd_data; + GError *gerr = NULL; + if (protocol < 0x200) { fprintf(stderr, "qemu: linux kernel too old to load a ram disk\n"); exit(1); } - initrd_size = get_image_size(initrd_filename); - if (initrd_size < 0) { + if (!g_file_get_contents(initrd_filename, &initrd_data, + &initrd_size, &gerr)) { fprintf(stderr, "qemu: error reading initrd %s: %s\n", - initrd_filename, strerror(errno)); + initrd_filename, gerr->message); exit(1); - } else if (initrd_size >= initrd_max) { + } + if (initrd_size >= initrd_max) { fprintf(stderr, "qemu: initrd is too large, cannot support." - "(max: %"PRIu32", need %"PRId64")\n", initrd_max, initrd_size); + "(max: %"PRIu32", need %"PRId64")\n", + initrd_max, initrd_size); exit(1); } initrd_addr = (initrd_max-initrd_size) & ~4095; - initrd_data = g_malloc(initrd_size); - load_image(initrd_filename, initrd_data); - fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_ADDR, initrd_addr); fw_cfg_add_i32(fw_cfg, FW_CFG_INITRD_SIZE, initrd_size); fw_cfg_add_bytes(fw_cfg, FW_CFG_INITRD_DATA, initrd_data, initrd_size);
The load_image() function is deprecated, as it does not let the caller specify how large the buffer to read the file into is. Use the glib g_file_get_contents() function instead, which does the whole "allocate memory for the file and read it in" operation. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/i386/pc.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) -- 2.19.1