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.
- 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