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
>From 9801526ff68166629ed0620da30304fc2dac300e Mon Sep 17 00:00:00 2001
From: Patrick Horgan <phorgan1@xxxxxxxxx>
Date: Thu, 11 Nov 2010 05:57:21 -0800
Subject: [PATCH 2/2] abstract common part of two routines to common png_open and have a short
read of header give better diagnostic
---
operations/external/png-load.c | 79 +++++++++++++++++++++-------------------
1 files changed, 41 insertions(+), 38 deletions(-)
diff --git a/operations/external/png-load.c b/operations/external/png-load.c
index 44128d8..b0a527f 100644
--- a/operations/external/png-load.c
+++ b/operations/external/png-load.c
@@ -34,6 +34,43 @@ gegl_chant_file_path (path, _("File"), "", _("Path of file to load."))
#include "gegl-chant.h"
#include <png.h>
+static FILE * open_png(const gchar *path)
+{
+ FILE *infile;
+ const size_t hdr_size=8;
+ size_t hdr_read_size;
+ unsigned char header[hdr_size];
+
+ if (!strcmp (path, "-"))
+ {
+ infile = stdin;
+ }
+ else
+ {
+ infile = fopen (path, "rb");
+ }
+ if (!infile)
+ {
+ return infile;
+ }
+
+ if((hdr_read_size=fread(header, 1, hdr_size, infile))!=hdr_size)
+ {
+ fclose(infile);
+ g_warning ("%s is too short for a png file, only %d bytes.",
+ path, hdr_read_size);
+ return NULL;
+ }
+
+ if (png_sig_cmp (header, 0, hdr_size))
+ {
+ fclose (infile);
+ g_warning ("%s is not a png file", path);
+ return NULL;
+ }
+ return infile;
+}
+
static gint
gegl_buffer_import_png (GeglBuffer *gegl_buffer,
const gchar *path,
@@ -53,7 +90,6 @@ gegl_buffer_import_png (GeglBuffer *gegl_buffer,
FILE *infile;
png_structp load_png_ptr;
png_infop load_info_ptr;
- unsigned char header[8];
guchar *pixels;
/*png_bytep *rows;*/
@@ -61,30 +97,10 @@ gegl_buffer_import_png (GeglBuffer *gegl_buffer,
unsigned int i;
png_bytep *row_p = NULL;
- if (!strcmp (path, "-"))
- {
- infile = stdin;
- }
- else
- {
- infile = fopen (path, "rb");
- }
- if (!infile)
- {
- return -1;
- }
-
- if(fread (header, 1, 8, infile)!=8)
- {
- fclose (infile);
- g_warning ("%s is not a valid png file", path);
- return -1;
- }
+ infile = open_png (path);
- if (png_sig_cmp (header, 0, 8))
+ if (!infile)
{
- fclose (infile);
- g_warning ("%s is not a png file", path);
return -1;
}
@@ -245,26 +261,13 @@ static gint query_png (const gchar *path,
FILE *infile;
png_structp load_png_ptr;
png_infop load_info_ptr;
- unsigned char header[8];
png_bytep *row_p = NULL;
- infile = fopen (path, "rb");
- if (!infile)
- {
- return -1;
- }
+ infile = open_png (path);
- if(fread (header, 1, 8, infile)!=8)
- {
- fclose (infile);
- g_warning ("%s is not a valid png file", path);
- return -1;
- }
- if (png_sig_cmp (header, 0, 8))
+ if (!infile)
{
- fclose (infile);
- g_warning ("%s is not a png file", path);
return -1;
}
--
1.7.1
_______________________________________________
Gegl-developer mailing list
Gegl-developer@xxxxxxxxxxxxxxxxxxxxxx
https://lists.XCF.Berkeley.EDU/mailman/listinfo/gegl-developer
