[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Added NULL check for qemu_find_file()
From: |
rutuja shah |
Subject: |
Re: [Qemu-devel] [PATCH] Added NULL check for qemu_find_file() |
Date: |
Mon, 14 Mar 2016 18:29:06 +0530 |
Regards
Rutuja Shah
On Mon, Mar 14, 2016 at 5:14 PM, Stefan Hajnoczi <address@hidden> wrote:
> On Sun, Mar 13, 2016 at 02:06:34AM +0530, address@hidden wrote:
>> From: Rutuja Shah <address@hidden>
>>
>> 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 <address@hidden>
>>
>> Regards
>> Rutuja Shah
>>
>>
Re: [Qemu-devel] [PATCH] Added NULL check for qemu_find_file(), Eric Blake, 2016/03/14