On Tue, 05 Dec 2017 15:37:09 +0100,
Takashi Sakamoto wrote:
>
> Hi,
>
> On Dec 5 2017 22:22, Maciej S. Szmigiero wrote:
> >>>> @@ -121,6 +125,7 @@ int snd_pcm_linear_put_index(snd_pcm_format_t
> >> src_format, snd_pcm_format_t dst_f
> >>> endian = 0;
> >>> pwidth = snd_pcm_format_physical_width(dst_format);
> >>> width = snd_pcm_format_width(dst_format);
> >>> + assert(width >= 8 && width <= 32);
> >>> if (pwidth == 24) {
> >>> switch (width) {
> >>> case 24:
> >>
> >> I can understand your intention to insert these assert(), while it's better not to include them in this series of patch.
> >>
> >> A call of 'snd_pcm_format_physical_width()' and 'snd_pcm_format_width()'
> >> returns 4 for 'SNDRV_PCM_FORMAT_IMA_ADPCM'. Furthermore, both of them
> >> returns -EINVAL for 'SNDRV_PCM_FORMAT_MPEG', 'SNDRV_PCM_FORMAT_GSM' and
> >> 'SNDRV_PCM_FORMAT_SPECIAL'. They can bring wrong calculation to the
> >> resampling function, in the worse case result in any fault.
> >
> > snd_pcm_linear_get_index() and snd_pcm_linear_put_index() should
> > be called for linear formats only (like their names suggest).
> >
> > If they were called for 'SNDRV_PCM_FORMAT_IMA_ADPCM' this will result
> > in a negative conversion arrays index.
> > The same goes for formats that return '-EINVAL' as their width.
> >
> > This will mean that an application will try to access these arrays
> > outside of their bounds, which may produce a hard to find bugs.
> > I think it is really better to fail with an assertion failure in such
> > cases.
> >
> >> However, this is another issue, at least out of your aim for this
> >> patchset. It's a kind of bug for wider influence. I think to have
> >> another opportunity to discuss about the way to handle such special
> >> formats in the resampling layer. Would you please post this patchset
> >> again without these assertions?
> >
> > If these assertions are really controversial I will remove it, but please
> > think again about it in the light of the paragraph above.
> > Asserts can also be split out to a separate patch.
>
> Your have correct understanding of this issue. However, it's better for
> us to achieve your aim (addition of the new pcm formats) at first. Then
> we start discussion about better solution to handle the non-Linear
> formats.
>
> In my opinion, addition of constraint of PCM hardware parameters to
> these PCM plugins is preferable to insertion of the assertions,
> because abort inner library is bad idea itself. Proper errors should
> be returned
> to applications in protocol via alsa-lib PCM APIs. It's more friendly to
> usual developers and users.
Well, it's basically moot to discuss about it, as
snd_pcm_lib_linear_get_index() is an internal function, i.e. it's not
supposed to be called from applications.
In that sense, assert() wouldn't be too bad. However, the primary
question is -- would it really catch anything in practice? What drove
you to put that assert() at all?
If a check of the valid formats, then calling snd_pcm_format_linear()
is the safest.
thanks,
Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
