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

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

[Alexander E. Patrakov]

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(-)

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

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

[Takashi Iwai]

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
> 

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

[Alexander E. Patrakov]

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?

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

Re: [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@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
> >>
> 

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

[Takashi Iwai]

On Thu, 12 May 2016 15:45:23 +0200,
Takashi Iwai wrote:
> 
> 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.

So here is a quick hack.  It adds a simple refcount in snd_config_t.
snd_config_update_get() gives a top config with a refcount so that it
won't be deleted until disposed by snd_config_put().

In this patch, the config update itself is kept in traditional way,
i.e. lazy update with mtime check.  For the forced refresh, I added
snd_config_refresh() which removes snd_config_global_update so that at
the next snd_config_update() will reread properly.  PA can call this
function appropriately, e.g. when hotplugged.

The patch is just a proof of concept and even untested.  Once when
confirmed that it's the way to go, I'll cook up more.


Takashi

---
diff --git a/include/conf.h b/include/conf.h
index 087c05dc6bcf..72fd7c06a8d9 100644
--- a/include/conf.h
+++ b/include/conf.h
@@ -94,6 +94,10 @@ int snd_config_update_r(snd_config_t **top, snd_config_update_t **update, const
 int snd_config_update_free(snd_config_update_t *update);
 int snd_config_update_free_global(void);
 
+void snd_config_refresh(void);
+int snd_config_update_get(snd_config_t **top);
+void snd_config_put(snd_config_t *top);
+
 int snd_config_search(snd_config_t *config, const char *key,
 		      snd_config_t **result);
 int snd_config_searchv(snd_config_t *config, 
diff --git a/src/conf.c b/src/conf.c
index f8b7a6686529..fb395dc42601 100644
--- a/src/conf.c
+++ b/src/conf.c
@@ -434,6 +434,7 @@ static pthread_once_t snd_config_update_mutex_once = PTHREAD_ONCE_INIT;
 struct _snd_config {
 	char *id;
 	snd_config_type_t type;
+	int refcount;
 	union {
 		long integer;
 		long long integer64;
@@ -1833,6 +1834,10 @@ int snd_config_remove(snd_config_t *config)
 int snd_config_delete(snd_config_t *config)
 {
 	assert(config);
+	if (config->refcount > 0) {
+		config->refcount--;
+		return 0;
+	}
 	switch (config->type) {
 	case SND_CONFIG_TYPE_COMPOUND:
 	{
@@ -3851,6 +3856,39 @@ int snd_config_update(void)
 	return err;
 }
 
+void snd_config_refresh(void)
+{
+	snd_config_lock();
+	if (snd_config_global_update)
+		snd_config_update_free(snd_config_global_update);
+	snd_config_global_update = NULL;
+	snd_config_unlock();
+}
+
+int snd_config_update_get(snd_config_t **top)
+{
+	int err;
+
+	*top = NULL;
+	snd_config_lock();
+	err = snd_config_update_r(&snd_config, &snd_config_global_update, NULL);
+	if (!err) {
+		*top = snd_config;
+		if (*top)
+			(*top)->refcount++;
+	}
+	snd_config_unlock();
+	return err;
+}
+
+void snd_config_put(snd_config_t *cfg)
+{
+	snd_config_lock();
+	if (cfg)
+		snd_config_delete(cfg);
+	snd_config_unlock();
+}
+
 /** 
  * \brief Frees a private update structure.
  * \param[in] update The private update structure to free.
diff --git a/src/control/control.c b/src/control/control.c
index 8a5d530f2674..6078189a6a70 100644
--- a/src/control/control.c
+++ b/src/control/control.c
@@ -968,12 +968,16 @@ static int snd_ctl_open_noupdate(snd_ctl_t **ctlp, snd_config_t *root, const cha
  */
 int snd_ctl_open(snd_ctl_t **ctlp, const char *name, int mode)
 {
+	snd_config_t *top;
 	int err;
+
 	assert(ctlp && name);
-	err = snd_config_update();
+	err = snd_config_update_get(&top);
 	if (err < 0)
 		return err;
-	return snd_ctl_open_noupdate(ctlp, snd_config, name, mode);
+	err = snd_ctl_open_noupdate(ctlp, top, name, mode);
+	snd_config_put(top);
+	return err;
 }
 
 /**
diff --git a/src/hwdep/hwdep.c b/src/hwdep/hwdep.c
index 5dc791c99189..c8b368fa68a2 100644
--- a/src/hwdep/hwdep.c
+++ b/src/hwdep/hwdep.c
@@ -168,12 +168,16 @@ static int snd_hwdep_open_noupdate(snd_hwdep_t **hwdep, snd_config_t *root, cons
  */
 int snd_hwdep_open(snd_hwdep_t **hwdep, const char *name, int mode)
 {
+	snd_config_t *top;
 	int err;
+
 	assert(hwdep && name);
-	err = snd_config_update();
+	err = snd_config_update_get(&top);
 	if (err < 0)
 		return err;
-	return snd_hwdep_open_noupdate(hwdep, snd_config, name, mode);
+	err = snd_hwdep_open_noupdate(hwdep, top, name, mode);
+	snd_config_put(top);
+	return err;
 }
 
 /**
diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index 203e7a52491b..42da2c8a8b55 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -2288,12 +2288,16 @@ static int snd_pcm_open_noupdate(snd_pcm_t **pcmp, snd_config_t *root,
 int snd_pcm_open(snd_pcm_t **pcmp, const char *name, 
 		 snd_pcm_stream_t stream, int mode)
 {
+	snd_config_t *top;
 	int err;
+
 	assert(pcmp && name);
-	err = snd_config_update();
+	err = snd_config_update_get(&top);
 	if (err < 0)
 		return err;
-	return snd_pcm_open_noupdate(pcmp, snd_config, name, stream, mode, 0);
+	err = snd_pcm_open_noupdate(pcmp, top, name, stream, mode, 0);
+	snd_config_put(top);
+	return err;
 }
 
 /**
diff --git a/src/rawmidi/rawmidi.c b/src/rawmidi/rawmidi.c
index 0c89b8b984b9..d965edaf1f42 100644
--- a/src/rawmidi/rawmidi.c
+++ b/src/rawmidi/rawmidi.c
@@ -305,12 +305,16 @@ static int snd_rawmidi_open_noupdate(snd_rawmidi_t **inputp, snd_rawmidi_t **out
 int snd_rawmidi_open(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp,
 		     const char *name, int mode)
 {
+	snd_config_t *top;
 	int err;
+
 	assert((inputp || outputp) && name);
-	err = snd_config_update();
+	err = snd_config_update_get(&top);
 	if (err < 0)
 		return err;
-	return snd_rawmidi_open_noupdate(inputp, outputp, snd_config, name, mode);
+	err = snd_rawmidi_open_noupdate(inputp, outputp, top, name, mode);
+	snd_config_put(top);
+	return err;
 }
 
 /**
diff --git a/src/seq/seq.c b/src/seq/seq.c
index 4405e68a7fe9..c1aa7981d557 100644
--- a/src/seq/seq.c
+++ b/src/seq/seq.c
@@ -974,12 +974,16 @@ static int snd_seq_open_noupdate(snd_seq_t **seqp, snd_config_t *root,
 int snd_seq_open(snd_seq_t **seqp, const char *name, 
 		 int streams, int mode)
 {
+	snd_config_t *top;
 	int err;
+
 	assert(seqp && name);
-	err = snd_config_update();
+	err = snd_config_update_get(&top);
 	if (err < 0)
 		return err;
-	return snd_seq_open_noupdate(seqp, snd_config, name, streams, mode, 0);
+	err = snd_seq_open_noupdate(seqp, top, name, streams, mode, 0);
+	snd_config_put(top);
+	return err;
 }
 
 /**
diff --git a/src/timer/timer.c b/src/timer/timer.c
index a25e4f797ce4..6af6d8224df1 100644
--- a/src/timer/timer.c
+++ b/src/timer/timer.c
@@ -201,12 +201,16 @@ static int snd_timer_open_noupdate(snd_timer_t **timer, snd_config_t *root, cons
  */
 int snd_timer_open(snd_timer_t **timer, const char *name, int mode)
 {
+	snd_config_t *top;
 	int err;
+
 	assert(timer && name);
-	err = snd_config_update();
+	err = snd_config_update_get(&top);
 	if (err < 0)
 		return err;
-	return snd_timer_open_noupdate(timer, snd_config, name, mode);
+	err = snd_timer_open_noupdate(timer, top, name, mode);
+	snd_config_put(top);
+	return err;
 }
 
 /**
diff --git a/src/timer/timer_query.c b/src/timer/timer_query.c
index 93d2455d07fc..23bf0711aafe 100644
--- a/src/timer/timer_query.c
+++ b/src/timer/timer_query.c
@@ -159,12 +159,16 @@ static int snd_timer_query_open_noupdate(snd_timer_query_t **timer, snd_config_t
  */
 int snd_timer_query_open(snd_timer_query_t **timer, const char *name, int mode)
 {
+	snd_config_t *top;
 	int err;
+
 	assert(timer && name);
-	err = snd_config_update();
+	err = snd_config_update_get(&top);
 	if (err < 0)
 		return err;
-	return snd_timer_query_open_noupdate(timer, snd_config, name, mode);
+	err = snd_timer_query_open_noupdate(timer, top, name, mode);
+	snd_config_put(top);
+	return err;
 }
 
 /**

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

[Takashi Iwai]

On Fri, 13 May 2016 16:07:12 +0200,
Takashi Iwai wrote:
> 
> On Thu, 12 May 2016 15:45:23 +0200,
> Takashi Iwai wrote:
> > 
> > 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.
> 
> So here is a quick hack.  It adds a simple refcount in snd_config_t.
> snd_config_update_get() gives a top config with a refcount so that it
> won't be deleted until disposed by snd_config_put().
> 
> In this patch, the config update itself is kept in traditional way,
> i.e. lazy update with mtime check.  For the forced refresh, I added
> snd_config_refresh() which removes snd_config_global_update so that at
> the next snd_config_update() will reread properly.  PA can call this
> function appropriately, e.g. when hotplugged.
> 
> The patch is just a proof of concept and even untested.  Once when
> confirmed that it's the way to go, I'll cook up more.

There was a typo indeed.  The revised patch is below.
aplay worked at least :)  And now it has comments.


Takashi

---
diff --git a/include/conf.h b/include/conf.h
index 087c05dc6bcf..72fd7c06a8d9 100644
--- a/include/conf.h
+++ b/include/conf.h
@@ -94,6 +94,10 @@ int snd_config_update_r(snd_config_t **top, snd_config_update_t **update, const
 int snd_config_update_free(snd_config_update_t *update);
 int snd_config_update_free_global(void);
 
+void snd_config_refresh(void);
+int snd_config_update_get(snd_config_t **top);
+void snd_config_put(snd_config_t *top);
+
 int snd_config_search(snd_config_t *config, const char *key,
 		      snd_config_t **result);
 int snd_config_searchv(snd_config_t *config, 
diff --git a/src/conf.c b/src/conf.c
index f8b7a6686529..1386a38b0b27 100644
--- a/src/conf.c
+++ b/src/conf.c
@@ -434,6 +434,7 @@ static pthread_once_t snd_config_update_mutex_once = PTHREAD_ONCE_INIT;
 struct _snd_config {
 	char *id;
 	snd_config_type_t type;
+	int refcount; /* default = 0 */
 	union {
 		long integer;
 		long long integer64;
@@ -1833,6 +1834,10 @@ int snd_config_remove(snd_config_t *config)
 int snd_config_delete(snd_config_t *config)
 {
 	assert(config);
+	if (config->refcount > 0) {
+		config->refcount--;
+		return 0;
+	}
 	switch (config->type) {
 	case SND_CONFIG_TYPE_COMPOUND:
 	{
@@ -3851,6 +3856,60 @@ int snd_config_update(void)
 	return err;
 }
 
+/**
+ * \brief Clear the local config update cache
+ *
+ * This function clears the local config update cache to guarantee that
+ * the next call of #snd_config_update will reread the config files.
+ */
+void snd_config_refresh(void)
+{
+	snd_config_lock();
+	if (snd_config_global_update)
+		snd_config_update_free(snd_config_global_update);
+	snd_config_global_update = NULL;
+	snd_config_unlock();
+}
+
+/**
+ * \brief Updates #snd_config and take its reference.
+ * \return 0 if #snd_config was up to date, 1 if #snd_config was
+ *         updated, otherwise a negative error code.
+ *
+ * Unlike #snd_config_update, this function increases a reference counter
+ * so that the obtained tree won't be deleted until unreferenced by
+ * #snd_config_put.
+ */
+int snd_config_update_get(snd_config_t **top)
+{
+	int err;
+
+	*top = NULL;
+	snd_config_lock();
+	err = snd_config_update_r(&snd_config, &snd_config_global_update, NULL);
+	if (err >= 0) {
+		*top = snd_config;
+		if (*top)
+			(*top)->refcount++;
+	}
+	snd_config_unlock();
+	return err;
+}
+
+/**
+ * \brief Unreference the config tree.
+ *
+ * Decreases a reference counter of the given config tree.  This is the
+ * counterpart of #snd_config_update_get.
+ */
+void snd_config_put(snd_config_t *cfg)
+{
+	snd_config_lock();
+	if (cfg)
+		snd_config_delete(cfg);
+	snd_config_unlock();
+}
+
 /** 
  * \brief Frees a private update structure.
  * \param[in] update The private update structure to free.
diff --git a/src/control/control.c b/src/control/control.c
index 8a5d530f2674..6078189a6a70 100644
--- a/src/control/control.c
+++ b/src/control/control.c
@@ -968,12 +968,16 @@ static int snd_ctl_open_noupdate(snd_ctl_t **ctlp, snd_config_t *root, const cha
  */
 int snd_ctl_open(snd_ctl_t **ctlp, const char *name, int mode)
 {
+	snd_config_t *top;
 	int err;
+
 	assert(ctlp && name);
-	err = snd_config_update();
+	err = snd_config_update_get(&top);
 	if (err < 0)
 		return err;
-	return snd_ctl_open_noupdate(ctlp, snd_config, name, mode);
+	err = snd_ctl_open_noupdate(ctlp, top, name, mode);
+	snd_config_put(top);
+	return err;
 }
 
 /**
diff --git a/src/hwdep/hwdep.c b/src/hwdep/hwdep.c
index 5dc791c99189..c8b368fa68a2 100644
--- a/src/hwdep/hwdep.c
+++ b/src/hwdep/hwdep.c
@@ -168,12 +168,16 @@ static int snd_hwdep_open_noupdate(snd_hwdep_t **hwdep, snd_config_t *root, cons
  */
 int snd_hwdep_open(snd_hwdep_t **hwdep, const char *name, int mode)
 {
+	snd_config_t *top;
 	int err;
+
 	assert(hwdep && name);
-	err = snd_config_update();
+	err = snd_config_update_get(&top);
 	if (err < 0)
 		return err;
-	return snd_hwdep_open_noupdate(hwdep, snd_config, name, mode);
+	err = snd_hwdep_open_noupdate(hwdep, top, name, mode);
+	snd_config_put(top);
+	return err;
 }
 
 /**
diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index 203e7a52491b..42da2c8a8b55 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -2288,12 +2288,16 @@ static int snd_pcm_open_noupdate(snd_pcm_t **pcmp, snd_config_t *root,
 int snd_pcm_open(snd_pcm_t **pcmp, const char *name, 
 		 snd_pcm_stream_t stream, int mode)
 {
+	snd_config_t *top;
 	int err;
+
 	assert(pcmp && name);
-	err = snd_config_update();
+	err = snd_config_update_get(&top);
 	if (err < 0)
 		return err;
-	return snd_pcm_open_noupdate(pcmp, snd_config, name, stream, mode, 0);
+	err = snd_pcm_open_noupdate(pcmp, top, name, stream, mode, 0);
+	snd_config_put(top);
+	return err;
 }
 
 /**
diff --git a/src/rawmidi/rawmidi.c b/src/rawmidi/rawmidi.c
index 0c89b8b984b9..d965edaf1f42 100644
--- a/src/rawmidi/rawmidi.c
+++ b/src/rawmidi/rawmidi.c
@@ -305,12 +305,16 @@ static int snd_rawmidi_open_noupdate(snd_rawmidi_t **inputp, snd_rawmidi_t **out
 int snd_rawmidi_open(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp,
 		     const char *name, int mode)
 {
+	snd_config_t *top;
 	int err;
+
 	assert((inputp || outputp) && name);
-	err = snd_config_update();
+	err = snd_config_update_get(&top);
 	if (err < 0)
 		return err;
-	return snd_rawmidi_open_noupdate(inputp, outputp, snd_config, name, mode);
+	err = snd_rawmidi_open_noupdate(inputp, outputp, top, name, mode);
+	snd_config_put(top);
+	return err;
 }
 
 /**
diff --git a/src/seq/seq.c b/src/seq/seq.c
index 4405e68a7fe9..c1aa7981d557 100644
--- a/src/seq/seq.c
+++ b/src/seq/seq.c
@@ -974,12 +974,16 @@ static int snd_seq_open_noupdate(snd_seq_t **seqp, snd_config_t *root,
 int snd_seq_open(snd_seq_t **seqp, const char *name, 
 		 int streams, int mode)
 {
+	snd_config_t *top;
 	int err;
+
 	assert(seqp && name);
-	err = snd_config_update();
+	err = snd_config_update_get(&top);
 	if (err < 0)
 		return err;
-	return snd_seq_open_noupdate(seqp, snd_config, name, streams, mode, 0);
+	err = snd_seq_open_noupdate(seqp, top, name, streams, mode, 0);
+	snd_config_put(top);
+	return err;
 }
 
 /**
diff --git a/src/timer/timer.c b/src/timer/timer.c
index a25e4f797ce4..6af6d8224df1 100644
--- a/src/timer/timer.c
+++ b/src/timer/timer.c
@@ -201,12 +201,16 @@ static int snd_timer_open_noupdate(snd_timer_t **timer, snd_config_t *root, cons
  */
 int snd_timer_open(snd_timer_t **timer, const char *name, int mode)
 {
+	snd_config_t *top;
 	int err;
+
 	assert(timer && name);
-	err = snd_config_update();
+	err = snd_config_update_get(&top);
 	if (err < 0)
 		return err;
-	return snd_timer_open_noupdate(timer, snd_config, name, mode);
+	err = snd_timer_open_noupdate(timer, top, name, mode);
+	snd_config_put(top);
+	return err;
 }
 
 /**
diff --git a/src/timer/timer_query.c b/src/timer/timer_query.c
index 93d2455d07fc..23bf0711aafe 100644
--- a/src/timer/timer_query.c
+++ b/src/timer/timer_query.c
@@ -159,12 +159,16 @@ static int snd_timer_query_open_noupdate(snd_timer_query_t **timer, snd_config_t
  */
 int snd_timer_query_open(snd_timer_query_t **timer, const char *name, int mode)
 {
+	snd_config_t *top;
 	int err;
+
 	assert(timer && name);
-	err = snd_config_update();
+	err = snd_config_update_get(&top);
 	if (err < 0)
 		return err;
-	return snd_timer_query_open_noupdate(timer, snd_config, name, mode);
+	err = snd_timer_query_open_noupdate(timer, top, name, mode);
+	snd_config_put(top);
+	return err;
 }
 
 /**

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

[Alexander E. Patrakov]

2016-05-13 20:04 GMT+05:00 Takashi Iwai <tiwai@suse.de>:
> On Fri, 13 May 2016 16:07:12 +0200,
> Takashi Iwai wrote:
>>
>> On Thu, 12 May 2016 15:45:23 +0200,
>> Takashi Iwai wrote:
>> >
>> > 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.
>>
>> So here is a quick hack.  It adds a simple refcount in snd_config_t.
>> snd_config_update_get() gives a top config with a refcount so that it
>> won't be deleted until disposed by snd_config_put().
>>
>> In this patch, the config update itself is kept in traditional way,
>> i.e. lazy update with mtime check.  For the forced refresh, I added
>> snd_config_refresh() which removes snd_config_global_update so that at
>> the next snd_config_update() will reread properly.  PA can call this
>> function appropriately, e.g. when hotplugged.
>>
>> The patch is just a proof of concept and even untested.  Once when
>> confirmed that it's the way to go, I'll cook up more.
>
> There was a typo indeed.  The revised patch is below.
> aplay worked at least :)  And now it has comments.

I will test it tomorrow.

However, now (it's 04:51 localtime, and I still can't sleep) I can't
understand whether snd_config_refresh() can be called safely at all
when something is playing to a hooks pcm with "ctl_elems" type (and
that use case is important for PulseAudio). Are the names of the
controls copied from the config with something like strdup (then it's
OK), or just point to a live structure that you are about to refresh?

P.S. The same objection, of course, applies to my original patch.

>
>
> Takashi
>
> ---
> diff --git a/include/conf.h b/include/conf.h
> index 087c05dc6bcf..72fd7c06a8d9 100644
> --- a/include/conf.h
> +++ b/include/conf.h
> @@ -94,6 +94,10 @@ int snd_config_update_r(snd_config_t **top, snd_config_update_t **update, const
>  int snd_config_update_free(snd_config_update_t *update);
>  int snd_config_update_free_global(void);
>
> +void snd_config_refresh(void);
> +int snd_config_update_get(snd_config_t **top);
> +void snd_config_put(snd_config_t *top);
> +
>  int snd_config_search(snd_config_t *config, const char *key,
>                       snd_config_t **result);
>  int snd_config_searchv(snd_config_t *config,
> diff --git a/src/conf.c b/src/conf.c
> index f8b7a6686529..1386a38b0b27 100644
> --- a/src/conf.c
> +++ b/src/conf.c
> @@ -434,6 +434,7 @@ static pthread_once_t snd_config_update_mutex_once = PTHREAD_ONCE_INIT;
>  struct _snd_config {
>         char *id;
>         snd_config_type_t type;
> +       int refcount; /* default = 0 */
>         union {
>                 long integer;
>                 long long integer64;
> @@ -1833,6 +1834,10 @@ int snd_config_remove(snd_config_t *config)
>  int snd_config_delete(snd_config_t *config)
>  {
>         assert(config);
> +       if (config->refcount > 0) {
> +               config->refcount--;
> +               return 0;
> +       }
>         switch (config->type) {
>         case SND_CONFIG_TYPE_COMPOUND:
>         {
> @@ -3851,6 +3856,60 @@ int snd_config_update(void)
>         return err;
>  }
>
> +/**
> + * \brief Clear the local config update cache
> + *
> + * This function clears the local config update cache to guarantee that
> + * the next call of #snd_config_update will reread the config files.
> + */
> +void snd_config_refresh(void)
> +{
> +       snd_config_lock();
> +       if (snd_config_global_update)
> +               snd_config_update_free(snd_config_global_update);
> +       snd_config_global_update = NULL;
> +       snd_config_unlock();
> +}
> +
> +/**
> + * \brief Updates #snd_config and take its reference.
> + * \return 0 if #snd_config was up to date, 1 if #snd_config was
> + *         updated, otherwise a negative error code.
> + *
> + * Unlike #snd_config_update, this function increases a reference counter
> + * so that the obtained tree won't be deleted until unreferenced by
> + * #snd_config_put.
> + */
> +int snd_config_update_get(snd_config_t **top)
> +{
> +       int err;
> +
> +       *top = NULL;
> +       snd_config_lock();
> +       err = snd_config_update_r(&snd_config, &snd_config_global_update, NULL);
> +       if (err >= 0) {
> +               *top = snd_config;
> +               if (*top)
> +                       (*top)->refcount++;
> +       }
> +       snd_config_unlock();
> +       return err;
> +}
> +
> +/**
> + * \brief Unreference the config tree.
> + *
> + * Decreases a reference counter of the given config tree.  This is the
> + * counterpart of #snd_config_update_get.
> + */
> +void snd_config_put(snd_config_t *cfg)
> +{
> +       snd_config_lock();
> +       if (cfg)
> +               snd_config_delete(cfg);
> +       snd_config_unlock();
> +}
> +
>  /**
>   * \brief Frees a private update structure.
>   * \param[in] update The private update structure to free.
> diff --git a/src/control/control.c b/src/control/control.c
> index 8a5d530f2674..6078189a6a70 100644
> --- a/src/control/control.c
> +++ b/src/control/control.c
> @@ -968,12 +968,16 @@ static int snd_ctl_open_noupdate(snd_ctl_t **ctlp, snd_config_t *root, const cha
>   */
>  int snd_ctl_open(snd_ctl_t **ctlp, const char *name, int mode)
>  {
> +       snd_config_t *top;
>         int err;
> +
>         assert(ctlp && name);
> -       err = snd_config_update();
> +       err = snd_config_update_get(&top);
>         if (err < 0)
>                 return err;
> -       return snd_ctl_open_noupdate(ctlp, snd_config, name, mode);
> +       err = snd_ctl_open_noupdate(ctlp, top, name, mode);
> +       snd_config_put(top);
> +       return err;
>  }
>
>  /**
> diff --git a/src/hwdep/hwdep.c b/src/hwdep/hwdep.c
> index 5dc791c99189..c8b368fa68a2 100644
> --- a/src/hwdep/hwdep.c
> +++ b/src/hwdep/hwdep.c
> @@ -168,12 +168,16 @@ static int snd_hwdep_open_noupdate(snd_hwdep_t **hwdep, snd_config_t *root, cons
>   */
>  int snd_hwdep_open(snd_hwdep_t **hwdep, const char *name, int mode)
>  {
> +       snd_config_t *top;
>         int err;
> +
>         assert(hwdep && name);
> -       err = snd_config_update();
> +       err = snd_config_update_get(&top);
>         if (err < 0)
>                 return err;
> -       return snd_hwdep_open_noupdate(hwdep, snd_config, name, mode);
> +       err = snd_hwdep_open_noupdate(hwdep, top, name, mode);
> +       snd_config_put(top);
> +       return err;
>  }
>
>  /**
> diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
> index 203e7a52491b..42da2c8a8b55 100644
> --- a/src/pcm/pcm.c
> +++ b/src/pcm/pcm.c
> @@ -2288,12 +2288,16 @@ static int snd_pcm_open_noupdate(snd_pcm_t **pcmp, snd_config_t *root,
>  int snd_pcm_open(snd_pcm_t **pcmp, const char *name,
>                  snd_pcm_stream_t stream, int mode)
>  {
> +       snd_config_t *top;
>         int err;
> +
>         assert(pcmp && name);
> -       err = snd_config_update();
> +       err = snd_config_update_get(&top);
>         if (err < 0)
>                 return err;
> -       return snd_pcm_open_noupdate(pcmp, snd_config, name, stream, mode, 0);
> +       err = snd_pcm_open_noupdate(pcmp, top, name, stream, mode, 0);
> +       snd_config_put(top);
> +       return err;
>  }
>
>  /**
> diff --git a/src/rawmidi/rawmidi.c b/src/rawmidi/rawmidi.c
> index 0c89b8b984b9..d965edaf1f42 100644
> --- a/src/rawmidi/rawmidi.c
> +++ b/src/rawmidi/rawmidi.c
> @@ -305,12 +305,16 @@ static int snd_rawmidi_open_noupdate(snd_rawmidi_t **inputp, snd_rawmidi_t **out
>  int snd_rawmidi_open(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp,
>                      const char *name, int mode)
>  {
> +       snd_config_t *top;
>         int err;
> +
>         assert((inputp || outputp) && name);
> -       err = snd_config_update();
> +       err = snd_config_update_get(&top);
>         if (err < 0)
>                 return err;
> -       return snd_rawmidi_open_noupdate(inputp, outputp, snd_config, name, mode);
> +       err = snd_rawmidi_open_noupdate(inputp, outputp, top, name, mode);
> +       snd_config_put(top);
> +       return err;
>  }
>
>  /**
> diff --git a/src/seq/seq.c b/src/seq/seq.c
> index 4405e68a7fe9..c1aa7981d557 100644
> --- a/src/seq/seq.c
> +++ b/src/seq/seq.c
> @@ -974,12 +974,16 @@ static int snd_seq_open_noupdate(snd_seq_t **seqp, snd_config_t *root,
>  int snd_seq_open(snd_seq_t **seqp, const char *name,
>                  int streams, int mode)
>  {
> +       snd_config_t *top;
>         int err;
> +
>         assert(seqp && name);
> -       err = snd_config_update();
> +       err = snd_config_update_get(&top);
>         if (err < 0)
>                 return err;
> -       return snd_seq_open_noupdate(seqp, snd_config, name, streams, mode, 0);
> +       err = snd_seq_open_noupdate(seqp, top, name, streams, mode, 0);
> +       snd_config_put(top);
> +       return err;
>  }
>
>  /**
> diff --git a/src/timer/timer.c b/src/timer/timer.c
> index a25e4f797ce4..6af6d8224df1 100644
> --- a/src/timer/timer.c
> +++ b/src/timer/timer.c
> @@ -201,12 +201,16 @@ static int snd_timer_open_noupdate(snd_timer_t **timer, snd_config_t *root, cons
>   */
>  int snd_timer_open(snd_timer_t **timer, const char *name, int mode)
>  {
> +       snd_config_t *top;
>         int err;
> +
>         assert(timer && name);
> -       err = snd_config_update();
> +       err = snd_config_update_get(&top);
>         if (err < 0)
>                 return err;
> -       return snd_timer_open_noupdate(timer, snd_config, name, mode);
> +       err = snd_timer_open_noupdate(timer, top, name, mode);
> +       snd_config_put(top);
> +       return err;
>  }
>
>  /**
> diff --git a/src/timer/timer_query.c b/src/timer/timer_query.c
> index 93d2455d07fc..23bf0711aafe 100644
> --- a/src/timer/timer_query.c
> +++ b/src/timer/timer_query.c
> @@ -159,12 +159,16 @@ static int snd_timer_query_open_noupdate(snd_timer_query_t **timer, snd_config_t
>   */
>  int snd_timer_query_open(snd_timer_query_t **timer, const char *name, int mode)
>  {
> +       snd_config_t *top;
>         int err;
> +
>         assert(timer && name);
> -       err = snd_config_update();
> +       err = snd_config_update_get(&top);
>         if (err < 0)
>                 return err;
> -       return snd_timer_query_open_noupdate(timer, snd_config, name, mode);
> +       err = snd_timer_query_open_noupdate(timer, top, name, mode);
> +       snd_config_put(top);
> +       return err;
>  }
>
>  /**



-- 
Alexander E. Patrakov

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

[Takashi Iwai]

On Sat, 14 May 2016 01:58:57 +0200,
Alexander E. Patrakov wrote:
> 
> 2016-05-13 20:04 GMT+05:00 Takashi Iwai <tiwai@suse.de>:
> > On Fri, 13 May 2016 16:07:12 +0200,
> > Takashi Iwai wrote:
> >>
> >> On Thu, 12 May 2016 15:45:23 +0200,
> >> Takashi Iwai wrote:
> >> >
> >> > 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.
> >>
> >> So here is a quick hack.  It adds a simple refcount in snd_config_t.
> >> snd_config_update_get() gives a top config with a refcount so that it
> >> won't be deleted until disposed by snd_config_put().
> >>
> >> In this patch, the config update itself is kept in traditional way,
> >> i.e. lazy update with mtime check.  For the forced refresh, I added
> >> snd_config_refresh() which removes snd_config_global_update so that at
> >> the next snd_config_update() will reread properly.  PA can call this
> >> function appropriately, e.g. when hotplugged.
> >>
> >> The patch is just a proof of concept and even untested.  Once when
> >> confirmed that it's the way to go, I'll cook up more.
> >
> > There was a typo indeed.  The revised patch is below.
> > aplay worked at least :)  And now it has comments.
> 
> I will test it tomorrow.
> 
> However, now (it's 04:51 localtime, and I still can't sleep) I can't
> understand whether snd_config_refresh() can be called safely at all
> when something is playing to a hooks pcm with "ctl_elems" type (and
> that use case is important for PulseAudio). Are the names of the
> controls copied from the config with something like strdup (then it's
> OK), or just point to a live structure that you are about to refresh?

IIRC, the config parser reads and builds the whole from the scratch at
each time.


Takashi

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

[Alexander E. Patrakov]

13.05.2016 20:04, Takashi Iwai пишет:
> On Fri, 13 May 2016 16:07:12 +0200,
> Takashi Iwai wrote:
>>
>> On Thu, 12 May 2016 15:45:23 +0200,
>> Takashi Iwai wrote:
>>>
>>> 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.
>>
>> So here is a quick hack.  It adds a simple refcount in snd_config_t.
>> snd_config_update_get() gives a top config with a refcount so that it
>> won't be deleted until disposed by snd_config_put().
>>
>> In this patch, the config update itself is kept in traditional way,
>> i.e. lazy update with mtime check.  For the forced refresh, I added
>> snd_config_refresh() which removes snd_config_global_update so that at
>> the next snd_config_update() will reread properly.  PA can call this
>> function appropriately, e.g. when hotplugged.
>>
>> The patch is just a proof of concept and even untested.  Once when
>> confirmed that it's the way to go, I'll cook up more.
>
> There was a typo indeed.  The revised patch is below.
> aplay worked at least :)  And now it has comments.

I have read the patch, and also tested with aplay, speaker-test and 
pulseaudio.

Here are my thoughts so far.

First, the patch combines two separate logical changes: proper locking 
and refcounting.

Locking looks almost OK (one small issue with snd_config_delete() and 
two "maybe" suggestions, see below). The mutex is recursive, so e.g. 
calling snd_ctl_open() from PCM hooks (and thus from inside 
snd_pcm_open()) is not a problem.

Regarding the new snd_config_refresh() API, do we really need it? There 
are precedents of applications (e.g. Phonon) using 
snd_config_update_free_global() for the same purpose. The only new thing 
that snd_config_refresh() brings is the guarantee that it either does 
not exist or does proper locking.

It may be, instead, a good idea to add the same kind of locking to 
snd_config_update_free_global(). And maybe to snd_config_update()? 
Ideally, I would like to be able to safely call 
snd_config_update_free_global() at any time in a multithreaded 
application that does not call any other snd_config_*() functions directly.

Now to refcounting.

The new functions, snd_config_update_get()/snd_config_put(), as a new 
public API, have received zero testing from me. With this patch, 
snd_config_delete() will be called sometimes on snd_config, but so far 
in my testing with aplay and speaker-test the relevant refcount is > 0. 
So it looks like there is no danger for existing applications.

As snd_config_delete() is still public API, it looks like there is a 
(bad) code path that accesses refcount without locking. Fixing it would 
require adding a lock to snd_config_delete(), but then what's the point 
of snd_config_put()?

I have also investigated the hooks PCM specifically for the question 
whether it is safe to refresh the config while such PCM is open, and 
found that, eventually, the control name is copied. This is done in 
snd_ctl_elem_id_set_name. So this particular PCM type seems to be safe.

Also, I have tried a patched pulseaudio. The patch adds a call to 
snd_config_refresh() in pa_alsa_open_by_device_string. It now interacts 
with a hot-plugged USB device successfully. But so it does if I call 
snd_config_update_free_global() (except for locking).

And I still think that I need to do come more testing...

-- 
Alexander E. Patrakov

>
>
> Takashi
>
> ---
> diff --git a/include/conf.h b/include/conf.h
> index 087c05dc6bcf..72fd7c06a8d9 100644
> --- a/include/conf.h
> +++ b/include/conf.h
> @@ -94,6 +94,10 @@ int snd_config_update_r(snd_config_t **top, snd_config_update_t **update, const
>  int snd_config_update_free(snd_config_update_t *update);
>  int snd_config_update_free_global(void);
>
> +void snd_config_refresh(void);
> +int snd_config_update_get(snd_config_t **top);
> +void snd_config_put(snd_config_t *top);
> +
>  int snd_config_search(snd_config_t *config, const char *key,
>  		      snd_config_t **result);
>  int snd_config_searchv(snd_config_t *config,
> diff --git a/src/conf.c b/src/conf.c
> index f8b7a6686529..1386a38b0b27 100644
> --- a/src/conf.c
> +++ b/src/conf.c
> @@ -434,6 +434,7 @@ static pthread_once_t snd_config_update_mutex_once = PTHREAD_ONCE_INIT;
>  struct _snd_config {
>  	char *id;
>  	snd_config_type_t type;
> +	int refcount; /* default = 0 */
>  	union {
>  		long integer;
>  		long long integer64;
> @@ -1833,6 +1834,10 @@ int snd_config_remove(snd_config_t *config)
>  int snd_config_delete(snd_config_t *config)
>  {
>  	assert(config);
> +	if (config->refcount > 0) {
> +		config->refcount--;
> +		return 0;
> +	}
>  	switch (config->type) {
>  	case SND_CONFIG_TYPE_COMPOUND:
>  	{
> @@ -3851,6 +3856,60 @@ int snd_config_update(void)
>  	return err;
>  }
>
> +/**
> + * \brief Clear the local config update cache
> + *
> + * This function clears the local config update cache to guarantee that
> + * the next call of #snd_config_update will reread the config files.
> + */
> +void snd_config_refresh(void)
> +{
> +	snd_config_lock();
> +	if (snd_config_global_update)
> +		snd_config_update_free(snd_config_global_update);
> +	snd_config_global_update = NULL;
> +	snd_config_unlock();
> +}
> +
> +/**
> + * \brief Updates #snd_config and take its reference.
> + * \return 0 if #snd_config was up to date, 1 if #snd_config was
> + *         updated, otherwise a negative error code.
> + *
> + * Unlike #snd_config_update, this function increases a reference counter
> + * so that the obtained tree won't be deleted until unreferenced by
> + * #snd_config_put.
> + */
> +int snd_config_update_get(snd_config_t **top)
> +{
> +	int err;
> +
> +	*top = NULL;
> +	snd_config_lock();
> +	err = snd_config_update_r(&snd_config, &snd_config_global_update, NULL);
> +	if (err >= 0) {
> +		*top = snd_config;
> +		if (*top)
> +			(*top)->refcount++;
> +	}
> +	snd_config_unlock();
> +	return err;
> +}
> +
> +/**
> + * \brief Unreference the config tree.
> + *
> + * Decreases a reference counter of the given config tree.  This is the
> + * counterpart of #snd_config_update_get.
> + */
> +void snd_config_put(snd_config_t *cfg)
> +{
> +	snd_config_lock();
> +	if (cfg)
> +		snd_config_delete(cfg);
> +	snd_config_unlock();
> +}
> +
>  /**
>   * \brief Frees a private update structure.
>   * \param[in] update The private update structure to free.
> diff --git a/src/control/control.c b/src/control/control.c
> index 8a5d530f2674..6078189a6a70 100644
> --- a/src/control/control.c
> +++ b/src/control/control.c
> @@ -968,12 +968,16 @@ static int snd_ctl_open_noupdate(snd_ctl_t **ctlp, snd_config_t *root, const cha
>   */
>  int snd_ctl_open(snd_ctl_t **ctlp, const char *name, int mode)
>  {
> +	snd_config_t *top;
>  	int err;
> +
>  	assert(ctlp && name);
> -	err = snd_config_update();
> +	err = snd_config_update_get(&top);
>  	if (err < 0)
>  		return err;
> -	return snd_ctl_open_noupdate(ctlp, snd_config, name, mode);
> +	err = snd_ctl_open_noupdate(ctlp, top, name, mode);
> +	snd_config_put(top);
> +	return err;
>  }
>
>  /**
> diff --git a/src/hwdep/hwdep.c b/src/hwdep/hwdep.c
> index 5dc791c99189..c8b368fa68a2 100644
> --- a/src/hwdep/hwdep.c
> +++ b/src/hwdep/hwdep.c
> @@ -168,12 +168,16 @@ static int snd_hwdep_open_noupdate(snd_hwdep_t **hwdep, snd_config_t *root, cons
>   */
>  int snd_hwdep_open(snd_hwdep_t **hwdep, const char *name, int mode)
>  {
> +	snd_config_t *top;
>  	int err;
> +
>  	assert(hwdep && name);
> -	err = snd_config_update();
> +	err = snd_config_update_get(&top);
>  	if (err < 0)
>  		return err;
> -	return snd_hwdep_open_noupdate(hwdep, snd_config, name, mode);
> +	err = snd_hwdep_open_noupdate(hwdep, top, name, mode);
> +	snd_config_put(top);
> +	return err;
>  }
>
>  /**
> diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
> index 203e7a52491b..42da2c8a8b55 100644
> --- a/src/pcm/pcm.c
> +++ b/src/pcm/pcm.c
> @@ -2288,12 +2288,16 @@ static int snd_pcm_open_noupdate(snd_pcm_t **pcmp, snd_config_t *root,
>  int snd_pcm_open(snd_pcm_t **pcmp, const char *name,
>  		 snd_pcm_stream_t stream, int mode)
>  {
> +	snd_config_t *top;
>  	int err;
> +
>  	assert(pcmp && name);
> -	err = snd_config_update();
> +	err = snd_config_update_get(&top);
>  	if (err < 0)
>  		return err;
> -	return snd_pcm_open_noupdate(pcmp, snd_config, name, stream, mode, 0);
> +	err = snd_pcm_open_noupdate(pcmp, top, name, stream, mode, 0);
> +	snd_config_put(top);
> +	return err;
>  }
>
>  /**
> diff --git a/src/rawmidi/rawmidi.c b/src/rawmidi/rawmidi.c
> index 0c89b8b984b9..d965edaf1f42 100644
> --- a/src/rawmidi/rawmidi.c
> +++ b/src/rawmidi/rawmidi.c
> @@ -305,12 +305,16 @@ static int snd_rawmidi_open_noupdate(snd_rawmidi_t **inputp, snd_rawmidi_t **out
>  int snd_rawmidi_open(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp,
>  		     const char *name, int mode)
>  {
> +	snd_config_t *top;
>  	int err;
> +
>  	assert((inputp || outputp) && name);
> -	err = snd_config_update();
> +	err = snd_config_update_get(&top);
>  	if (err < 0)
>  		return err;
> -	return snd_rawmidi_open_noupdate(inputp, outputp, snd_config, name, mode);
> +	err = snd_rawmidi_open_noupdate(inputp, outputp, top, name, mode);
> +	snd_config_put(top);
> +	return err;
>  }
>
>  /**
> diff --git a/src/seq/seq.c b/src/seq/seq.c
> index 4405e68a7fe9..c1aa7981d557 100644
> --- a/src/seq/seq.c
> +++ b/src/seq/seq.c
> @@ -974,12 +974,16 @@ static int snd_seq_open_noupdate(snd_seq_t **seqp, snd_config_t *root,
>  int snd_seq_open(snd_seq_t **seqp, const char *name,
>  		 int streams, int mode)
>  {
> +	snd_config_t *top;
>  	int err;
> +
>  	assert(seqp && name);
> -	err = snd_config_update();
> +	err = snd_config_update_get(&top);
>  	if (err < 0)
>  		return err;
> -	return snd_seq_open_noupdate(seqp, snd_config, name, streams, mode, 0);
> +	err = snd_seq_open_noupdate(seqp, top, name, streams, mode, 0);
> +	snd_config_put(top);
> +	return err;
>  }
>
>  /**
> diff --git a/src/timer/timer.c b/src/timer/timer.c
> index a25e4f797ce4..6af6d8224df1 100644
> --- a/src/timer/timer.c
> +++ b/src/timer/timer.c
> @@ -201,12 +201,16 @@ static int snd_timer_open_noupdate(snd_timer_t **timer, snd_config_t *root, cons
>   */
>  int snd_timer_open(snd_timer_t **timer, const char *name, int mode)
>  {
> +	snd_config_t *top;
>  	int err;
> +
>  	assert(timer && name);
> -	err = snd_config_update();
> +	err = snd_config_update_get(&top);
>  	if (err < 0)
>  		return err;
> -	return snd_timer_open_noupdate(timer, snd_config, name, mode);
> +	err = snd_timer_open_noupdate(timer, top, name, mode);
> +	snd_config_put(top);
> +	return err;
>  }
>
>  /**
> diff --git a/src/timer/timer_query.c b/src/timer/timer_query.c
> index 93d2455d07fc..23bf0711aafe 100644
> --- a/src/timer/timer_query.c
> +++ b/src/timer/timer_query.c
> @@ -159,12 +159,16 @@ static int snd_timer_query_open_noupdate(snd_timer_query_t **timer, snd_config_t
>   */
>  int snd_timer_query_open(snd_timer_query_t **timer, const char *name, int mode)
>  {
> +	snd_config_t *top;
>  	int err;
> +
>  	assert(timer && name);
> -	err = snd_config_update();
> +	err = snd_config_update_get(&top);
>  	if (err < 0)
>  		return err;
> -	return snd_timer_query_open_noupdate(timer, snd_config, name, mode);
> +	err = snd_timer_query_open_noupdate(timer, top, name, mode);
> +	snd_config_put(top);
> +	return err;
>  }
>
>  /**
>

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

On Sun, 15 May 2016 20:32:09 +0200,
Alexander E. Patrakov wrote:
> 
> 13.05.2016 20:04, Takashi Iwai пишет:
> > On Fri, 13 May 2016 16:07:12 +0200,
> > Takashi Iwai wrote:
> >>
> >> On Thu, 12 May 2016 15:45:23 +0200,
> >> Takashi Iwai wrote:
> >>>
> >>> 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.
> >>
> >> So here is a quick hack.  It adds a simple refcount in snd_config_t.
> >> snd_config_update_get() gives a top config with a refcount so that it
> >> won't be deleted until disposed by snd_config_put().
> >>
> >> In this patch, the config update itself is kept in traditional way,
> >> i.e. lazy update with mtime check.  For the forced refresh, I added
> >> snd_config_refresh() which removes snd_config_global_update so that at
> >> the next snd_config_update() will reread properly.  PA can call this
> >> function appropriately, e.g. when hotplugged.
> >>
> >> The patch is just a proof of concept and even untested.  Once when
> >> confirmed that it's the way to go, I'll cook up more.
> >
> > There was a typo indeed.  The revised patch is below.
> > aplay worked at least :)  And now it has comments.
> 
> I have read the patch, and also tested with aplay, speaker-test and
> pulseaudio.
> 
> Here are my thoughts so far.
> 
> First, the patch combines two separate logical changes: proper locking
> and refcounting.
> 
> Locking looks almost OK (one small issue with snd_config_delete() and
> two "maybe" suggestions, see below). The mutex is recursive, so
> e.g. calling snd_ctl_open() from PCM hooks (and thus from inside
> snd_pcm_open()) is not a problem.

There is nothing new in regard of locking.  It existed before the
patch, snd_config_update() and snd_config_update_free_global() already
took it.  So, this must be pretty safe even in the new code.

> Regarding the new snd_config_refresh() API, do we really need it?
> There are precedents of applications (e.g. Phonon) using
> snd_config_update_free_global() for the same purpose. The only new
> thing that snd_config_refresh() brings is the guarantee that it either
> does not exist or does proper locking.

OK, this can be removed.  I thought it would be nicer, but reducing
the unneeded API would be better indeed.

> It may be, instead, a good idea to add the same kind of locking to
> snd_config_update_free_global(). And maybe to snd_config_update()?
> Ideally, I would like to be able to safely call
> snd_config_update_free_global() at any time in a multithreaded
> application that does not call any other snd_config_*() functions
> directly.

As said, snd_config_update() and snd_config_update_free_global() have
already locks and they can be called safely.  The problem is that the
tree itself that is being used in snd_*_open() (i.e. snd_config global
tree) isn't race-free.  The introduction of refcount helps for that.

> Now to refcounting.
> 
> The new functions, snd_config_update_get()/snd_config_put(), as a new
> public API, have received zero testing from me. With this patch,
> snd_config_delete() will be called sometimes on snd_config, but so far
> in my testing with aplay and speaker-test the relevant refcount is >
> 0. So it looks like there is no danger for existing applications.
> 
> As snd_config_delete() is still public API, it looks like there is a
> (bad) code path that accesses refcount without locking. Fixing it
> would require adding a lock to snd_config_delete(), but then what's
> the point of snd_config_put()?

The difference is that snd_config_delete() can't be protected with
mutex, as this function itself is called recursively from the
destructor in the mutex.  So, we may see snd_config_delete() as a
function to delete the local tree while snd_config_put() (or maybe
better named as snd_config_unref() or such) is a function to
unreference (and eventually delete) the tree in a thread safe way.


Takashi

> I have also investigated the hooks PCM specifically for the question
> whether it is safe to refresh the config while such PCM is open, and
> found that, eventually, the control name is copied. This is done in
> snd_ctl_elem_id_set_name. So this particular PCM type seems to be
> safe.
> 
> Also, I have tried a patched pulseaudio. The patch adds a call to
> snd_config_refresh() in pa_alsa_open_by_device_string. It now
> interacts with a hot-plugged USB device successfully. But so it does
> if I call snd_config_update_free_global() (except for locking).
> 
> And I still think that I need to do come more testing...
> 
> -- 
> Alexander E. Patrakov
> 
> >
> >
> > Takashi
> >
> > ---
> > diff --git a/include/conf.h b/include/conf.h
> > index 087c05dc6bcf..72fd7c06a8d9 100644
> > --- a/include/conf.h
> > +++ b/include/conf.h
> > @@ -94,6 +94,10 @@ int snd_config_update_r(snd_config_t **top, snd_config_update_t **update, const
> >  int snd_config_update_free(snd_config_update_t *update);
> >  int snd_config_update_free_global(void);
> >
> > +void snd_config_refresh(void);
> > +int snd_config_update_get(snd_config_t **top);
> > +void snd_config_put(snd_config_t *top);
> > +
> >  int snd_config_search(snd_config_t *config, const char *key,
> >  		      snd_config_t **result);
> >  int snd_config_searchv(snd_config_t *config,
> > diff --git a/src/conf.c b/src/conf.c
> > index f8b7a6686529..1386a38b0b27 100644
> > --- a/src/conf.c
> > +++ b/src/conf.c
> > @@ -434,6 +434,7 @@ static pthread_once_t snd_config_update_mutex_once = PTHREAD_ONCE_INIT;
> >  struct _snd_config {
> >  	char *id;
> >  	snd_config_type_t type;
> > +	int refcount; /* default = 0 */
> >  	union {
> >  		long integer;
> >  		long long integer64;
> > @@ -1833,6 +1834,10 @@ int snd_config_remove(snd_config_t *config)
> >  int snd_config_delete(snd_config_t *config)
> >  {
> >  	assert(config);
> > +	if (config->refcount > 0) {
> > +		config->refcount--;
> > +		return 0;
> > +	}
> >  	switch (config->type) {
> >  	case SND_CONFIG_TYPE_COMPOUND:
> >  	{
> > @@ -3851,6 +3856,60 @@ int snd_config_update(void)
> >  	return err;
> >  }
> >
> > +/**
> > + * \brief Clear the local config update cache
> > + *
> > + * This function clears the local config update cache to guarantee that
> > + * the next call of #snd_config_update will reread the config files.
> > + */
> > +void snd_config_refresh(void)
> > +{
> > +	snd_config_lock();
> > +	if (snd_config_global_update)
> > +		snd_config_update_free(snd_config_global_update);
> > +	snd_config_global_update = NULL;
> > +	snd_config_unlock();
> > +}
> > +
> > +/**
> > + * \brief Updates #snd_config and take its reference.
> > + * \return 0 if #snd_config was up to date, 1 if #snd_config was
> > + *         updated, otherwise a negative error code.
> > + *
> > + * Unlike #snd_config_update, this function increases a reference counter
> > + * so that the obtained tree won't be deleted until unreferenced by
> > + * #snd_config_put.
> > + */
> > +int snd_config_update_get(snd_config_t **top)
> > +{
> > +	int err;
> > +
> > +	*top = NULL;
> > +	snd_config_lock();
> > +	err = snd_config_update_r(&snd_config, &snd_config_global_update, NULL);
> > +	if (err >= 0) {
> > +		*top = snd_config;
> > +		if (*top)
> > +			(*top)->refcount++;
> > +	}
> > +	snd_config_unlock();
> > +	return err;
> > +}
> > +
> > +/**
> > + * \brief Unreference the config tree.
> > + *
> > + * Decreases a reference counter of the given config tree.  This is the
> > + * counterpart of #snd_config_update_get.
> > + */
> > +void snd_config_put(snd_config_t *cfg)
> > +{
> > +	snd_config_lock();
> > +	if (cfg)
> > +		snd_config_delete(cfg);
> > +	snd_config_unlock();
> > +}
> > +
> >  /**
> >   * \brief Frees a private update structure.
> >   * \param[in] update The private update structure to free.
> > diff --git a/src/control/control.c b/src/control/control.c
> > index 8a5d530f2674..6078189a6a70 100644
> > --- a/src/control/control.c
> > +++ b/src/control/control.c
> > @@ -968,12 +968,16 @@ static int snd_ctl_open_noupdate(snd_ctl_t **ctlp, snd_config_t *root, const cha
> >   */
> >  int snd_ctl_open(snd_ctl_t **ctlp, const char *name, int mode)
> >  {
> > +	snd_config_t *top;
> >  	int err;
> > +
> >  	assert(ctlp && name);
> > -	err = snd_config_update();
> > +	err = snd_config_update_get(&top);
> >  	if (err < 0)
> >  		return err;
> > -	return snd_ctl_open_noupdate(ctlp, snd_config, name, mode);
> > +	err = snd_ctl_open_noupdate(ctlp, top, name, mode);
> > +	snd_config_put(top);
> > +	return err;
> >  }
> >
> >  /**
> > diff --git a/src/hwdep/hwdep.c b/src/hwdep/hwdep.c
> > index 5dc791c99189..c8b368fa68a2 100644
> > --- a/src/hwdep/hwdep.c
> > +++ b/src/hwdep/hwdep.c
> > @@ -168,12 +168,16 @@ static int snd_hwdep_open_noupdate(snd_hwdep_t **hwdep, snd_config_t *root, cons
> >   */
> >  int snd_hwdep_open(snd_hwdep_t **hwdep, const char *name, int mode)
> >  {
> > +	snd_config_t *top;
> >  	int err;
> > +
> >  	assert(hwdep && name);
> > -	err = snd_config_update();
> > +	err = snd_config_update_get(&top);
> >  	if (err < 0)
> >  		return err;
> > -	return snd_hwdep_open_noupdate(hwdep, snd_config, name, mode);
> > +	err = snd_hwdep_open_noupdate(hwdep, top, name, mode);
> > +	snd_config_put(top);
> > +	return err;
> >  }
> >
> >  /**
> > diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
> > index 203e7a52491b..42da2c8a8b55 100644
> > --- a/src/pcm/pcm.c
> > +++ b/src/pcm/pcm.c
> > @@ -2288,12 +2288,16 @@ static int snd_pcm_open_noupdate(snd_pcm_t **pcmp, snd_config_t *root,
> >  int snd_pcm_open(snd_pcm_t **pcmp, const char *name,
> >  		 snd_pcm_stream_t stream, int mode)
> >  {
> > +	snd_config_t *top;
> >  	int err;
> > +
> >  	assert(pcmp && name);
> > -	err = snd_config_update();
> > +	err = snd_config_update_get(&top);
> >  	if (err < 0)
> >  		return err;
> > -	return snd_pcm_open_noupdate(pcmp, snd_config, name, stream, mode, 0);
> > +	err = snd_pcm_open_noupdate(pcmp, top, name, stream, mode, 0);
> > +	snd_config_put(top);
> > +	return err;
> >  }
> >
> >  /**
> > diff --git a/src/rawmidi/rawmidi.c b/src/rawmidi/rawmidi.c
> > index 0c89b8b984b9..d965edaf1f42 100644
> > --- a/src/rawmidi/rawmidi.c
> > +++ b/src/rawmidi/rawmidi.c
> > @@ -305,12 +305,16 @@ static int snd_rawmidi_open_noupdate(snd_rawmidi_t **inputp, snd_rawmidi_t **out
> >  int snd_rawmidi_open(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp,
> >  		     const char *name, int mode)
> >  {
> > +	snd_config_t *top;
> >  	int err;
> > +
> >  	assert((inputp || outputp) && name);
> > -	err = snd_config_update();
> > +	err = snd_config_update_get(&top);
> >  	if (err < 0)
> >  		return err;
> > -	return snd_rawmidi_open_noupdate(inputp, outputp, snd_config, name, mode);
> > +	err = snd_rawmidi_open_noupdate(inputp, outputp, top, name, mode);
> > +	snd_config_put(top);
> > +	return err;
> >  }
> >
> >  /**
> > diff --git a/src/seq/seq.c b/src/seq/seq.c
> > index 4405e68a7fe9..c1aa7981d557 100644
> > --- a/src/seq/seq.c
> > +++ b/src/seq/seq.c
> > @@ -974,12 +974,16 @@ static int snd_seq_open_noupdate(snd_seq_t **seqp, snd_config_t *root,
> >  int snd_seq_open(snd_seq_t **seqp, const char *name,
> >  		 int streams, int mode)
> >  {
> > +	snd_config_t *top;
> >  	int err;
> > +
> >  	assert(seqp && name);
> > -	err = snd_config_update();
> > +	err = snd_config_update_get(&top);
> >  	if (err < 0)
> >  		return err;
> > -	return snd_seq_open_noupdate(seqp, snd_config, name, streams, mode, 0);
> > +	err = snd_seq_open_noupdate(seqp, top, name, streams, mode, 0);
> > +	snd_config_put(top);
> > +	return err;
> >  }
> >
> >  /**
> > diff --git a/src/timer/timer.c b/src/timer/timer.c
> > index a25e4f797ce4..6af6d8224df1 100644
> > --- a/src/timer/timer.c
> > +++ b/src/timer/timer.c
> > @@ -201,12 +201,16 @@ static int snd_timer_open_noupdate(snd_timer_t **timer, snd_config_t *root, cons
> >   */
> >  int snd_timer_open(snd_timer_t **timer, const char *name, int mode)
> >  {
> > +	snd_config_t *top;
> >  	int err;
> > +
> >  	assert(timer && name);
> > -	err = snd_config_update();
> > +	err = snd_config_update_get(&top);
> >  	if (err < 0)
> >  		return err;
> > -	return snd_timer_open_noupdate(timer, snd_config, name, mode);
> > +	err = snd_timer_open_noupdate(timer, top, name, mode);
> > +	snd_config_put(top);
> > +	return err;
> >  }
> >
> >  /**
> > diff --git a/src/timer/timer_query.c b/src/timer/timer_query.c
> > index 93d2455d07fc..23bf0711aafe 100644
> > --- a/src/timer/timer_query.c
> > +++ b/src/timer/timer_query.c
> > @@ -159,12 +159,16 @@ static int snd_timer_query_open_noupdate(snd_timer_query_t **timer, snd_config_t
> >   */
> >  int snd_timer_query_open(snd_timer_query_t **timer, const char *name, int mode)
> >  {
> > +	snd_config_t *top;
> >  	int err;
> > +
> >  	assert(timer && name);
> > -	err = snd_config_update();
> > +	err = snd_config_update_get(&top);
> >  	if (err < 0)
> >  		return err;
> > -	return snd_timer_query_open_noupdate(timer, snd_config, name, mode);
> > +	err = snd_timer_query_open_noupdate(timer, top, name, mode);
> > +	snd_config_put(top);
> > +	return err;
> >  }
> >
> >  /**
> >
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel