For us, assertions are always enabled, but side-effect expressions
inside the argument to g_assert() are bad style anyway. Fix three
occurrences in IPMI related tests, which will silence some Coverity
nits.

Fixes: CID 1432322, CID 1432287, CID 1432291
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tests/qtest/ipmi-bt-test.c  | 6 ++++--
 tests/qtest/ipmi-kcs-test.c | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tests/qtest/ipmi-bt-test.c b/tests/qtest/ipmi-bt-test.c
index a42207d416f..8492f02a9c3 100644
--- a/tests/qtest/ipmi-bt-test.c
+++ b/tests/qtest/ipmi-bt-test.c
@@ -98,7 +98,8 @@ static void bt_wait_b_busy(void)
 {
     unsigned int count = 1000;
     while (IPMI_BT_CTLREG_GET_B_BUSY() != 0) {
-        g_assert(--count != 0);
+        --count;
+        g_assert(count != 0);
         usleep(100);
     }
 }
@@ -107,7 +108,8 @@ static void bt_wait_b2h_atn(void)
 {
     unsigned int count = 1000;
     while (IPMI_BT_CTLREG_GET_B2H_ATN() == 0) {
-        g_assert(--count != 0);
+        --count;
+        g_assert(count != 0);
         usleep(100);
     }
 }
diff --git a/tests/qtest/ipmi-kcs-test.c b/tests/qtest/ipmi-kcs-test.c
index fc0a918c8d1..afc24dd3e46 100644
--- a/tests/qtest/ipmi-kcs-test.c
+++ b/tests/qtest/ipmi-kcs-test.c
@@ -73,7 +73,8 @@ static void kcs_wait_ibf(void)
 {
     unsigned int count = 1000;
     while (IPMI_KCS_CMDREG_GET_IBF() != 0) {
-        g_assert(--count != 0);
+        --count;
+        g_assert(count != 0);
     }
 }
 
-- 
2.20.1

On 5/3/21 6:55 PM, Peter Maydell wrote:
> For us, assertions are always enabled, but side-effect expressions
> inside the argument to g_assert() are bad style anyway. Fix three
> occurrences in IPMI related tests, which will silence some Coverity
> nits.
> 
> Fixes: CID 1432322, CID 1432287, CID 1432291
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/qtest/ipmi-bt-test.c  | 6 ++++--
>  tests/qtest/ipmi-kcs-test.c | 3 ++-
>  2 files changed, 6 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

On 03/05/2021 18.55, Peter Maydell wrote:
> For us, assertions are always enabled, but side-effect expressions
> inside the argument to g_assert() are bad style anyway. Fix three
> occurrences in IPMI related tests, which will silence some Coverity
> nits.
> 
> Fixes: CID 1432322, CID 1432287, CID 1432291
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   tests/qtest/ipmi-bt-test.c  | 6 ++++--
>   tests/qtest/ipmi-kcs-test.c | 3 ++-
>   2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qtest/ipmi-bt-test.c b/tests/qtest/ipmi-bt-test.c
> index a42207d416f..8492f02a9c3 100644
> --- a/tests/qtest/ipmi-bt-test.c
> +++ b/tests/qtest/ipmi-bt-test.c
> @@ -98,7 +98,8 @@ static void bt_wait_b_busy(void)
>   {
>       unsigned int count = 1000;
>       while (IPMI_BT_CTLREG_GET_B_BUSY() != 0) {
> -        g_assert(--count != 0);
> +        --count;
> +        g_assert(count != 0);
>           usleep(100);
>       }
>   }
> @@ -107,7 +108,8 @@ static void bt_wait_b2h_atn(void)
>   {
>       unsigned int count = 1000;
>       while (IPMI_BT_CTLREG_GET_B2H_ATN() == 0) {
> -        g_assert(--count != 0);
> +        --count;
> +        g_assert(count != 0);
>           usleep(100);
>       }
>   }
> diff --git a/tests/qtest/ipmi-kcs-test.c b/tests/qtest/ipmi-kcs-test.c
> index fc0a918c8d1..afc24dd3e46 100644
> --- a/tests/qtest/ipmi-kcs-test.c
> +++ b/tests/qtest/ipmi-kcs-test.c
> @@ -73,7 +73,8 @@ static void kcs_wait_ibf(void)
>   {
>       unsigned int count = 1000;
>       while (IPMI_KCS_CMDREG_GET_IBF() != 0) {
> -        g_assert(--count != 0);
> +        --count;
> +        g_assert(count != 0);
>       }
>   }

According to 
https://developer.gnome.org/glib/unstable/glib-Testing.html#g-assert 
g_assert() should be avoided in unit tests and g_assert_true() should be 
used instead. So I think it might be nicer to use g_assert_true() here?

  Thomas

On Tue, 4 May 2021 at 08:18, Thomas Huth <thuth@redhat.com> wrote:
>
> On 03/05/2021 18.55, Peter Maydell wrote:
> > For us, assertions are always enabled, but side-effect expressions
> > inside the argument to g_assert() are bad style anyway. Fix three
> > occurrences in IPMI related tests, which will silence some Coverity
> > nits.
> >
> > Fixes: CID 1432322, CID 1432287, CID 1432291
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> > diff --git a/tests/qtest/ipmi-kcs-test.c b/tests/qtest/ipmi-kcs-test.c
> > index fc0a918c8d1..afc24dd3e46 100644
> > --- a/tests/qtest/ipmi-kcs-test.c
> > +++ b/tests/qtest/ipmi-kcs-test.c
> > @@ -73,7 +73,8 @@ static void kcs_wait_ibf(void)
> >   {
> >       unsigned int count = 1000;
> >       while (IPMI_KCS_CMDREG_GET_IBF() != 0) {
> > -        g_assert(--count != 0);
> > +        --count;
> > +        g_assert(count != 0);
> >       }
> >   }
>
> According to
> https://developer.gnome.org/glib/unstable/glib-Testing.html#g-assert
> g_assert() should be avoided in unit tests and g_assert_true() should be
> used instead. So I think it might be nicer to use g_assert_true() here?

That probably depends on what we decide about whether we want to
use glib-testing-style "assert but this might not stop execution" in our
tests: see this thread:
https://lore.kernel.org/qemu-devel/CAFEAcA9juOChqrh5orybJQwpQsyEZ5z3Dvmy7fjX0DW4Nbgmrg@mail.gmail.com/
(I should have cc'd you and Laurent on that as qtest maintainers; sorry.)

-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> For us, assertions are always enabled, but side-effect expressions
> inside the argument to g_assert() are bad style anyway. Fix three
> occurrences in IPMI related tests, which will silence some Coverity
> nits.
>
> Fixes: CID 1432322, CID 1432287, CID 1432291
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  tests/qtest/ipmi-bt-test.c  | 6 ++++--
>  tests/qtest/ipmi-kcs-test.c | 3 ++-
>  2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/tests/qtest/ipmi-bt-test.c b/tests/qtest/ipmi-bt-test.c
> index a42207d416f..8492f02a9c3 100644
> --- a/tests/qtest/ipmi-bt-test.c
> +++ b/tests/qtest/ipmi-bt-test.c
> @@ -98,7 +98,8 @@ static void bt_wait_b_busy(void)
>  {
>      unsigned int count = 1000;
>      while (IPMI_BT_CTLREG_GET_B_BUSY() != 0) {
> -        g_assert(--count != 0);
> +        --count;
> +        g_assert(count != 0);

This does seem a little weird - we are not asserting an interface
violation just that the read should have cleared in 1000 * 100 usec. If
it doesn't is that really a theoretically impossible situation or just
an example of a failed test.

That said looking at how deeply buried in the test these helpers are an
assert is probably better than a convoluted attempt to return out and
exit the test with a failure.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


>          usleep(100);
>      }
>  }
> @@ -107,7 +108,8 @@ static void bt_wait_b2h_atn(void)
>  {
>      unsigned int count = 1000;
>      while (IPMI_BT_CTLREG_GET_B2H_ATN() == 0) {
> -        g_assert(--count != 0);
> +        --count;
> +        g_assert(count != 0);
>          usleep(100);
>      }
>  }
> diff --git a/tests/qtest/ipmi-kcs-test.c b/tests/qtest/ipmi-kcs-test.c
> index fc0a918c8d1..afc24dd3e46 100644
> --- a/tests/qtest/ipmi-kcs-test.c
> +++ b/tests/qtest/ipmi-kcs-test.c
> @@ -73,7 +73,8 @@ static void kcs_wait_ibf(void)
>  {
>      unsigned int count = 1000;
>      while (IPMI_KCS_CMDREG_GET_IBF() != 0) {
> -        g_assert(--count != 0);
> +        --count;
> +        g_assert(count != 0);
>      }
>  }


-- 
Alex Bennée