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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 01/06/2016 08:24 AM, Jan Beulich wrote:
>>>> On 01.01.16 at 04:14, <jtotto@uwaterloo.ca> wrote:
>> Coverity CID 1343310
>>
>> No functional changes.
>>
>> Signed-off-by: Joshua Otto <jtotto@uwaterloo.ca>
>> ---
>> 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
>