Re: [libvirt] [PATCH] don't test "res == NULL" after we've already dereferenced it

On Thu, Jan 07, 2010 at 11:03:23AM +0100, Daniel Veillard wrote: > On Thu, Jan 07, 2010 at 09:37:57AM +0100, Jim Meyering wrote: > > > > However, the point is still valid, so I'll wait for confirmation. > > This is still about defensive coding, i.e., ensuring that > > maintenance doesn't violate invariants in harder-to-diagnose ways. > > If you get a bug report, which would you rather hear? > > "libvirt sometimes segfaults" or > > "I get an assertion failure on file.c:999" > > I guess it's mostly a matter of coding style, I'm not sure it makes > such a difference in practice, libvirt output will likely be burried > in a log file, unless virsh or similar CLI tool is used. > We have only 4 asserts in the code currently, I think it shows that > so far we are not relying on it. On one hand this mean calling exit() > and I prefer a good old core dump for debugging than a one line message, > on the other hand if you managed to catch that message, well this can > help pinpoint if the person reporting has no debugging skills. > > I think there is pros and cons and that the current status quo is > that we don't use asserts but more defensing coding by erroring out > when this happen. The best way is not to assert() when we may > dereference a NULL pointer but check for NULL at the point where > we get that pointer, that's closer to the current code practice of > libvirt (or well I expect so :-) and allow useful reporting (we > failed to do a given operation) and a graceful error handling without > exit'ing. So in general if we think we need an assert somewhere that > mean that we failed to do the check at the right place, and I would > rather not start to get into the practice of just asserting in a zillion > place instead of checking at the correct place. > > So in my opinion, I'm still not fond of assert(), and I would prefer > to catch up problem earlier, like parameter checking on function entry > points or checking return value for functions producing pointers. The other reason for adding assert(), is if the code leading upto a particular point is too complex to reliably understand whether a variable is NULL. I think that applies in this case. I don't think adding an assert() is the right way to deal with that complexity though. I think it is better to make the code clearer/easier to follow & understand