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.
(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.
This patchset[1] should have v2, because I did comment to your first
version[2]. So the next version should have v3.
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.
OK. Thanks.
[1]
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/128335.html
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/128336.html
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/128337.html
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/128338.html
[2]
http://mailman.alsa-project.org/pipermail/alsa-devel/2017-November/128278.html
Takashi Sakamoto
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
