On 06/28/2013 04:20 PM, Tanu Kaskinen wrote: > On Fri, 2013-06-28 at 11:01 +0200, David Henningsson wrote: >> 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 <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. >> >> 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). > > Ubuntu has an automatic crash report tool, doesn't it? So the > probability is quite high that assertion failures go somewhere. > Currently they don't reach our bug tracker, but perhaps that could be > arranged? Or I could subscribe to the crash reports myself? Just to give a rough measure, if you take the topmost assertion on errors.ubuntu.com's list in PulseAudio, that's bug 981149 [1]. It has 8 "numbers of users affected" in Launchpad, and 2000 instances reported anonymously last year on errors.ubuntu.com. That's less than 1% probability for a) in the first probability check, and then not everyone chooses to send a bug report to errors.ubuntu.com either. For many other crashers, the probability is 0%, i e, nobody has even bothered filing a Launchpad bug for it. You can subscribe to launchpad bugs for PulseAudio if you like, all bug fixes are welcome. You can also check manually, at regular intervals, how things are looking at errors.ubuntu.com (let me know if you have trouble accessing it and I'll try to look up how to get access there). >> 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. > > A stack trace helps pretty much always, though. The point was not that > we wouldn't be able to fix bugs without the stack trace if we put enough > effort in it, the point was that if debugging a bug is too difficult, we > may easily move on to other things and the bug is left abandoned in > bugzilla. A stack trace is not a guarantee of a fix, but it does improve > the chances a lot. My milage varies - stack traces often help, but I wouldn't say "pretty much always". I also find that often things are more complicated than can be figured out by just looking at the stack trace. Btw, in best case, the assertion catches what would otherwise have been a SIGSEGV. And SIGSEGVs also comes with stack traces. >> 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. > > My subjective experience is very different, but I don't have any stats either. > >>> 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" > > Thanks for digging that up. > >>>> 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. > > I don't have such grand vision. Features and refactoring add bugs, bug > fixing removes them. Until we reach feature completeness, I fully expect > PulseAudio to have a fair number of bugs. Assertions (documented > assumptions) increase maintainability, which speeds up development and > reduces, I believe, the total number of bugs, but assertions also make > the some of the bugs more annoying to the user.Whether the benefits of > higher maintainability or the benefit of less frequent crashing is more > useful for users is something that I don't think we will ever agree on. > There's no way of proving either case. For me, assertions often *decrease* maintainability. The alsa-mixer is a typical example: it would have been more maintainable to handle the case of zero elements in the first place. There are certainly cases where "documented assumptions" can increase maintainability too, but I wouldn't agree that on an overall the maintainability is increased by them. For me, I've never thought much about it, but I think it's probably 50/50 overall - i e, it makes not much of a difference either way in terms of maintainability. But if I can't convince you that less frequent crashing is better, which is quite obvious to me, could you at least refrain from converting proper checks to assertions? >>> 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. > > Yes, changing the code can break old assumptions, and it's sometimes > very hard to be aware of those assumptions when doing changes, as was > the case with the alsa-mixer bug. > > Now, back to the code that I changed: there are three parameters to > pa_alsa_path_set_add_ports() that are never NULL: ps, ports and core. > Why is it that you consider ps to be more likely to some day be NULL > than the two others, warranting the special handling? I don't remember exactly what I thought when I wrote that piece of code, but I think it makes sense: ps is a source of information, where a null object is most likely interpreted as "nothing to add". ports is the destination, so having source without destination...I don't know what that would mean. Hence it made more sense to check ps and assert ports. core I didn't care about (that's what you added), but is extremely unlikely to go away any time soon. -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic [1] https://bugs.launchpad.net/ubuntu/+source/pulseaudio/+bug/981149
