Re: [libvirt] [PATCH] don't test "res == NULL" after we've already dereferenced it

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.

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.

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&gt; 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&gt; 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