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

Is it coverity or clang ?
I just realized that covscan now uses both of them.

> > 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]);
> > >