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

[Thomas Martitz]

Am 28.06.2013 10:19, schrieb Tanu Kaskinen:
> On Fri, 2013-06-28 at 09:47 +0200, David Henningsson wrote:
>> 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.
> If (s)he file a bug, it may be hard to track down, because the error
> wasn't caught early enough. If it's hard to track down, the bug may
> never be resolved.
>
>> 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;"
> Of the assertions failures that you have had time to fix, how many have
> been cases where the fix has been to replace the assertion with proper
> error handling? I can remember one: the UCM crash when the configuration
> didn't have channel count set. That was incorrect assertion use, because
> the assertion trapped an error in configuration, not code.
>

I agree, assertions and error handling separate beasts. Assertions 
detect programming errors and make them easy to find (and fix). Crashing 
with backtrace helps that. Error handling should handle user input (or 
other runtime environment scenarios) gracefully so it doesn't crash. 
Crashing with backtrace doesn't help here because the error is not a bug 
and cannot be easily fixed.

So the question is if it's considered a programming error if 
pa_alsa_path_set_add_ports() is called with a NULL parameter, i.e. if 
all call sites are to be expected to not pass NULL.

If yes, then the assertion is right even if unconvinient for the user. 
Hiding programming errors in order to please the user is a recipe for 
failure. If no, the change should be reverted. But you have to make sure 
the call sites can handle the case where this function returns early and 
does nothing.

Best regards.