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

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

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

As a user, I want my software to have good quality. Assertions help with
that, because they improve the code maintainability, and thus can
prevent bugs from ever occurring. And if I encounter bugs, I prefer to
be able to provide a stack trace (preferably the stack trace would be
sent automatically to the developers).

In case it's not clear how this particular assertion helps
maintainability: when I read the code, and I saw "if (!ps)", it told me
that the path set can be NULL in some situations. It didn't match with
my understanding of the purpose of the function, so I started to suspect
that my understanding is wrong. Well, it turned out that my
understanding was not wrong, and I wasted time figuring out how
pa_alsa_path_set_add_ports() is used.

-- 
Tanu