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