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
