Daniel Veillard 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()
Is "don't use assert" libvirt policy? I hope not.
and I prefer a good old core dump for debugging than a one line message,
Same here, but there are many reasons for which a reporter will be unwilling or unable to provide a core dump. No one hesitates to include the output of a failed assertion in a bug report.
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
No. That is not how one should use assert, and certainly not how I proposed to use it. This is not about testing for a user- or system-provokable condition, but rather about testing code invariants. See below.
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.
In that case, res being NULL means that both answer and request parameters are null after the retry: label, since the two pointers are not modified in the function this should be tested when entering the function,
if ((answer == NULL) && (request == NULL)) { virProxyError (NULL, VIR_ERR_INVALID_ARG, __FUNCTION__); return -1; }
you get the error location, as in the assert but propagate the error back in the stack cleanly instead of exit'ing
It is important to distinguish two types of errors: - conditions that may arise due to user input or environment (misbehaving client or server, malformed packet, I/O error, etc.) - internal API abuse, violation of an internal assumption or invariant Using "assert" is appropriate only in the latter case, for detecting conditions that typically are provoked only by developer-introduced errors. Adding error-handling code like the above is appropriate only in the first case. Somewhat along these lines, we are starting to remove certain types of tests, (e.g., conn == NULL), when "conn" is always non-NULL. In the case of this patch, "request" is always non-null, and should probably have the "nonnull" attribute. New patch appended below. One advantage of using assert is that it usually _reduces_ the maintenance burden (i.e., when skimming the code, you may ignore the assert statement). However, if you add expressions like the above to "ensure" a condition that will always be true (i.e., that should not be possible to trigger via user input), you *increase* the maintenance burden by adding a test of an always-false condition that is handled as if it *can* sometimes be true. That would increase the WTF/m rate for certain readers. (http://www.osnews.com/story/19266/WTFs_m) Such a test might even trigger new warnings from tools like clang and coverity. Whether we use an assertion in this particular case is not important, but I hope that libvirt adopts a judicious policy on the use of "assert". Here's the revised patch, followed by the tiny nonnull-one. (also fixes typos s/ret/res/ in log message)
From 2a7fed9238667ff75a6ecd662b663549bc8fb7b5 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Wed, 6 Jan 2010 12:45:07 +0100 Subject: [PATCH] don't test "res == NULL" after we've already dereferenced "res"
* src/xen/proxy_internal.c (xenProxyCommand): "res" is known to be non-NULL at that point, so remove the "res == NULL" guard. --- src/xen/proxy_internal.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index ec4522b..ee4678a 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -1,7 +1,7 @@ /* * proxy_client.c: client side of the communication with the libvirt proxy. * - * Copyright (C) 2006, 2008, 2009 Red Hat, Inc. + * Copyright (C) 2006, 2008, 2009, 2010 Red Hat, Inc. * * See COPYING.LIB for the License of this software * @@ -444,7 +444,7 @@ retry: /* * do more checks on the incoming packet. */ - if ((res == NULL) || (res->version != PROXY_PROTO_VERSION) || + if ((res->version != PROXY_PROTO_VERSION) || (res->len < sizeof(virProxyPacket))) { virProxyError(conn, VIR_ERR_INTERNAL_ERROR, "%s", _("Communication error with proxy: malformed packet\n")); -- 1.6.6.425.g4dc2d
From eb6f7f0478ce510de8dc5fc9add4eaaca1c2bfc1 Mon Sep 17 00:00:00 2001 From: Jim Meyering <meyering@redhat.com> Date: Thu, 7 Jan 2010 19:48:05 +0100 Subject: [PATCH] proxy_internal.c: mark "request" parameter as nonnull
* src/xen/proxy_internal.c (xenProxyCommand): Mark "request" as an always-non-NULL parameter. --- src/xen/proxy_internal.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c index ee4678a..73a0469 100644 --- a/src/xen/proxy_internal.c +++ b/src/xen/proxy_internal.c @@ -340,7 +340,7 @@ xenProxyClose(virConnectPtr conn) return 0; } -static int +static int ATTRIBUTE_NONNULL(2) xenProxyCommand(virConnectPtr conn, virProxyPacketPtr request, virProxyFullPacketPtr answer, int quiet) { static int serial = 0; -- 1.6.6.425.g4dc2d