Re: [PATCH v2 2/2] test: Fix issues reported by cppcheck static analysis tool

On Thu, Aug 21, 2014 at 09:18:04PM +0200, Bertram Felgenhauer wrote:
> Ravi Nanjundappa wrote:
> > @@ -71,11 +71,15 @@ read_file (const cairo_test_context_t *ctx,
> >      *len = ftell(fp);
> >      fseek (fp, 0, SEEK_SET);
> >      *data = malloc (*len);
> > -    if (*data == NULL)
> > +    if (*data == NULL) {
> > +	fclose(fp);
> >  	return CAIRO_TEST_NO_MEMORY;
> > +    }
> >  
> > -    if (fread(*data, *len, 1, fp) != 1)
> > +    if (fread(*data, *len, 1, fp) != 1) {
> > +	fclose(fp);
> >  	return CAIRO_TEST_FAILURE;
> > +    }
> 
> shouldn't there be a 'free(data)' here, too?

Actually, the caller frees the returned data if the test status is
failed.  However, I agree it'd be cleaner if the function cleaned up
after itself in this case.

Also, all the callers are printing out the same error message, "Could
not read input jpeg file", when actually the function call has four
distinct error conditions.

I'm thinking that read_file() should be made a bit more self-contained
by having it take ownership of printing the appropriate error message
for each error condition, as well as closing files and freeing memory.

Then the calling code can be simplified down to just:

    test_status = read_file (...);
    if (test_status != CAIRO_TEST_SUCCESS)
        return test_status;

Bryce
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo