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

[Takashi Iwai <tiwai@suse.de>]

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