2010.03.02 16:05 "Re: [Tiff] Patch for tif_unix.c", by Eric Doenges

Can I simply assume that SSIZE_MAX will be defined in <limits.h>, and that <limits.h> will be available for all platforms that matter to libtiff ?

If Visual Studio has SSIZE_MAX in <limits.h>, then it probably will be. :)

As for the patch, I alluded earlier (poorly?) that ssize_t is not available on all platforms. Specifically, it's not available on Windows. Despite it's name, tif_unix.c has been known to be used on Windows by people. Looking at the source in some detail now, tmsize_t looks to be the cross-platform signed equivalent to size_t in libtiff. Note also that _tiff{Read,Write}Proc() already tries to carefully ensure that "(size_t)size == (tmsize_t)size". Given this, I suspect that your extra checks with SSIZE_MAX are probably unnecessary?

I've probably missed something but I think the original patch was sufficient (EINTR issues notwithstanding) as long as "bytes" was declared as a tmsize_t?

If tmsize_t can be guaranteed to be identical to ssize_t, then no check is necessary (as long as we can assume there are no systems where SSIZE_MAX is smaller than what would physically fit into a ssize_t). If not, then the "(size_t)size == (tmsize_t)size" check won't necessarily help us as the read()/write() functions have undefined behaviour when attempting to read/write more than SSIZE_MAX bytes.

However, if tmsize_t is just a libtiff equivalent to ssize_t, then the "(size_t)size == (tmsize_t)size" isn't necessary either, since the condition should always be true (unless a negative size is requested, which doesn't make any sense for I/O operations). I had assumed the check was necessary because tmsize_t might be bigger than {s}size_t, and thus didn't think I could use tmsize_t instead of ssize_t. Perhaps whoever wrote the current form of the _tiff{Read,Write}Proc() functions could chime in as to what was intended?

What is the correct response to receiving EINTR (i.e. being interrupted before any data is read/written) ? Should the IO operation simply be retried in _tiffReadProc/_tiffWriteProc, or should the caller handle it ?

To me, this is a thorny issue. On Windows, signals don't exist so you can't exactly check for EINTR. Two ideas off the top of my head:

Continue punting. We've lasted this long without it. :) One rationale is to force users to use SA_RESTART with sigaction. Of course it doesn't help you if you're on one of those systems that don't support this. In which case, the caller needs to revert back to providing their own custom handlers again. Does your particular system support SA_RESTART?

The system in question doesn't have signals or support having system calls being interrupted, so it's not an issue for us. As far a I'm concerned, if the libtiff maintainers can live with libtiff not handling EINTR (as is currently the case), so can I. However, since changing the error handling code to

...
else if (0 == bytes)
{
        if (EINTR == errno)
                continue;
        else
                return (tmsize_t)-1;
}
...

would be trivial (and should handle the problem of what to do with EINTR regardless of wether the underlying system actually supports signals or not - systems without signals shouldn't return EINTR in the first place), I wanted to hear what the intended behaviour should be.

Wrap the read()/write() calls using the TEMP_FAILURE_RETRY() macro and configure it properly depending on the platform. (See: http://www.gnu.org/s/libc/manual/html_node/Interrupted-Primitives.html#Interrupted-Primitives)

With kind regards,
Eric Dönges
--
Dr. Eric Dönges doenges@mvtec.com

MVTec Software GmbH | Neherstr. 1 | 81675 München | Germany

www.mvtec.com | Tel: +49 89 457695-0 | Fax: +49 89 457695-55

Geschäftsführer: Dr. Wolfgang Ecks