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; } -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list