[alsa-devel] [PATCH] alsa-lib: snd_device_name_hint misbehaving
* Remove erroneous snd_config_delete calls that cause later calls to snd_pcm_open to fail.
* Don't list card-independent PCM devices ("null" or PulseAudio, for example) more than once even if multiple sound cards are installed.
* Add "ctl" to the list of device types that snd_device_name_hint can search for.
* Cache the path to libasound.so within snd_dlopen rather than looking it up with dladdr every call.
For a longer explanation: http://mailman.alsa-project.org/pipermail/alsa-devel/2009-October/022505.htm...
Signed-off-by: John Lindgren john.lindgren@tds.net
------
src/conf.c | 2 -- src/control/namehint.c | 34 +++++++++++++++++++++++++++------- src/dlmisc.c | 10 +++++++--- 3 files changed, 34 insertions(+), 12 deletions(-)
------
diff --git a/src/conf.c b/src/conf.c index 570c90f..52a477c 100644 --- a/src/conf.c +++ b/src/conf.c @@ -1132,7 +1132,6 @@ static int parse_def(snd_config_t *parent, input_t *input, int skip, int overrid free(id); continue; } - snd_config_delete(n); } if (mode == MERGE) { SNDERR("%s does not exists", id); @@ -1156,7 +1155,6 @@ static int parse_def(snd_config_t *parent, input_t *input, int skip, int overrid skip = 1; n = NULL; } else if (mode == OVERRIDE) { - snd_config_delete(n); n = NULL; } } else { diff --git a/src/control/namehint.c b/src/control/namehint.c index e878f83..ec9d04f 100644 --- a/src/control/namehint.c +++ b/src/control/namehint.c @@ -328,7 +328,6 @@ static int try_config(struct hint_list *list, if (snd_config_search(cfg1, "slave", &cfg) >= 0 && snd_config_search(cfg, base, &cfg1) >= 0) goto __hint; - snd_config_delete(res); res = NULL; if (strchr(buf, ':') != NULL) goto __ok; @@ -379,8 +378,6 @@ static int try_config(struct hint_list *list, err = hint_list_add(list, buf, buf1); } __skip_add: - if (res) - snd_config_delete(res); if (buf1) free(buf1); free(buf); @@ -450,13 +447,10 @@ static int add_card(struct hint_list *list, int card) if (err == -EXDEV) continue; if (err < 0) { + list->card = card; list->device = -1; err = try_config(list, list->siface, str); } - if (err < 0) { - list->card = -1; - err = try_config(list, list->siface, str); - } if (err == -ENOMEM) goto __error; } @@ -466,6 +460,29 @@ static int add_card(struct hint_list *list, int card) return err; }
+static int add_software_devices(struct hint_list *list) +{ + int err; + snd_config_t *conf, *n; + snd_config_iterator_t i, next; + const char *str; + + err = snd_config_search(snd_config, list->siface, &conf); + if (err < 0) + return err; + snd_config_for_each(i, next, conf) { + n = snd_config_iterator_entry(i); + if (snd_config_get_id(n, &str) < 0) + continue; + list->card = -1; + list->device = -1; + err = try_config(list, list->siface, str); + if (err == -ENOMEM) + return -ENOMEM; + } + return 0; +} + static int get_card_name(struct hint_list *list, int card) { char scard[16], *s; @@ -532,6 +549,8 @@ int snd_device_name_hint(int card, const char *iface, void ***hints) list.iface = SND_CTL_ELEM_IFACE_SEQUENCER; else if (strcmp(iface, "hwdep") == 0) list.iface = SND_CTL_ELEM_IFACE_HWDEP; + else if (strcmp(iface, "ctl") == 0) + list.iface = SND_CTL_ELEM_IFACE_MIXER; else return -EINVAL; list.show_all = 0; @@ -543,6 +562,7 @@ int snd_device_name_hint(int card, const char *iface, void ***hints) if (err >= 0) err = add_card(&list, card); } else { + add_software_devices(&list); err = snd_card_next(&card); if (err < 0) goto __error; diff --git a/src/dlmisc.c b/src/dlmisc.c index c882cdc..3cca0f0 100644 --- a/src/dlmisc.c +++ b/src/dlmisc.c @@ -54,9 +54,13 @@ void *snd_dlopen(const char *name, int mode) #else #ifdef HAVE_LIBDL if (name == NULL) { - Dl_info dlinfo; - if (dladdr(snd_dlopen, &dlinfo) > 0) - name = dlinfo.dli_fname; + static const char * self = NULL; + if (self == NULL) { + Dl_info dlinfo; + if (dladdr(snd_dlopen, &dlinfo) > 0) + self = dlinfo.dli_fname; + } + name = self; } #endif #endif
On Sun, 1 Nov 2009, John Lindgren wrote:
- Remove erroneous snd_config_delete calls that cause later calls to
snd_pcm_open to fail.
It does not look good. Have you checked with valgrind if there are no memory leaks?
- Don't list card-independent PCM devices ("null" or PulseAudio, for
example) more than once even if multiple sound cards are installed.
- Add "ctl" to the list of device types that snd_device_name_hint can
search for.
- Cache the path to libasound.so within snd_dlopen rather than looking
it up with dladdr every call.
Please, split your changes to single patches. Thanks.
Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
On Mon, 2009-11-02 at 07:21 +0100, Jaroslav Kysela wrote:
On Sun, 1 Nov 2009, John Lindgren wrote:
- Remove erroneous snd_config_delete calls that cause later calls to
snd_pcm_open to fail.
It does not look good. Have you checked with valgrind if there are no memory leaks?
You're making the same mistake that the original author of the code made. snd_config_delete is not a call to free memory; it is a call to remove sections of the loaded configuration. The configuration nodes that were being passed to snd_config_delete were returned by snd_config_search, which returns direct pointers to the actual nodes in the configuration. It does no memory duplication.
Please, split your changes to single patches. Thanks.
I will do so when I have the time to learn Git a little better. (I normally use Mercurial).
John Lindgren
At Mon, 02 Nov 2009 13:12:21 -0500, John Lindgren wrote:
On Mon, 2009-11-02 at 07:21 +0100, Jaroslav Kysela wrote:
On Sun, 1 Nov 2009, John Lindgren wrote:
- Remove erroneous snd_config_delete calls that cause later calls to
snd_pcm_open to fail.
It does not look good. Have you checked with valgrind if there are no memory leaks?
You're making the same mistake that the original author of the code made. snd_config_delete is not a call to free memory; it is a call to remove sections of the loaded configuration. The configuration nodes that were being passed to snd_config_delete were returned by snd_config_search, which returns direct pointers to the actual nodes in the configuration. It does no memory duplication.
Well, no, the code there should be OK. It's the place to replace the whole sub-tree by overriding the object. Thus, freeing the object returned from snd_config_search() is correct.
The problem is rather in namehint.c, as found in my patch in another post.
Please, split your changes to single patches. Thanks.
I will do so when I have the time to learn Git a little better. (I normally use Mercurial).
This is easy even without VCS. Just split your own patch to several files by an editor :) Namely, the change of dlmisc.c, and the addition of "ctl" check in namehint.c.
thanks,
Takashi
Is there any test program for this change since currently only "hw" and "pulse" can be listed by name hint ?
* Add "ctl" to the list of device types that snd_device_name_hint can search for.
2009/11/2 John Lindgren john.lindgren@tds.net
- Remove erroneous snd_config_delete calls that cause later calls to
snd_pcm_open to fail.
- Don't list card-independent PCM devices ("null" or PulseAudio, for
example) more than once even if multiple sound cards are installed.
- Add "ctl" to the list of device types that snd_device_name_hint can
search for.
- Cache the path to libasound.so within snd_dlopen rather than looking
it up with dladdr every call.
For a longer explanation:
http://mailman.alsa-project.org/pipermail/alsa-devel/2009-October/022505.htm...
Signed-off-by: John Lindgren john.lindgren@tds.net
src/conf.c | 2 -- src/control/namehint.c | 34 +++++++++++++++++++++++++++------- src/dlmisc.c | 10 +++++++--- 3 files changed, 34 insertions(+), 12 deletions(-)
diff --git a/src/conf.c b/src/conf.c index 570c90f..52a477c 100644 --- a/src/conf.c +++ b/src/conf.c @@ -1132,7 +1132,6 @@ static int parse_def(snd_config_t *parent, input_t *input, int skip, int overrid free(id); continue; }
snd_config_delete(n); } if (mode == MERGE) { SNDERR("%s does not exists", id);@@ -1156,7 +1155,6 @@ static int parse_def(snd_config_t *parent, input_t *input, int skip, int overrid skip = 1; n = NULL; } else if (mode == OVERRIDE) {
snd_config_delete(n); n = NULL; } } else {diff --git a/src/control/namehint.c b/src/control/namehint.c index e878f83..ec9d04f 100644 --- a/src/control/namehint.c +++ b/src/control/namehint.c @@ -328,7 +328,6 @@ static int try_config(struct hint_list *list, if (snd_config_search(cfg1, "slave", &cfg) >= 0 && snd_config_search(cfg, base, &cfg1) >= 0) goto __hint;
snd_config_delete(res); res = NULL; if (strchr(buf, ':') != NULL) goto __ok;@@ -379,8 +378,6 @@ static int try_config(struct hint_list *list, err = hint_list_add(list, buf, buf1); } __skip_add:
if (res)snd_config_delete(res); if (buf1) free(buf1); free(buf);@@ -450,13 +447,10 @@ static int add_card(struct hint_list *list, int card) if (err == -EXDEV) continue; if (err < 0) {
list->card = card; list->device = -1; err = try_config(list, list->siface, str); }
if (err < 0) {list->card = -1;err = try_config(list, list->siface, str);} if (err == -ENOMEM) goto __error; }@@ -466,6 +460,29 @@ static int add_card(struct hint_list *list, int card) return err; }
+static int add_software_devices(struct hint_list *list) +{
int err;snd_config_t *conf, *n;snd_config_iterator_t i, next;const char *str;err = snd_config_search(snd_config, list->siface, &conf);if (err < 0)return err;snd_config_for_each(i, next, conf) {n = snd_config_iterator_entry(i);if (snd_config_get_id(n, &str) < 0)continue;list->card = -1;list->device = -1;err = try_config(list, list->siface, str);if (err == -ENOMEM)return -ENOMEM;}return 0;+}
static int get_card_name(struct hint_list *list, int card) { char scard[16], *s; @@ -532,6 +549,8 @@ int snd_device_name_hint(int card, const char *iface, void ***hints) list.iface = SND_CTL_ELEM_IFACE_SEQUENCER; else if (strcmp(iface, "hwdep") == 0) list.iface = SND_CTL_ELEM_IFACE_HWDEP;
else if (strcmp(iface, "ctl") == 0)list.iface = SND_CTL_ELEM_IFACE_MIXER; else return -EINVAL; list.show_all = 0;@@ -543,6 +562,7 @@ int snd_device_name_hint(int card, const char *iface, void ***hints) if (err >= 0) err = add_card(&list, card); } else {
add_software_devices(&list); err = snd_card_next(&card); if (err < 0) goto __error;diff --git a/src/dlmisc.c b/src/dlmisc.c index c882cdc..3cca0f0 100644 --- a/src/dlmisc.c +++ b/src/dlmisc.c @@ -54,9 +54,13 @@ void *snd_dlopen(const char *name, int mode) #else #ifdef HAVE_LIBDL if (name == NULL) {
Dl_info dlinfo;if (dladdr(snd_dlopen, &dlinfo) > 0)name = dlinfo.dli_fname;
static const char * self = NULL;if (self == NULL) {Dl_info dlinfo;if (dladdr(snd_dlopen, &dlinfo) > 0)self = dlinfo.dli_fname;}name = self; }#endif #endif
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
On Mon, 2009-11-02 at 14:08 +0100, Takashi Iwai wrote:
- Remove erroneous snd_config_delete calls that cause later calls to
snd_pcm_open to fail.
This is bad indeed. Could you make a small test case to reproduce the problem more easily?
Sure.
------
#include <stdio.h> #include <alsa/asoundlib.h>
void try_open (const char * pcm) { snd_pcm_t * handle; int error;
error = snd_pcm_open (& handle, pcm, SND_PCM_STREAM_PLAYBACK, 0);
if (error < 0) printf ("Failed to open %s: %s.\n", pcm, snd_strerror (error)); else { printf ("Opened %s.\n", pcm); snd_pcm_close (handle); } }
int main (void) { void * * hints;
try_open ("default"); try_open ("front"); try_open ("pulse"); try_open ("default"); try_open ("front"); try_open ("pulse");
printf ("Calling snd_device_name_hint().\n"); snd_device_name_hint (-1, "pcm", & hints); snd_device_name_free_hint (hints);
try_open ("default"); try_open ("front"); try_open ("pulse"); try_open ("default"); try_open ("front"); try_open ("pulse");
return 0; }
------
$ ./pcmtest Opened default. Opened front. Opened pulse. Opened default. Opened front. Opened pulse. Calling snd_device_name_hint(). Opened default. ALSA lib pcm.c:2211:(snd_pcm_open_noupdate) Unknown PCM cards.pcm.front Failed to open front: No such file or directory. ALSA lib pcm.c:2211:(snd_pcm_open_noupdate) Unknown PCM pulse Failed to open pulse: No such file or directory. Opened default. ALSA lib pcm.c:2211:(snd_pcm_open_noupdate) Unknown PCM cards.pcm.front Failed to open front: No such file or directory. ALSA lib pcm.c:2211:(snd_pcm_open_noupdate) Unknown PCM pulse Failed to open pulse: No such file or directory.
At Mon, 02 Nov 2009 09:43:14 -0500, John Lindgren wrote:
On Mon, 2009-11-02 at 14:08 +0100, Takashi Iwai wrote:
- Remove erroneous snd_config_delete calls that cause later calls to
snd_pcm_open to fail.
This is bad indeed. Could you make a small test case to reproduce the problem more easily?
Sure.
Thanks. I guess this depends on the config files. Could you attach your ones?
Takashi
On Mon, 2009-11-02 at 15:55 +0100, Takashi Iwai wrote:
Thanks. I guess this depends on the config files. Could you attach your ones?
I can reproduce with only the stock /usr/share/alsa/alsa.conf from Debian installed if I try to use the "null" device after snd_device_name_hint. /usr/share/alsa/alsa.conf is attached.
------
#include <stdio.h> #include <alsa/asoundlib.h>
void try_open (const char * pcm) { snd_pcm_t * handle; int error;
error = snd_pcm_open (& handle, pcm, SND_PCM_STREAM_PLAYBACK, 0);
if (error < 0) printf ("Failed to open %s: %s.\n", pcm, snd_strerror (error)); else { printf ("Opened %s.\n", pcm); snd_pcm_close (handle); } }
int main (void) { void * * hints;
try_open ("default"); try_open ("null"); try_open ("default"); try_open ("null");
printf ("Calling snd_device_name_hint().\n"); snd_device_name_hint (-1, "pcm", & hints); snd_device_name_free_hint (hints);
try_open ("default"); try_open ("null"); try_open ("default"); try_open ("null");
return 0; }
------
$ ./pcmtest Opened default. Opened null. Opened default. Opened null. Calling snd_device_name_hint(). Opened default. ALSA lib pcm.c:2211:(snd_pcm_open_noupdate) Unknown PCM null Failed to open null: No such file or directory. Opened default. ALSA lib pcm.c:2211:(snd_pcm_open_noupdate) Unknown PCM null Failed to open null: No such file or directory.
At Mon, 02 Nov 2009 12:58:20 -0500, John Lindgren wrote:
On Mon, 2009-11-02 at 15:55 +0100, Takashi Iwai wrote:
Thanks. I guess this depends on the config files. Could you attach your ones?
I can reproduce with only the stock /usr/share/alsa/alsa.conf from Debian installed if I try to use the "null" device after snd_device_name_hint. /usr/share/alsa/alsa.conf is attached.
Thanks! I could reproduce the problem on my machine, too.
Now I think I found the culprit. This is not in the core conf.c code, but it's in namehint.c. Try the patch below.
The point is that the variable "res" can be two different instances in try_config(). One is the result of snd_config_search_definition(). In this case, res is the copied (expanded) objects.
This one is freed in the middle of try_config(), and then res is assigned again to another one by snd_config_search_alias_hooks(). This guy is no expanded object but a reference.
However, the original code calls snd_config_free(res) no matter which object is. So, freeing the latter one results in the breakage of the config tree.
The patch adds the check of the object type to be freed or not.
Takashi
--- diff --git a/src/control/namehint.c b/src/control/namehint.c index e878f83..a134ed7 100644 --- a/src/control/namehint.c +++ b/src/control/namehint.c @@ -219,6 +219,7 @@ static int try_config(struct hint_list *list, const char *str; int err = 0, level; long dev = list->device; + int cleanup_res = 0;
list->device_input = -1; list->device_output = -1; @@ -244,6 +245,7 @@ static int try_config(struct hint_list *list, snd_lib_error_set_handler(eh); if (err < 0) goto __skip_add; + cleanup_res = 1; err = -EINVAL; if (snd_config_get_type(res) != SND_CONFIG_TYPE_COMPOUND) goto __cleanup; @@ -330,6 +332,7 @@ static int try_config(struct hint_list *list, goto __hint; snd_config_delete(res); res = NULL; + cleanup_res = 0; if (strchr(buf, ':') != NULL) goto __ok; /* find, if all parameters have a default, */ @@ -379,7 +382,7 @@ static int try_config(struct hint_list *list, err = hint_list_add(list, buf, buf1); } __skip_add: - if (res) + if (res && cleanup_res) snd_config_delete(res); if (buf1) free(buf1);
At Tue, 03 Nov 2009 08:09:34 +0100, I wrote:
At Mon, 02 Nov 2009 12:58:20 -0500, John Lindgren wrote:
On Mon, 2009-11-02 at 15:55 +0100, Takashi Iwai wrote:
Thanks. I guess this depends on the config files. Could you attach your ones?
I can reproduce with only the stock /usr/share/alsa/alsa.conf from Debian installed if I try to use the "null" device after snd_device_name_hint. /usr/share/alsa/alsa.conf is attached.
Thanks! I could reproduce the problem on my machine, too.
Now I think I found the culprit. This is not in the core conf.c code, but it's in namehint.c. Try the patch below.
The point is that the variable "res" can be two different instances in try_config(). One is the result of snd_config_search_definition(). In this case, res is the copied (expanded) objects.
This one is freed in the middle of try_config(), and then res is assigned again to another one by snd_config_search_alias_hooks(). This guy is no expanded object but a reference.
However, the original code calls snd_config_free(res) no matter which object is. So, freeing the latter one results in the breakage of the config tree.
The patch adds the check of the object type to be freed or not.
Since no problem is found with valgrind, I applied the patch to git tree now.
thanks,
Takashi
On Tue, 2009-11-03 at 09:03 +0100, Takashi Iwai wrote:
Since no problem is found with valgrind, I applied the patch to git tree now.
Thanks. I compiled the git version and the problem is fixed. Here are the other changes, split into four separate patches:
* Card-independent devices such as "null" or "pulse" should only be added once, not once for each card.
diff --git a/src/control/namehint.c b/src/control/namehint.c index a134ed7..408b36f 100644 --- a/src/control/namehint.c +++ b/src/control/namehint.c @@ -456,10 +456,6 @@ static int add_card(struct hint_list *list, int card) list->device = -1; err = try_config(list, list->siface, str); } - if (err < 0) { - list->card = -1; - err = try_config(list, list->siface, str); - } if (err == -ENOMEM) goto __error; } @@ -485,6 +481,29 @@ static int get_card_name(struct hint_list *list, int card) return 0; }
+static int add_software_devices(struct hint_list *list) +{ + int err; + snd_config_t *conf, *n; + snd_config_iterator_t i, next; + const char *str; + + err = snd_config_search(snd_config, list->siface, &conf); + if (err < 0) + return err; + snd_config_for_each(i, next, conf) { + n = snd_config_iterator_entry(i); + if (snd_config_get_id(n, &str) < 0) + continue; + list->card = -1; + list->device = -1; + err = try_config(list, list->siface, str); + if (err == -ENOMEM) + return -ENOMEM; + } + return 0; +} + /** * \brief Return string list with device name hints. * \param card Card number or -1 (means all cards) @@ -546,6 +565,7 @@ int snd_device_name_hint(int card, const char *iface, void ***hints) if (err >= 0) err = add_card(&list, card); } else { + add_software_devices(&list); err = snd_card_next(&card); if (err < 0) goto __error;
<<<<<<
* Allow snd_device_name_hint to search for mixer devices.
diff --git a/src/control/namehint.c b/src/control/namehint.c index 408b36f..34338dc 100644 --- a/src/control/namehint.c +++ b/src/control/namehint.c @@ -554,6 +554,8 @@ int snd_device_name_hint(int card, const char *iface, void ***hints) list.iface = SND_CTL_ELEM_IFACE_SEQUENCER; else if (strcmp(iface, "hwdep") == 0) list.iface = SND_CTL_ELEM_IFACE_HWDEP; + else if (strcmp(iface, "ctl") == 0) + list.iface = SND_CTL_ELEM_IFACE_MIXER; else return -EINVAL; list.show_all = 0;
<<<<<<
* list->card is wrongly assumed to be initialized, but the previous initialization is within a conditional that is false when only card-independent devices are found. (This is the case when searching for mixers on my system; the end result is that the "pulse" mixer is listed three times.)
diff --git a/src/control/namehint.c b/src/control/namehint.c index 34338dc..78572d8 100644 --- a/src/control/namehint.c +++ b/src/control/namehint.c @@ -453,6 +453,7 @@ static int add_card(struct hint_list *list, int card) if (err == -EXDEV) continue; if (err < 0) { + list->card = card; list->device = -1; err = try_config(list, list->siface, str); }
<<<<<<
* Speed up repeated calls to snd_dlopen by caching the path to libasound.so; this reduces the instructions executed by snd_device_name_hint by 40 percent.
diff --git a/src/dlmisc.c b/src/dlmisc.c index c882cdc..3cca0f0 100644 --- a/src/dlmisc.c +++ b/src/dlmisc.c @@ -54,9 +54,13 @@ void *snd_dlopen(const char *name, int mode) #else #ifdef HAVE_LIBDL if (name == NULL) { - Dl_info dlinfo; - if (dladdr(snd_dlopen, &dlinfo) > 0) - name = dlinfo.dli_fname; + static const char * self = NULL; + if (self == NULL) { + Dl_info dlinfo; + if (dladdr(snd_dlopen, &dlinfo) > 0) + self = dlinfo.dli_fname; + } + name = self; } #endif #endif
<<<<<<
Peace, John Lindgren
On Tue, 3 Nov 2009, John Lindgren wrote:
On Tue, 2009-11-03 at 09:03 +0100, Takashi Iwai wrote:
Since no problem is found with valgrind, I applied the patch to git tree now.
Thanks. I compiled the git version and the problem is fixed. Here are the other changes, split into four separate patches:
I applied your changes. But please ... send one patch in one e-mail in future to reduce our work with patch importing. Also, it makes sense to have one-line subject for patch and Signed-off-by: line in patches.
Thanks, Jaroslav
----- Jaroslav Kysela perex@perex.cz Linux Kernel Sound Maintainer ALSA Project, Red Hat, Inc.
participants (4)
-
Jaroslav Kysela -
John Lindgren -
Raymond Yau -
Takashi Iwai