Re: [pdf-devel] Error module implementation

[Aleksander Morgado]

Hi gerel,

Just a couple of comments,


> int pdf_error_one_per_line = 1;

Is this variable going to be modified at runtime?

If so, it is not thread-safe so maybe a specific context should be 
defined for the error module, so that it can be passed to the error 
function when calling it (or just that variable as an extra argument to 
pdf_error).

Or, maybe this variable is set in a `pdf_error_init' function only once 
at program startup, and not modified afterwards. Then it would be enough 
to declare it as static in the file, to avoid exporting the symbol.

If this variable is not going to be never modified at runtime it should 
be explicitly defined as constant, to avoid confusion, or directly 
remove the feature and always print the EOL control char.

This feature reminds me the `error_at_line' function, which is based on 
a global variable `error_one_per_line'. But the difference is that the 
purpose of error_at_line is to block all prints (except for the first 
one) related to errors in a given file and a given line number within 
the file, used mainly to locate errors in input files (like errors 
reported by a compiler). Our pdf_error_one_per_line is to avoid printing 
messages in different lines and merge all of them in the same line, 
which I don't really think is a good way to manage the error prints. I 
just checked previous mails and saw that you are also not sure if the 
feature is useful, so I would remove it to avoid reentrancy problems.


>   fprintf (fd, "GNU PDF");

You can use the `program_name' variable from pdf-global.c:
./src/pdf-global.c:char *program_name = "libgnupdf";


>   if (format != NULL)
>     {
>       fprintf (fd, ": ");
>       va_start (args, format);
>       vfprintf (fd, format, args);
>     }

The macro `va_end' should be called after having called `va_start', even 
if GCC does nothing with it (portability issues).


> Comments are welcome.
>
> BTW we said we won't abort the program by any way, although PDF_ENOMEM
> comes to my mind :-/

I was thinking the same, but Cirilo is right, the library should never 
abort the program. Every call to a function which is allocating memory 
must return either a pdf_status_t which should be checked against 
PDF_OK; or a pointer to the newly allocated memory, so that it can be 
checked against NULL.


BTW, I read an old mail from Behdad 
(http://lists.gnu.org/archive/html/pdf-devel/2007-12/msg00059.html) 
explaining the way errors are propagated in Cairo, using an error state 
variable within object types. This could be useful (in addition to the 
current error reporting scheme) if, for example, functions deal with 
more than one object at the same time (an error could invalidate one of 
the objects, or both of them, or none). If just an error enum is 
returned, it is not possible to know which returned objects are valid 
(unless you specify different return values covering those cases...). 
The solution would be storing an error state in each object type (which 
could even be of type pdf_status_t). Did someone elaborate more on this?


Regards!

-Aleksander