| Message ID | 1457814994-10698-1-git-send-email-rutu.shah.26@gmail.com |
|---|---|
| State | New |
| Headers | show |
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 > >
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 >> >>
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.
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
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
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
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",