[Spice-devel] [spice-server PATCH 2/3] test-loop: increment a variable outside of spice_assert

[Frediano Ziglio]

> 
> On 8/11/19 2:56 PM, Frediano Ziglio wrote:
> >>
> >> spice_assert is a macro and it may be that variable will
> >> be incremented twice (in theory, possibly not in practice).
> >>
> > 
> > No, the issue is that Coverity assume that code can be stripped out
> > as usually assert can be stripped out (defining NDEBUG for normal
> > assert).
> 
> You are correct that this is what the covscan complains about:
> "The containing function might work differently in a non-debug build."
> 
> But spice_assert definition does not depend on NDEBUG.
> On the other hand it does not happen in reality that
> the variable is incremented twice.
> 
> I'll change the above comment to say
>    spice_assert is a macro so prevent side-effects by
>    not changing the variable in it.
> 

Why not telling the truth? That is to remove a Coverity false
positive specifying which one?

> Uri.
> 
> > 
> >> Simply do it one line above.
> >>
> >> Found by covscan
> >>
> >> Signed-off-by: Uri Lublin <uril at redhat.com>
> 
> 
> > 
> > Beside the commit message no complains to make Coverity happy
> > 
> >> ---
> >>   server/tests/test-loop.c | 3 ++-
> >>   1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/server/tests/test-loop.c b/server/tests/test-loop.c
> >> index 82af80ab3..4df3a7d20 100644
> >> --- a/server/tests/test-loop.c
> >> +++ b/server/tests/test-loop.c
> >> @@ -78,7 +78,8 @@ static SpiceTimer *twice_timers_remove[2] = { NULL, NULL
> >> };
> >>   static int twice_remove_called = 0;
> >>   static void timer_not_twice_remove(void *opaque)
> >>   {
> >> -    spice_assert(++twice_remove_called == 1);
> >> +    ++twice_remove_called;
> >> +    spice_assert(twice_remove_called == 1);
> >>   
> >>       /* delete timers, should not have another call */
> >>       core->timer_remove(twice_timers_remove[0]);
> > 
> > Frediano
> > 
> 
>