Regards Rutuja Shah
On Tue, Mar 15, 2016 at 6:01 PM, Stefan Hajnoczi <stefa...@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
