[pulseaudio-discuss] [pulseaudio-commits] src/modules
tanu.kaskinen at linux.intel.com
Fri Jun 28 01:19:26 PDT 2013
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  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
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.
More information about the pulseaudio-discuss