Just feeling my way in here, and it seemed an easy fix to get rid of a warning that uncovered an error. fread return value was being ignored when checking header of a png file, and if the file was truncated it would have caused other errors. Better to catch the error and abort then instead of trying to parse the short header to decide if it's valid.

Patrick

On 11/09/2010 10:32 PM, Martin Nordholts wrote:

Thanks! A few comments:

On 11/10/2010 02:02 AM, Patrick Horgan wrote:

+ if(fread (header, 1, 8, infile)!=8)

No magic numbers please, use a define for at least 8

Ok, I use a const size_t now.

+ g_warning ("%s is not a valid png file", path);

You should give a more detailed error message than just "is not valid"

Ok, now I say that it is too short to be a png file.

+ if(fread (header, 1, 8, infile)!=8) + {
+ fclose (infile);
+ g_warning ("%s is not a valid png file", path); + return -1;
+ }

Instead of duplicating this code from above, it would be better to put it in a helper function

/ Martin

Ok, I abstracted the beginning code from the two routines into a open_png(const gchar path) routine that returns the NULL for failure just as fopen() does, but on successful return has advanced past the header and checked it.

It was the most that I could abstract without out args or returning a struct, because each of the routines begins to allocate local data structures with libpng calls right after this. Seemed to maximize elegance.

I just copied the indentation scheme.

patch is against origin

Patrick

On 11/16/2010 11:22 AM, Martin Nordholts wrote:

Hi

The patch doesn't apply cleanly for me (sorry for messy formating):

Sorry about that. I think I may know why, I was two checkins away from trunk and the patch only reflected one. This one should apply, but if not please let me know.

Patrick

On 11/24/2010 12:58 PM, Martin Nordholts wrote:

On 11/23/2010 05:00 AM, Patrick Horgan wrote:

On 11/16/2010 11:22 AM, Martin Nordholts wrote:

Hi

The patch doesn't apply cleanly for me (sorry for messy formating):

Sorry about that. I think I may know why, I was two checkins away from trunk and the patch only reflected one. This one should apply, but if not please let me know.

Patrick

Could you add this to bugzilla please? I don't think I'll have time to look into the patch in a few weeks in worst case.

I know it's not a big patch, but I need to collect some motivation too.

Ok:) I've gotten a patch into GIMP before, but this would be my first in gegl. https://bugzilla.gnome.org/show_bug.cgi?id=635747

Patrick