[alsa-devel] [PATCH 0/6] ALSA: pcm: check parameter on snd_pcm_running()

[Takashi Sakamoto]

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.

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.


Regards

Takashi Sakamoto