On Tue, 28 Nov 2017 07:33:24 +0100,
Kuninori Morimoto wrote:
>
>
> Hi Mark, Takashi-san
>
> I wonder are these patches acceptable ? or not ?
> If acceptable, should I resend ?
Since the use-case to pass NULL is limited, I'd rather avoid the
change. If there were more than handful users, maybe I would rethink,
but currently not convincing enough.
And, the NULL check is needed only when the caller is outside the
stream lock. Such a situation already smells like a buggy code.
We don't need to make it easier :)
thanks,
Takashi
> > 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
