[libvirt] [PATCH 11/24] hostdev: Remove redundant check

[John Ferlan]

On 03/10/2016 01:46 PM, Andrea Bolognani wrote:
> On Thu, 2016-03-10 at 12:35 -0500, John Ferlan wrote:
>>>> If for some reason we "continue" (or not) and eventually "goto resetvfnetconfig;"
>>>> before ever setting last_processed_hostdev_vf, then we get to the goto.,..
>>>>   
>>>>>     resetvfnetconfig:
>>>>> -    for (i = 0;
>>>>> -         last_processed_hostdev_vf != -1 && i <= last_processed_hostdev_vf; i++)
>>>> and last_processed_hostdev_vf still == -1
>>>>   
>>>> So that check needs to be there - perhaps just add an:
>>>>   
>>>>       if (last_processed_hostdev_vf > -1) {
>>>>       }
>>> I fail to see how this is a problem: if last_processed_hostdev_vf
>>> has never been assigned a value after being declared, then the
>>> rewritten loop would be equivalent to
>>>  
>>>    for (i = 0; i <= -1; i++) { ... }
>>>  
>>> which means the loop body will never be executed, just as
>>> expected. Am I missing something?
>>  
>> size_t i;
>>  
>> Coverity says:
>>  
>> (60) Event negative_returns:  Using unsigned variable
>> "last_processed_hostdev_vf" in a loop exit condition.
> 
> I admit I had to do some research, but I get it now.
> 
> Not having any experience with the tool, its output looks very
> confusing to me[1]; thankfully you are fluent in Coverity and
> pointed me in the right direction :)

Yeah it's like reading a whole new language sometimes. Especially
confusing when you lookup the last*vf and see it's an 'int'.  I would
have felt better if it complained using an 'unsigned int' (i) and that
shouldn't be compared with an 'int' (last*vf).

> 
> Will change it to move the check on last_processed_hostdev_vf
> outside the for loop as you suggested, it might not be as nice
> as getting rid of it altogether but it's still much more
> readable than the current code.

Well you could always go with a while loop and remove from the end..

while (last*vf >= 0) {
    hostdev = hostdevs[last*vf--];
    virHostdevNetConfigRestore(hostdev,...);
}

> 
> Cheers.
> 
> 
> [1] Why does it claim that 'last_processed_hostdev_vf' is an
>     unsigned variable when it's not? And why would using an
>     unsigned variable in loop exit condition automatically be
>     a problem?

Asking the wrong guy about compilers, size_t's, ssize_t's, int's,
unsigned int's and what happens. In the long run it's all about
compilers and types.  That loop probably ends up being controlled in
compiler code by using a 64-bit register doing unsigned variable
comparisons (since that's what 'i' is). The last*vf will get moved into
another register as an unsigned. That's a WAG.

John