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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

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