Re: [Xen-devel] [PATCH] svm: rephrase local variable use for Coverity.

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

On 01/06/2016 08:24 AM, Jan Beulich wrote:
> > > > On 01.01.16 at 04:14, <jtotto@xxxxxxxxxxxx> wrote:
> > Coverity CID 1343310
> >
> > No functional changes.
> >
> > Signed-off-by: Joshua Otto <jtotto@xxxxxxxxxxxx>
> > ---
> > On Mon, Dec 28, 2015 at 09:34:28AM +0000, Andrew Cooper wrote:
> > > The error message isn't fantastic, but the complaint that Coverity
> > > has is that we store intr here, then unilaterally store it again
> > > slightly lower in the function, no matter what value it had (with
> > > the early return presumably not being taken into account).
> > >
> > > The error would probably be resolved if lines 95 and 96 turned into
> > > "if ( vmcb_get_vintr(gvmcb).fields.irq )"
> > This patch implements that change - as a general rule, is maintainer
> > preference to resolve false positives like this by suppressing them in
> > the tool or through code changes like this one?
I'd rather suppress this in the tool as I am one of those people that 
Jan refers to below ;-)
However, if it's too much of a hassle then this patch would be OK.

-boris

> Asking such a question it would be helpful if you included the
> maintainers of the code in question, since to a good part this
> is a matter of taste, especially when ...
>
> > --- a/xen/arch/x86/hvm/svm/intr.c
> > +++ b/xen/arch/x86/hvm/svm/intr.c
> > @@ -92,8 +92,7 @@ static void svm_enable_intr_window(struct vcpu *v, struct 
> > hvm_intack intack)
> >                * return here or l2 guest looses interrupts, otherwise.
> >                */
> >               ASSERT(gvmcb != NULL);
> > -            intr = vmcb_get_vintr(gvmcb);
> > -            if ( intr.fields.irq )
> > +            if ( vmcb_get_vintr(gvmcb).fields.irq )
> ... some people (not me) frown upon complex expressions like the
> one resulting here.
>
> Also please note that while perhaps minor here, obvious with
> the quote from an earlier mail conversation, naming the person
> having suggested the change would be appropriate - if you
> look for them, you'll find quite a few Suggested-by: tags in the
> commit history.
>
> Jan

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