On 2011-05-28 Guillem Jover wrote:
> Adding the _r (or _e for error or whatever) counterparts seems like
> the most portable solution (with the C/POSIX restictions you
> mention), at the cost of API bloat (as we discussed on IRC), and
> probably more code churn? The normal functions could then be made to
> be tiny shims just passing NULL as the error argument.
Yes, it needs more changes to the code than using a thread-local
variable.
> Passing just a pointer to a buffer might be problematic due to the
> size being unknown to the caller, so ideally it should be a pointer
> to pointer to buffer, and the function allocating the message or
> assigning from a static string table, or a pointer to an int (or
> some other integral type) to just assign an extended lzma_ret code.
Defining big enough maximum size for the message should be enough, e.g.
512 bytes. A caller can allocate such a buffer on stack. I don't want to
dynamically allocate memory because that can fail and that needs to be
freed too.
Using a custom string allows more specific messages, e.g. including what
value was seen in the file vs. what was expected. It also is safer when
liblzma is loaded with dlopen() because there is no static string that
could disappear.
> I'd add the TLS storage class specifier as an option, as it seems to
> be suported by quite a few compilers, it obviously depends on which
> ones you want to support currently.
So far my goal has been to support anything that supports C99 enough,
sometimes using something else than Autotools-based build system. In
practice this has excluded GCC 2 and Microsoft's compilers.
It is very annoying if liblzma provides a function that is available
only on some platforms. So if TLS is used, it needs to be supported
almost everywhere to be acceptable for XZ Utils.
> It seems (from [0] and [1]) at least these compilers support
> something like __thread or __declspec(thread):
>
> * Borland C++ Builder
> * Digital Mars C/C++
> * GNU C/C++
> * HP Tru64 UNIX C/C++
> * IBM XL C/C++
> * Intel C/C++
> * Sun Studio C/C++
> * Visual C++
That covers quite a lot, but I'm not confident that it is enough. One
must keep in mind that compiler support isn't enough: it needs also
operating system support. So having GCC >= 3.3 available doesn't imply
that TLS is supported.
> > - If the library is never unloaded with dlclose(), then there
> > is no problem with pthread_key_create(). This isn't an
> > acceptable limitation for liblzma.
>
> Well, pthread_key_create() allows to provide a destructor function,
> so as long as that function is not part of liblzma (free(3)), then
> it can do proper cleanup once the thread terminates, ragardless of
> liblzma having been unloaded.
The pthread_key_t is in the memory of liblzma and thus may be gone when
liblzma is unloaded. Pointer to the destructor function might be stored
in the key. The key would also be leaked, which can be a real-world
problem if the library is loaded and unloaded multiple times, because
the system might support only a limited number of keys per process:
https://svn.boost.org/trac/boost/ticket/4639
--
Lasse Collin | IRC: Larhzu @ IRCnet & Freenode
