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