Hi Mark, Takashi-san
I wonder are these patches acceptable ? or not ?
If acceptable, should I resend ?
> Takashi Sakamoto wrote:
> >
> > On Nov 9 2017 11:11, Kuninori Morimoto wrote:
> > >
> > > Hi Takashi-san, Mark
> > >
> > > snd_pcm_running() is using "substream" and "substream->runtime"
> > > pointer, no check.
> > > These patches adds its check in function,
> > > and removes duplicate checks from each drivers.
> > >
> > > Not super important, but can be cleanup
> > >
> > > Kuninori Morimoto (6):
> > > ALSA: pcm: check parameter on snd_pcm_running()
> > > ALSA: pdaudiocf: remove unneeded check for snd_pcm_running()
> > > ASoC: dwc: remove unneeded check for snd_pcm_running()
> > > ASoC: omap-hdmi-audio: remove unneeded check for snd_pcm_running()
> > > ASoC: xtfpga-i2s: remove unneeded check for snd_pcm_running()
> > > ASoC: rsnd: remove unneeded check for snd_pcm_running()
> > >
> > > include/sound/pcm.h | 3 +++
> > > sound/pcmcia/pdaudiocf/pdaudiocf_irq.c | 2 +-
> > > sound/soc/dwc/dwc-pcm.c | 2 +-
> > > sound/soc/omap/omap-hdmi-audio.c | 3 +--
> > > sound/soc/sh/rcar/core.c | 5 +----
> > > sound/soc/xtensa/xtfpga-i2s.c | 4 ++--
> > > 6 files changed, 9 insertions(+), 10 deletions(-)
> >
> > This is a bad direction. I exactly oppose to your idea.
> >
> > > include/sound/pcm.h | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> > > index 24febf9..a8e49f5 100644
> > > --- a/include/sound/pcm.h
> > > +++ b/include/sound/pcm.h
> > > @@ -664,6 +664,9 @@ void snd_pcm_stream_unlock_irqrestore(struct
> > snd_pcm_substream *substream,
> > > */
> > > static inline int snd_pcm_running(struct snd_pcm_substream *substream)
> > > {
> > > + if (!substream || !substream->runtime)
> > > + return 0;
> > > +
> > > return (substream->runtime->status->state ==
> > SNDRV_PCM_STATE_RUNNING ||
> > > (substream->runtime->status->state ==
> > SNDRV_PCM_STATE_DRAINING &&
> > > substream->stream == SNDRV_PCM_STREAM_PLAYBACK));
> >
> > In a view of 'design by contract', this function has a pre-condition
> > that a given argument should not be NULL. Callers _should_ guarantee
> > it to keep semantics of this function.
>
> Generally I agree, but note that it depends on the exposure of the API
> function itself. If the API function is supposed to be an interface
> directly communicated with the outside, it's not seldom to allow NULL
> there. An implicit NULL-check would be handy and often makes coding
> easier (see the case of free()).
>
> > Your idea appends the duty of callers to this function. This causes a
> > semantical contradiction. If it were something to bring kernel
> > corruption such as BUG_ON(), the original design would be kept. When
> > substream is NULL, it's a bug of drivers in adding PCM
> > components. When runtime is NULL, it's a bug of ALSA PCM core in
> > handling open system call.
>
> When you call snd_pcm_running(), basically you're evaluating the PCM
> stream status, and likely a state machine. It often assumes that PCM
> state is consistent during the following action, and it implies the
> PCM stream lock was acquired. And, of course, PCM stream lock
> requires the non-NULL substream.
>
> That said, if the code has a proper protection for the PCM stream
> consistency, the substream NULL check had to be done far before that
> point due to a stream lock.
>
> Though, most codes aren't super-critical about the state change and
> the direct snd_pcm_running() works in most cases. But in the perfect
> world, stream locking is preferred around the state evaluation and the
> action according to it.
>
>
> thanks,
>
> Takashi
Best regards
---
Kuninori Morimoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
