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

[Stefan Hajnoczi]

On Tue, Mar 15, 2016 at 06:26:05PM +0530, rutuja shah wrote:
> 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 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

signature.asc
Description: PGP signature