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

Submitted by Uri Lublin on Aug. 11, 2019, 11:47 a.m.

Details

Message ID 20190811114723.22360-2-uril@redhat.com
State Superseded
Headers show
Series "Series without cover letter" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Uri Lublin Aug. 11, 2019, 11:47 a.m.
spice_assert is a macro and it may be that variable will
be incremented twice (in theory, possibly not in practice).

Simply do it one line above.

Found by covscan

Signed-off-by: Uri Lublin <uril@redhat.com>
---
 server/tests/test-loop.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

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

Comments

> 
> 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).

> Simply do it one line above.
> 
> Found by covscan
> 
> Signed-off-by: Uri Lublin <uril@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
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.

Uri.

> 
>> Simply do it one line above.
>>
>> Found by covscan
>>
>> Signed-off-by: Uri Lublin <uril@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
>
> 
> 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@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
> > 
> 
>
> > 
> > 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@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]);
> > >