[cairo] [PATCH] Improve error message when using cairo_image_surface_create_from_png with an invalid file.

[Uli Schlachter]

Hi,

On 22.03.2016 23:19, Cyril Roelandt wrote:
[...]
> The following patch, or something along those lines, might help users
> get a better error message:
>
> diff --git a/src/cairo-png.c b/src/cairo-png.c
> index 068617d..e87c465 100644
> --- a/src/cairo-png.c
> +++ b/src/cairo-png.c
> @@ -134,9 +134,15 @@ png_simple_error_callback (png_structp png,
>   {
>       cairo_status_t *error = png_get_error_ptr (png);
>
> -    /* default to the most likely error */
> -    if (*error == CAIRO_STATUS_SUCCESS)
> -       *error = _cairo_error (CAIRO_STATUS_NO_MEMORY);
> +    if (*error == CAIRO_STATUS_SUCCESS) {
> +        /* Try do determine the error using the error message.
> +           If not possible, default to the most likely error. */
> +        if (!strcmp(error_msg, "Not a PNG file"))
> +            *error = CAIRO_STATUS_READ_ERROR;
> +        else
> +           *error = CAIRO_STATUS_NO_MEMORY;
> +        *error = _cairo_error(*error);
> +    }
>
>   #ifdef PNG_SETJMP_SUPPORTED
>       longjmp (png_jmpbuf (png), 1);

I don't like "grepping" through error messages like this (what if libpng changes 
the exact wording?) and wondered if there is no better API for this. For the 
first time in my life I looked at the libpng API (and the API docs) and I hope 
that I will never do that again...
So second try was to search for "Not a PNG file" in debian codesearch where I 
found the following:

http://sources.debian.net/src/vtk/5.10.1+dfsg-2/IO/vtkPNGReader.cxx/?hl=58#L58

This reads the first 8 bytes of the file (handles short reads correctly!) and 
uses png_sig_cmp() to check that these bytes are correct (and then it uses 
png_set_sig_bytes() in line 101 to compensate for these missing bytes).

Having seen this code, I like libpng's API even less, but apparently this would 
be the "official" way to handle this.

Perhabs checking the error message isn't so bad after all... I'm unsure.

 > Maybe we could even have a CAIRO_STATUS_NOT_A_PNG error status. What do
 > you think?

The PNG-API in cairo is part of the toy interfaces, which exist to simplify 
writing examples and tests. They aren't meant to offer all possibilities (use 
something like gdk-pixbuf for that).

Thus, I don't think we should have special error codes for the toy API (but I 
don't mind if someone thinks otherwise and adds them anyway).


Cheers,
Uli