Re: [Libguestfs] [PATCH 3/5] threads: Use thread-local storage for errors.

On Thu, Jun 11, 2015 at 11:27:23AM +0100, Daniel P. Berrange wrote: > On Sat, Jun 06, 2015 at 02:20:39PM +0100, Richard W.M. Jones wrote: > > We permit the following constructs in libguestfs code: > > > > if (guestfs_some_call (g) == -1) { > > fprintf (stderr, "failed: error is %s\n", guestfs_last_error (g)); > > } > > > > and: > > > > guestfs_push_error_handler (g, NULL, NULL); > > guestfs_some_call (g); > > guestfs_pop_error_handler (g); > > > > Neither of these would be safe if we allowed the handle to be used > > from threads concurrently, since the error string or error handler > > could be changed by another thread. > > > > Solve this in approximately the same way that libvirt does: by making > > the error, current error handler, and stack of error handlers use > > thread-local storage (TLS). > > > > The implementation is not entirely straightforward, mainly because > > POSIX doesn't give us useful destructor behaviour, so effectively we > > end up creating our own destructor using a linked list. > > > > Note that you have to set the error handler in each thread separately, > > which is an API change (eg: if you set the error handler in one > > thread, then pass the handle 'g' to another thread, the error handler > > in the second thread appears to have reset itself back to the default > > error handler). I haven't yet worked out a better way to solve this. > > Do you have a feeling whether the error handler function push/pop > is a commonly used feature or not ? All the time, in our code: $ git grep -E 'guestfs_(push|pop)_error_handler' -- *.c | wc -l 83 Of course we could change those, but it is still an API guarantee.

> In libvirt we originally had > > virGetLastError (global error) > virConnGetLastError (per connection error) > virSetErrorFunc (global error handler func) > > When we introduced thread support we didn't try to make the > virConnGetLastError/SetErrorFun work. We just updated the docs > to tell applications to /never/ use them in threaded applications, > and always use the virGetLastError which was thread-local > backed. For back compatibility we do copy the error from > the global thread local into the per-connection object, so we > didn't break single-threaded apps using that old function. > > IOW I think you could avoid alot of the complexity here if you > just marked the existing error handler functions as deprecated > in the context of multi-threaded usage and just introduced a > single global "get error" function that was backed by a static > allocated thread local.