At Wed, 30 Jan 2013 19:13:54 +0100, Jérôme Forissier wrote:
On 30 janv. 2013, at 17:03, Takashi Iwai wrote:
At Wed, 30 Jan 2013 16:22:15 +0100, Jerome Forissier wrote:
This is a set of two patches to fix thread safety issues I found when running a video application over ALSA. The application uses LibVLC to play several videos simultaneously. LibVLC calls snd_pcm_open() and snd_device_name_hint() from multiple threads, without locking--which is allowed according to http://www.alsa-project.org/main/index.php/SMP_Design.
The fix for snd_pcm_direct_parse_open_conf() looks good. I'm going to apply this now.
Good, thank you :)
snd_device_name_hint() should use snd_config_update_r() and pass the local config space to others instead of snd_config.
I tried following this path, but there is a problem: try_config() calls snd_lib_error_set_handler() so I fail to see how we can make this code reentrant. In fact, the random errors I've seen when running my multi-threaded test were probably just a side-effect of fiddling with the global error handler from multiple threads. Probably harmless, but still it makes snd_device_name_hint() not thread-safe, strictly speaking.
Yeah, I see the issue now, too.
The problem is that the error handler is really global, not only used in conf.c and co. Thus applying a lock locally doesn't suffice.
Any suggestion? Should I consider moving snd_lib_error to thread-local storage?
TLB would be an option, indeed. But blindly moving snd_lib_error to TLS breaks the current behavior, so we should avoid it.
How about a patch like below? It adds another error redirection. Unless local_error is set, the behavior is identical.
(Maybe we can name it better, yeah. I just wanted to avoid to put a word "thread" in the function because it should be usable in both thread- and non-thread-capable environment.)
thanks,
Takashi
--- diff --git a/include/error.h b/include/error.h index 6d27083..256cd5f 100644 --- a/include/error.h +++ b/include/error.h @@ -74,5 +74,11 @@ extern int snd_lib_error_set_handler(snd_lib_error_handler_t handler); } #endif
+typedef void (*snd_local_error_handler_t)(const char *file, int line, + const char *func, int err, + const char *fmt, va_list arg); + +snd_local_error_handler_t snd_lib_error_set_local(snd_local_error_handler_t func); + #endif /* __ALSA_ERROR_H */
diff --git a/src/control/namehint.c b/src/control/namehint.c index 19352be..defc036 100644 --- a/src/control/namehint.c +++ b/src/control/namehint.c @@ -80,7 +80,8 @@ static void zero_handler(const char *file ATTRIBUTE_UNUSED, int line ATTRIBUTE_UNUSED, const char *function ATTRIBUTE_UNUSED, int err ATTRIBUTE_UNUSED, - const char *fmt ATTRIBUTE_UNUSED, ...) + const char *fmt ATTRIBUTE_UNUSED, + va_list arg ATTRIBUTE_UNUSED) { }
@@ -212,7 +213,7 @@ static int try_config(struct hint_list *list, const char *base, const char *name) { - snd_lib_error_handler_t eh; + snd_local_error_handler_t eh; snd_config_t *res = NULL, *cfg, *cfg1, *n; snd_config_iterator_t i, next; char *buf, *buf1 = NULL, *buf2; @@ -239,10 +240,9 @@ static int try_config(struct hint_list *list, sprintf(buf, "%s:CARD=%s", name, snd_ctl_card_info_get_id(list->info)); else strcpy(buf, name); - eh = snd_lib_error; - snd_lib_error_set_handler(&zero_handler); + eh = snd_lib_error_set_local(&zero_handler); err = snd_config_search_definition(snd_config, base, buf, &res); - snd_lib_error_set_handler(eh); + snd_lib_error_set_local(eh); if (err < 0) goto __skip_add; cleanup_res = 1; @@ -337,10 +337,9 @@ static int try_config(struct hint_list *list, goto __ok; /* find, if all parameters have a default, */ /* otherwise filter this definition */ - eh = snd_lib_error; - snd_lib_error_set_handler(&zero_handler); + eh = snd_lib_error_set_local(&zero_handler); err = snd_config_search_alias_hooks(snd_config, base, buf, &res); - snd_lib_error_set_handler(eh); + snd_lib_error_set_local(eh); if (err < 0) goto __cleanup; if (snd_config_search(res, "@args", &cfg) >= 0) { diff --git a/src/error.c b/src/error.c index 7d5f509..6026ede 100644 --- a/src/error.c +++ b/src/error.c @@ -60,6 +60,23 @@ const char *snd_strerror(int errnum) return snd_error_codes[errnum]; }
+#define SUPPORT_TLS /* XXX should be checked in configure */ + +#ifdef SUPPORT_TLS +#define TLS_PFX __thread +#else +#define TLS_PFX /* NOP */ +#endif + +static TLS_PFX snd_local_error_handler_t local_error; + +snd_local_error_handler_t snd_lib_error_set_local(snd_local_error_handler_t func) +{ + snd_local_error_handler_t old = local_error; + local_error = func; + return old; +} + /** * \brief The default error handler function. * \param file The filename where the error was hit. @@ -75,6 +92,11 @@ static void snd_lib_error_default(const char *file, int line, const char *functi { va_list arg; va_start(arg, fmt); + if (local_error) { + local_error(file, line, function, err, fmt, arg); + va_end(arg); + return; + } fprintf(stderr, "ALSA lib %s:%i:(%s) ", file, line, function); vfprintf(stderr, fmt, arg); if (err)