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

On Wed, Jan 06, 2010 at 09:55:23PM +0100, Jim Meyering wrote: > 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. No, this code is the client side of the Xen proxy implementation, that is never used within daemon context.