svm: rephrase local variable use for Coverity.

Commit Message

Joshua Otto Jan. 1, 2016, 3:14 a.m. UTC

Coverity CID 1343310

No functional changes.

Signed-off-by: Joshua Otto <[e-mail]>
---
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?

 xen/arch/x86/hvm/svm/intr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Comments

Jan Beulich Jan. 6, 2016, 1:24 p.m. UTC | #1

>>> On 01.01.16 at 04:14, <[e-mail]> wrote:
> Coverity CID 1343310
> 
> No functional changes.
> 
> Signed-off-by: Joshua Otto <[e-mail]>
> ---
> 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

Boris Ostrovsky Jan. 6, 2016, 2:33 p.m. UTC | #2

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

Patch

diff --git a/xen/arch/x86/hvm/svm/intr.c b/xen/arch/x86/hvm/svm/intr.c
index bd94731..240eb35 100644
--- 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 )
                 return;
         }
     }