On Tue, 10 May 2016 10:40:08 +0200, Alexander E. Patrakov wrote: > > 09.05.2016 17:44, Takashi Iwai wrote: > > On Sun, 01 May 2016 14:43:42 +0200, > > Alexander E. Patrakov wrote: > >> Even if no files have been changed, the config may become outdated > >> due to hot-plugged devices. > >> > >> Note: this patch has a huge potential to crash badly-written applications > >> that keep config pointers and strings for too long. But I see no other > >> way to support hot-pluggable devices. > > Can we opt in this feature, e.g. by enabling it via some new API > > function? Enabling this blindly appears too risky. > > Hello Takashi, > > I have read your remark, and your concern does make sense. However, I > have some doubts whether any new API is needed, and some suggestions > for further changes. Here are my ideas. > > 1. We already have such API. It is called snd_config_top(), > snd_config_update_r() and snd_pcm_open_lconf(). Would you accept a > documentation patch that says that snd_pcm_open cannot deal with > hot-plugged devices, and directs users to snd_pcm_open_lconf()? Of > course, this depends on Tanu's willingness to accept conversion to > snd_pcm_open_lconf() in PulseAudio. Yeah, it'd be good to mention there. > 2. Even if a user calls snd_pcm_open_lconf(), currently, when > attempting to figure out the driver, mixer configuration is updated > with snd_config_update() - i.e. global configuration is updated, and > this IMHO defeats the point of having a separate configuration. To fix > the bug, I would need to make sure that the mixer configuration is > read from the same snd_config_t structure that was passed to > snd_pcm_open_lconf(). I am not sure about the difficulty of this task. > > 3. One of my early attempts of the patch (without the hunk about > snd_ctl_open_lconf()) was crashing due to bad pointers. Now I realize > that it is already a bug in alsa-lib - the current code would crash, > too, if the configuration file has changed between the calls to > snd_pcm_update() in snd_pcm_open() and in snd_ctl_open() (which is > called from a hook that determines the driver). Again, I would need to > make sure that the mixer configuration is read from the same > snd_config_t structure that was passed to snd_pcm_open_noupdate(), see > above. Right. The crash doesn't happen so often because the update of config tree doesn't happen also so often. > 4. I still think that the file mtime check in snd_config_update_r does > not make any sense. It fails to notice changes in included files, as > well as in the list of plugged-in devices. In other words, it never > worked except for the initial read, and it's too risky to make it > actually work as documented. How about making it read the > configuration from files only on the first call for a given config, > even if they have changed? Yes, that sounds more reasonable as a first move. It doesn't work 100% reliably, and if you really want a proper update, you can do it by calling snd_config_update_free_global() beforehand. There is a minimal protection by pthread mutex in conf.c, but it's only at regenerating the config tree. We need refcounting by referrer for a proper protection. Once after having the refcount protection, we can safely update the config tree unconditionally. thanks, Takashi > > What do you think? > > -- > Alexander E. Patrakov > > > > > > > > thanks, > > > > Takashi > > > >> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=54029 > >> Signed-off-by: Alexander E. Patrakov <patrakov@xxxxxxxxx> > >> --- > >> src/conf.c | 63 ++++++++++++++++++++++++---------------------------------- > >> src/confmisc.c | 4 +++- > >> 2 files changed, 29 insertions(+), 38 deletions(-) > >> > >> diff --git a/src/conf.c b/src/conf.c > >> index f8b7a66..34576aa 100644 > >> --- a/src/conf.c > >> +++ b/src/conf.c > >> @@ -3661,14 +3661,15 @@ SND_DLSYM_BUILD_VERSION(snd_config_hook_load_for_all_cards, SND_CONFIG_DLSYM_VER > >> #endif > >> /** > >> - * \brief Updates a configuration tree by rereading the configuration files (if needed). > >> + * \brief Updates a configuration tree by rereading the configuration files. > >> * \param[in,out] _top Address of the handle to the top-level node. > >> * \param[in,out] _update Address of a pointer to private update information. > >> * \param[in] cfgs A list of configuration file names, delimited with ':'. > >> * If \p cfgs is \c NULL, the default global > >> * configuration file is used. > >> - * \return 0 if \a _top was up to date, 1 if the configuration files > >> - * have been reread, otherwise a negative error code. > >> + * \return 0 if \a _top was up to date (this can happen only in previous > >> + * ALSA-lib versions), 1 if the configuration files have been > >> + * reread successfully, otherwise a negative error code. > >> * > >> * The variables pointed to by \a _top and \a _update can be initialized > >> * to \c NULL before the first call to this function. The private > >> @@ -3679,7 +3680,8 @@ SND_DLSYM_BUILD_VERSION(snd_config_hook_load_for_all_cards, SND_CONFIG_DLSYM_VER > >> * The global configuration files are specified in the environment variable > >> * \c ALSA_CONFIG_PATH. > >> * > >> - * \warning If the configuration tree is reread, all string pointers and > >> + * \warning In order to deal with hot-pluggable devices, the configuration > >> + * tree is always reread, even if nothing changed. All string pointers and > >> * configuration node handles previously obtained from this tree become > >> * invalid. > >> * > >> @@ -3754,35 +3756,6 @@ int snd_config_update_r(snd_config_t **_top, snd_config_update_t **_update, cons > >> local->count--; > >> } > >> } > >> - if (!update) > >> - goto _reread; > >> - if (local->count != update->count) > >> - goto _reread; > >> - for (k = 0; k < local->count; ++k) { > >> - struct finfo *lf = &local->finfo[k]; > >> - struct finfo *uf = &update->finfo[k]; > >> - if (strcmp(lf->name, uf->name) != 0 || > >> - lf->dev != uf->dev || > >> - lf->ino != uf->ino || > >> - lf->mtime != uf->mtime) > >> - goto _reread; > >> - } > >> - err = 0; > >> - > >> - _end: > >> - if (err < 0) { > >> - if (top) { > >> - snd_config_delete(top); > >> - *_top = NULL; > >> - } > >> - if (update) { > >> - snd_config_update_free(update); > >> - *_update = NULL; > >> - } > >> - } > >> - if (local) > >> - snd_config_update_free(local); > >> - return err; > >> _reread: > >> *_top = NULL; > >> @@ -3823,15 +3796,31 @@ int snd_config_update_r(snd_config_t **_top, snd_config_update_t **_update, cons > >> *_top = top; > >> *_update = local; > >> return 1; > >> + > >> +_end: > >> + if (err < 0) { > >> + if (top) { > >> + snd_config_delete(top); > >> + *_top = NULL; > >> + } > >> + if (update) { > >> + snd_config_update_free(update); > >> + *_update = NULL; > >> + } > >> + } > >> + if (local) > >> + snd_config_update_free(local); > >> + return err; > >> } > >> /** > >> - * \brief Updates #snd_config by rereading the global configuration files (if needed). > >> - * \return 0 if #snd_config was up to date, 1 if #snd_config was > >> - * updated, otherwise a negative error code. > >> + * \brief Updates #snd_config by rereading the global configuration files. > >> + * \return 0 if #snd_config was up to date (this can happen only in previous > >> + * ALSA-lib versions), 1 if #snd_config was updated successfully, > >> + * otherwise a negative error code. > >> * > >> * \warning Whenever #snd_config is updated, all string pointers and > >> - * configuration node handles previously obtained from it may become > >> + * configuration node handles previously obtained from it become > >> * invalid. > >> * > >> * \par Errors: > >> diff --git a/src/confmisc.c b/src/confmisc.c > >> index ae0275f..8f97b72 100644 > >> --- a/src/confmisc.c > >> +++ b/src/confmisc.c > >> @@ -599,7 +599,9 @@ static int open_ctl(long card, snd_ctl_t **ctl) > >> char name[16]; > >> snprintf(name, sizeof(name), "hw:%li", card); > >> name[sizeof(name)-1] = '\0'; > >> - return snd_ctl_open(ctl, name, 0); > >> + > >> + /* Take care not to update the config - we may be running hooks on pcms right now */ > >> + return snd_ctl_open_lconf(ctl, name, 0, snd_config); > >> } > >> #if 0 > >> -- > >> 2.8.0 > >> > _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel