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

[rutuja shah]

Regards
Rutuja Shah


On Tue, Mar 15, 2016 at 6:01 PM, Stefan Hajnoczi <address@hidden> 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