[pulseaudio-discuss] [pulseaudio-commits] src/modules

[David Henningsson]

On 06/28/2013 04:40 AM, Tanu Kaskinen wrote:
> On Thu, 2013-06-27 at 21:57 +0200, David Henningsson wrote:
>> On 06/27/2013 06:19 PM, Tanu Kaskinen wrote:
>>>    src/modules/alsa/alsa-mixer.c |    5 ++---
>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> New commits:
>>> commit 2613e4c74733e67d56af165df4637bf902b08508
>>> Author: Tanu Kaskinen <tanu.kaskinen at linux.intel.com>
>>> Date:   Thu Jun 27 18:47:12 2013 +0300
>>>
>>>       alsa-mixer: Add a couple of assertions
>>>
>>>       I checked the code to ensure that the assertions hold currently.
>>>
>>> diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
>>> index f4410d7..b2f6c2e 100644
>>> --- a/src/modules/alsa/alsa-mixer.c
>>> +++ b/src/modules/alsa/alsa-mixer.c
>>> @@ -4530,10 +4530,9 @@ void pa_alsa_path_set_add_ports(
>>>        pa_alsa_path *path;
>>>        void *state;
>>>
>>> +    pa_assert(ps);
>>>        pa_assert(ports);
>>> -
>>> -    if (!ps)
>>> -        return;
>>
>> Spontaneous NAK for the above change.
>>
>> I like the code the way I wrote it. Please explain.
>
> Why would you ever pass NULL path set? The whole point of the function
> is to generate ports from a path set.

For practical and robustness reasons, just like free/pa_xfree accept 
NULL pointers.

But the question should not be "why would you ever...", but "if you ever 
do, what would you likely want to happen?"

This opens up for code reuse, but more importantly, see this from a user 
perspective. We release PulseAudio to millions of users. Some of these 
have unusual hardware or software for which we can't or don't test here. 
Do you think that user wants his PulseAudio to crash, or do you think 
that the user wants it to work as good as it can?

The user *might* be satisfied with having it working as good as it can, 
if not, (s)he will file a bug. (S)he will definitely not be satisfied 
with having PulseAudio crashing.

I'm having *a lot* of different assertion failures [1] in Ubuntu, more 
than I have time to fix, and it might not even be the best use of my 
time to fix them. And I'm not saying all of these assertions should be 
just removed, but certainly many of them would benefit from someone 
thinking them through and replacing them with proper error handling 
code. Which often are as simple as "if (a == NULL) return;"

Assertions are a double edged sword - they are both helpful for 
developers, and something crashing our users' machines. Could it be that 
you mostly see this from the developer's side, and missing the user 
perspective?


-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic

[1] https://errors.ubuntu.com/?package=pulseaudio&period=month - not 
sure if this URL is accessible to everyone though