| Message ID | 1462106622-29461-1-git-send-email-patrakov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. thanks, Takashi > Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=54029 > Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com> > --- > 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 >
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. 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. 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? What do you think?
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@gmail.com> > >> --- > >> 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 > >> >
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
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. Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=54029 Signed-off-by: Alexander E. Patrakov <patrakov@gmail.com> --- src/conf.c | 63 ++++++++++++++++++++++++---------------------------------- src/confmisc.c | 4 +++- 2 files changed, 29 insertions(+), 38 deletions(-)