On Sat, 14 May 2016 01:58:57 +0200, Alexander E. Patrakov wrote: > > 2016-05-13 20:04 GMT+05:00 Takashi Iwai <tiwai@xxxxxxx>: > > 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 _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
