[Qemu-devel] [PATCH] Added NULL check for qemu_find_file()

[Qemu-devel] [PATCH] Added NULL check for qemu_find_file()

[rutu.shah.26]

From: Rutuja Shah <rutu.shah.26@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.
---
 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.26@gmail.com>

Regards
Rutuja Shah

Re: [Qemu-devel] [PATCH] Added NULL check for qemu_find_file()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 3117 bytes --]

On Sun, Mar 13, 2016 at 02:06:34AM +0530, rutu.shah.26@gmail.com wrote:
> From: Rutuja Shah <rutu.shah.26@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.

Avoiding unnecessary function calls isn't worthwhile if all callers now
duplicate the if (filename) ... else size = -1 code.

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

> ---
>  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.26@gmail.com>
> 
> Regards
> Rutuja Shah
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] Added NULL check for qemu_find_file()

[rutuja shah]

Regards
Rutuja Shah


On Mon, Mar 14, 2016 at 5:14 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Sun, Mar 13, 2016 at 02:06:34AM +0530, rutu.shah.26@gmail.com wrote:
>> From: Rutuja Shah <rutu.shah.26@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.26@gmail.com>
>>
>> Regards
>> Rutuja Shah
>>
>>

Re: [Qemu-devel] [PATCH] Added NULL check for qemu_find_file()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1045 bytes --]

On 03/12/2016 01:36 PM, rutu.shah.26@gmail.com wrote:
> From: Rutuja Shah <rutu.shah.26@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 

Please wrap your commit messages at 70 or so columns (since 'git log'
will display your text with indentation, and many people still prefer
80-column terminal windows).

s/incase/in case/

> ---
>  hw/ppc/e500.c    | 17 +++++++++++++----
>  hw/sparc/leon3.c |  6 +++++-
>  2 files changed, 18 insertions(+), 5 deletions(-)
> 

> 
> Signed-off-by: Rutuja Shah <rutu.shah.26@gmail.com>

This S-o-b is in the wrong place; it needs to appear before the ---
separator to be included in the git log after a maintainer does 'git am'
on your patch.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] Added NULL check for qemu_find_file()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]

On Mon, Mar 14, 2016 at 06:29:06PM +0530, rutuja shah wrote:
> > 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.

Does this mean the main purpose of the patch is to make these
load_elf()/load_uimage() callers consistent with other callers?

The commit description should say "why" rather than "what" the patch is
doing.

In it's current state I'm not sure what the benefit of this change is.
I also wonder whether the load_elf()/load_uimage() function prototype
should be adjusted to avoid the boilerplate if statement in all callers.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH] Added NULL check for qemu_find_file()

[rutuja shah]

Regards
Rutuja Shah


On Tue, Mar 15, 2016 at 6:01 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Mon, Mar 14, 2016 at 06:29:06PM +0530, rutuja shah wrote:
>> > 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.
>
> Does this mean the main purpose of the patch is to make these
> load_elf()/load_uimage() callers consistent with other callers?
This is a task listed on http://qemu-project.org/BiteSizedTasks
(presently, I am unaware of the intention it is put up here). I
propose this change so that callers are consistent. Also it adds to
the performance, if such a case is encountered for reasons mentioned
earlier.
>
> The commit description should say "why" rather than "what" the patch is
> doing.
Ok.
>
> In it's current state I'm not sure what the benefit of this change is.
> I also wonder whether the load_elf()/load_uimage() function prototype
> should be adjusted to avoid the boilerplate if statement in all callers.
Changing the prototype would result in a cleaner code, for all the
functions which are using the file name received from
qemu_find_file().
>
> Stefan

Re: [Qemu-devel] [PATCH] Added NULL check for qemu_find_file()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1796 bytes --]

On Tue, Mar 15, 2016 at 06:26:05PM +0530, rutuja shah wrote:
> On Tue, Mar 15, 2016 at 6:01 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > On Mon, Mar 14, 2016 at 06:29:06PM +0530, rutuja shah wrote:
> >> > 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.
> >
> > Does this mean the main purpose of the patch is to make these
> > load_elf()/load_uimage() callers consistent with other callers?
> This is a task listed on http://qemu-project.org/BiteSizedTasks
> (presently, I am unaware of the intention it is put up here). I
> propose this change so that callers are consistent. Also it adds to
> the performance, if such a case is encountered for reasons mentioned
> earlier.

The task description on the wiki is terse.  I'm not sure the intention
was to make the change you have posted.  I suspect the issue is related
to unexpected behavior due to callers dereferencing a NULL filename
pointer or assuming size > 0.

Maybe one of the CCed maintainers remembers or has an opinion.  If there
is no additional feedback from anyone then I'd hold back on this patch
because it perturbs the code without a strong reason to do so.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]