Regards Rutuja Shah
On Mon, Mar 14, 2016 at 5:14 PM, Stefan Hajnoczi <stefa...@redhat.com> wrote: > On Sun, Mar 13, 2016 at 02:06:34AM +0530, rutu.shah...@gmail.com wrote: >> From: Rutuja Shah <rutu.shah...@gmail.com> >> >> This patch adds NULL check for return value from qemu_find_file(), where it >> is missing. It avoids unnecessary function calls with NULL parameter which >> in turn return -1. Especially, incase of load_uimage(), two functions are >> called which return -1 on passing NULL filename. > > What is the benefit of adding the NULL checks? The only difference I > see is that the error message from load_elf() is silenced. Yes, in case of load_elf, error message is silenced. The immediate call to load_uimage() is conditional to error from load_elf(). load_uimage() in turn calls load_uboot_image() which then calls open() with NULL filename and returns with -1. In my opinion, this can be avoided. > > Avoiding unnecessary function calls isn't worthwhile if all callers now > duplicate the if (filename) ... else size = -1 code. As far as I have seen the code, all callers have this check in place already, except the patched files. > > Please wrap the commit description (the body of the commit message) to > 72 characters or at most 80 characters. > > Please include your Signed-off-by line here: > http://qemu-project.org/Contribute/SubmitAPatch#Patch_emails_must_include_a_Signed-off-by:_line I will take care of this henceforth. > >> --- >> hw/ppc/e500.c | 17 +++++++++++++---- >> hw/sparc/leon3.c | 6 +++++- >> 2 files changed, 18 insertions(+), 5 deletions(-) >> >> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c >> index 09154fa..006adf1 100644 >> --- a/hw/ppc/e500.c >> +++ b/hw/ppc/e500.c >> @@ -1016,15 +1016,24 @@ void ppce500_init(MachineState *machine, >> PPCE500Params *params) >> } >> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); >> >> - bios_size = load_elf(filename, NULL, NULL, &bios_entry, &loadaddr, NULL, >> - 1, PPC_ELF_MACHINE, 0, 0); >> + if (filename) { >> + bios_size = load_elf(filename, NULL, NULL, &bios_entry, &loadaddr, >> NULL, >> + 1, PPC_ELF_MACHINE, 0, 0); >> + } else { >> + bios_size = -1; >> + } >> + >> if (bios_size < 0) { >> /* >> * Hrm. No ELF image? Try a uImage, maybe someone is giving us an >> * ePAPR compliant kernel >> */ >> - kernel_size = load_uimage(filename, &bios_entry, &loadaddr, NULL, >> - NULL, NULL); >> + if (filename) { >> + kernel_size = load_uimage(filename, &bios_entry, &loadaddr, >> NULL, >> + NULL, NULL); >> + } else { >> + kernel_size = -1; >> + } >> if (kernel_size < 0) { >> fprintf(stderr, "qemu: could not load firmware '%s'\n", >> filename); >> exit(1); >> diff --git a/hw/sparc/leon3.c b/hw/sparc/leon3.c >> index c579f5b..b4e9334 100644 >> --- a/hw/sparc/leon3.c >> +++ b/hw/sparc/leon3.c >> @@ -168,7 +168,11 @@ static void leon3_generic_hw_init(MachineState *machine) >> } >> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name); >> >> - bios_size = get_image_size(filename); >> + if (filename) { >> + bios_size = get_image_size(filename); >> + } else { >> + bios_size = -1; >> + } >> >> if (bios_size > prom_size) { >> fprintf(stderr, "qemu: could not load prom '%s': file too big\n", >> >> 1.9.1 >> >> Signed-off-by: Rutuja Shah <rutu.shah...@gmail.com> >> >> Regards >> Rutuja Shah >> >>
