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

Daniel Veillard wrote: > On Wed, Jan 06, 2010 at 12:46:11PM +0100, Jim Meyering wrote: >> As the log says, once we've dereferenced it, >> there's no point in comparing to NULL. >> >> >From 463eaf1027a154e71839a67eca85b3ada8b817ff 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 it >> >> * src/xen/proxy_internal.c (xenProxyCommand): It is known to be >> non-NULL at that point, so remove the "ret == NULL" guard, and >> instead add an assert(ret != NULL), in case future code changes >> cause the condition to becomes false. >> Include <assert.h>. >> --- >> src/xen/proxy_internal.c | 6 ++++-- >> 1 files changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/src/xen/proxy_internal.c b/src/xen/proxy_internal.c >> index ec4522b..42b6e91 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 >> * >> @@ -11,6 +11,7 @@ >> #include <config.h> >> >> #include <stdio.h> >> +#include <assert.h> >> #include <stdlib.h> >> #include <unistd.h> >> #include <errno.h> >> @@ -444,7 +445,8 @@ retry: >> /* >> * do more checks on the incoming packet. >> */ >> - if ((res == NULL) || (res->version != PROXY_PROTO_VERSION) || >> + assert (res != NULL); >> + if ((res->version != PROXY_PROTO_VERSION) || >> (res->len < sizeof(virProxyPacket))) { >> virProxyError(conn, VIR_ERR_INTERNAL_ERROR, "%s", >> _("Communication error with proxy: malformed packet\n")); > > I'm not too fond of adding an assert, res should not be null there or > we should already have crashed. > > ACK to the change without the new assert line. Considering that this is in the daemon and that "bad things" have been known to happen via NULL derefs, some would argue that an assertion failure is preferable.