Re: [libvirt] [PATCH] xm_internal.c: remove misleading dead code

Daniel P. Berrange wrote: > On Mon, Dec 14, 2009 at 09:40:59PM +0100, Jim Meyering wrote: >> This code triggered a warning about possible NULL-dereference. >> Actually it's more about being self-contradictory, since >> the NULL-dereference is not possible. >> >> >From 796e3a3254cb08fc20bce790997b5787dab51902 Mon Sep 17 00:00:00 2001 >> From: Jim Meyering <meyering(a)redhat.com&gt; >> Date: Mon, 14 Dec 2009 21:37:54 +0100 >> Subject: [PATCH] xm_internal.c: remove misleading dead code >> >> * src/xen/xm_internal.c (xenXMConfigGetULong): Remove useless and >> misleading test (always false) for val->str == NULL before code that >> always dereferences val->str. "val" comes from virConfGetValue, and >> at that point, val->str is guaranteed to be non-NULL. >> (xenXMConfigGetBool): Likewise. > > It isn't entirely clear to me that the virConf class guarnetees > val->str to be non-NULL. virConfParseValue ensures that non val->str it produces is never NULL. As you pointed out, there's still the possibility that some internal caller of virConfSetValue could mistakenly introduce an invalid entry. How about amending my patch with this addition to prevent that? diff --git a/src/util/conf.c b/src/util/conf.c index 0c7e556..60cf0b4 100644 --- a/src/util/conf.c +++ b/src/util/conf.c @@ -858,6 +858,9 @@ virConfSetValue (virConfPtr conf, { virConfEntryPtr cur, prev = NULL; + if (value && value->type == VIR_CONF_STRING && value->str == NULL) + return -1; + cur = conf->entries; while (cur != NULL) { if ((cur->name != NULL) && (STREQ(cur->name, setting))) {