Re: [Libguestfs] [libnbd PATCH v2] lib/errors.c: Fix assert fail in exit path in multi-threaded code

> @@ -87,16 +92,23 @@ free_errors_key (void *vp) > static struct last_error * > allocate_last_error_on_demand (void) > { > - struct last_error *last_error = pthread_getspecific (errors_key); > + struct last_error *last_error = NULL; > > - if (!last_error) { > - last_error = calloc (1, sizeof *last_error); > - if (last_error) { > - int err = pthread_setspecific (errors_key, last_error); > - if (err != 0) { > - /* This is not supposed to happen (XXX). */ > - fprintf (stderr, "%s: %s: %s\n", "libnbd", "pthread_setspecific", > - strerror (err)); > + if (key_use_count) { > + last_error = pthread_getspecific (errors_key); > + if (!last_error) { > + last_error = calloc (1, sizeof *last_error); > + if (last_error) { > + int err = pthread_setspecific (errors_key, last_error); > + key_use_count++; > + if (err != 0) { > + /* This is not supposed to happen (XXX). */ > + fprintf (stderr, "%s: %s: %s\n", "libnbd", "pthread_setspecific", > + strerror (err)); > + free (last_error); > + last_error = NULL; > + key_use_count--; > + } > } > } > } I suspect this may not be completely race condition free unless there's a lock. Otherwise key_use_count could be > 0 when we enter this code and then another thread could run the destructor at the same time.

However I don't want to add locking around these functions since (especially the one setting context) is highly contended and this could kill performance. I think the worst that might happen is we would get EINVAL from pthread_setspecific, print a message, but at least we won't crash because the assert has been removed. So soft ACK, but I'm still wondering if there's a better way to do this.

If simply never calling pthread_key_destroy a good idea? The worst is it leaks slots in glibc's thread-local storage array.