Hi Takashi,
On 05.12.2017 06:50, Takashi Sakamoto wrote:
> Hi,
>
> On Nov 30 2017 07:17, Maciej S. Szmigiero wrote:
(..)
>> diff --git a/src/pcm/pcm_linear.c b/src/pcm/pcm_linear.c
>> index 0d61e927323f..e4973a308004 100644
>> --- a/src/pcm/pcm_linear.c
>> +++ b/src/pcm/pcm_linear.c
>> @@ -90,6 +90,7 @@ int snd_pcm_linear_get_index(snd_pcm_format_t src_format, snd_pcm_format_t dst_f
>> endian = 0;
>> pwidth = snd_pcm_format_physical_width(src_format);
>> width = snd_pcm_format_width(src_format);
>> + assert(width >= 8 && width <= 32);
>>
>> ...
>> > @@ -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.
> (Additionally, I suspect that current implementation of the resampling
> layer itself is not enough proper for non-PCM samples such as float, DSD
> and so on.)
>
> And next time, please follow a below instruction.
> 1.use '--subject-prefix' option with 'alsa-lib][PATCH v3' for
> git-format-patch so that subject of your patches includes better
> information for reviewers.
> (If it's for kernel stuffs, just use '--v')
Hmm, but this series already had this prefix ('[alsa-lib][PATCH]')?
Had no version number, since it was the first iteration.
> 2. use '--cover-letter' option for git-format-patch so that you can
> write changelog of your successive work.
Will do.
> 3.use '--thread' option for git-send-email (or add 'sendemail.thread'
> entry into your local repository) so that posted patches spin in one
> message thread.
>
Will do.
Also, will add commas at the ends of last entries in enums, as you had
suggested in your other message.
> Thanks
>
> Takashi Sakamoto
Thanks,
Maciej
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
