5:13 a.m.
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(a)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(a)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