Daniel P. Berrange wrote:
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
I agree and have looked at the code in question (the somewhat duplicated if/else blocks in proxy_internal.c lines 385-443) http://libvirt.org/git/?p=libvirt.git;a=blob;f=src/xen/proxy_internal.c#l385 and don't see off hand why these if-blocks differ: res = request; if (res->len != sizeof(virProxyPacket)) { virProxyError(conn, VIR_ERR_INTERNAL_ERROR, _("Communication error with proxy: expected %d bytes got %d\n"), (int) sizeof(virProxyPacket), res->len); goto error; } res = (virProxyPacketPtr) answer; if ((res->len < sizeof(virProxyPacket)) || (res->len > sizeof(virProxyFullPacket))) { virProxyError(conn, VIR_ERR_INTERNAL_ERROR, _("Communication error with proxy: got %d bytes packet\n"), res->len); goto error; }