[alsa-devel] [PATCH] conf: don't check if config files are up to date

[Takashi Iwai]

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 at 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
> >>
>