Re: [Xen-devel] [PATCH 5/7] libxc: Removing dead code from xc_dom_allocate()

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Fri, 2015-07-03 at 16:33 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH 5/7] libxc: Removing dead code 
> from xc_dom_allocate()"):
> > On Fri, 2015-07-03 at 16:24 +0100, Ian Jackson wrote:
> > > Jennifer Herbert writes ("[Xen-devel] [PATCH 5/7] libxc: Removing dead 
> > > code from xc_dom_allocate()"):
> > > > The only place that jumps to 'err:' does so because !dom, which is
> > > > rechecked in 'err:'.  This patch simplifies, giving the same result.
> > > 
> > > I'm not particularly convinced by this change, but maybe Ian Campbell
> > > disagrees.
> > > 
> > > I presume that your Coverity instance is complaining about the fact
> > > that the if (dom) clause's test is always false.  This is true with
> > > the current code, but if this function were to gain any other code it
> > > might stop being true and the first thing to do to get a good error
> > > handling pattern would be to revert this patch.
> > 
> > I briefly thought something like this but decided it was under my
> > threshold for worrying about.
> 
> I guess my argument is that while this patch makes Coverity happy, it
> makes the code slightly worse.  A future programmer will be more
> likely to be tempted to simply `return' on error, even when it isn't
> right (from this, or other, functions).

Yes, true, but we'd probably catch that in review.

I'd be ok with either leaving this as is (and tagging CID 1055188 as
intentioanl) or adding the null initialiser as you suggested in your
"However: "

Ian.


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel