On 06/28/2013 10:19 AM, Tanu Kaskinen wrote:
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 <[email protected]>
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.
First, consider the probability of an average user to a) actually file a
bug or b) doing something else, like turning PulseAudio off, going back
to Windows 8, or just become annoyed.
Second, consider the probability that the bug actually reaches us, which
is somewhat depending on which bug tracker to use (our bugtracker we
look at, but Ubuntu's bug tracker has far too many errors).
Third, consider the probability that the assertion failure actually
helps us fix the bug, and we wouldn't be able to resolve it without it.
In many cases, a stack trace is not enough to resolve the problem.
Multiply all probabilities and you'll end up with something very low.
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?
Good question. I don't know git enough to look it through and come with
statistics. My *guess* is that it is more than half but it could very
well be wrong.
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.
Here's another one: "alsa-mixer: It's valid to have zero elements in a path"
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,
But the opposite is also true: assertion lowers software's quality,
because they crash users' machines.
There might be a grand vision that some day, people will report all
assertions as they come by, and we will spend time fixing them, and we
end up with the best quality, but this is unrealistic utopia: It takes
several months to get PA into the distros, then to the users, then
feedback to us. Meanwhile we're working on a new PA release, and every
new release of PA, we add new features and new assertions. As a result,
that best quality release will never happen.
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.
The problem is in your reasoning, not in the code: "some situations"
does not have to be something actually occurring right now, it can also
be a theoretical situation happening some time in the future. Like in
the alsa-mixer patch referenced above: when the assertion was added, it
was probably valid, but then circumstances changed, and suddenly the
assertion was the cause of a bug.
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
_______________________________________________
pulseaudio-discuss mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/pulseaudio-discuss
