mbox series

[0/5] misc: Reduce assert side-effects

Message ID 20200902080801.160652-1-philmd@redhat.com
Headers show
Series misc: Reduce assert side-effects | expand

Message

Philippe Mathieu-Daudé Sept. 2, 2020, 8:07 a.m. UTC 
Fix a Coverity warning in qtest,
do not declare local variable only used for assertions
(use them directly).

Philippe Mathieu-Daudé (5):
  tests/qtest/ipmi-kcs: Fix assert side-effect
  hw/pci-bridge: Do not declare local variable only used for assertion
  hw/ppc/spapr: Do not declare local variable only used for assertion
  tcg/tcg: Do not declare local variable only used for assertion
  util/qsp: Do not declare local variable only used for assertion

 hw/pci-bridge/pci_bridge_dev.c | 4 +---
 hw/ppc/spapr.c                 | 3 +--
 tcg/tcg.c                      | 3 +--
 tests/qtest/ipmi-kcs-test.c    | 3 ++-
 util/qsp.c                     | 4 +---
 5 files changed, 6 insertions(+), 11 deletions(-)

Comments

Greg Kurz Sept. 2, 2020, 8:50 a.m. UTC | #1 
On Wed,  2 Sep 2020 10:07:57 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> Fix assert side-effect reported by Coverity:
> 
>   /qemu/tests/qtest/ipmi-kcs-test.c: 84 in kcs_wait_obf()
>   83         while (IPMI_KCS_CMDREG_GET_OBF() == 0) {
>   >>>     CID 1432368:  Incorrect expression  (ASSERT_SIDE_EFFECT)
>   >>>     Argument "--count" of g_assert() has a side effect.  The containing function might work differently in a non-debug build.
>   84             g_assert(--count != 0);
> 
> Reported-by: Coverity (CID 1432368)
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  tests/qtest/ipmi-kcs-test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/ipmi-kcs-test.c b/tests/qtest/ipmi-kcs-test.c
> index 693a6aacb52..fc0a918c8d1 100644
> --- a/tests/qtest/ipmi-kcs-test.c
> +++ b/tests/qtest/ipmi-kcs-test.c
> @@ -81,7 +81,8 @@ static void kcs_wait_obf(void)
>  {
>      unsigned int count = 1000;
>      while (IPMI_KCS_CMDREG_GET_OBF() == 0) {
> -        g_assert(--count != 0);
> +        --count;
> +        g_assert(count != 0);
>      }
>  }
>
Greg Kurz Sept. 2, 2020, 9:02 a.m. UTC | #2 
On Wed,  2 Sep 2020 10:08:00 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> This variable is used once in an assertion. Remove single
> declaration and access directly in the assert().
> 
> See in "qemu/osdep.h":
> 
>  *                                  [...] disable assertion is not
>  * supported upstream so the risk is all yours.  Meanwhile, please
>  * submit patches to remove any side-effects inside an assertion, or
>  * fixing error handling that should use Error instead of assert.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

Ditto.

>  tcg/tcg.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 62f299e36e5..0bff49e544a 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -777,7 +777,6 @@ void tcg_register_thread(void)
>  #else
>  void tcg_register_thread(void)
>  {
> -    MachineState *ms = MACHINE(qdev_get_machine());
>      TCGContext *s = g_malloc(sizeof(*s));
>      unsigned int i, n;
>      bool err;
> @@ -795,7 +794,7 @@ void tcg_register_thread(void)
>  
>      /* Claim an entry in tcg_ctxs */
>      n = atomic_fetch_inc(&n_tcg_ctxs);
> -    g_assert(n < ms->smp.max_cpus);
> +    g_assert(n < MACHINE(qdev_get_machine())->smp.max_cpus);
>      atomic_set(&tcg_ctxs[n], s);
>  
>      if (n > 0) {
Thomas Huth Sept. 2, 2020, 9:54 a.m. UTC | #3 
On 02/09/2020 10.07, Philippe Mathieu-Daudé wrote:
> Fix assert side-effect reported by Coverity:
> 
>   /qemu/tests/qtest/ipmi-kcs-test.c: 84 in kcs_wait_obf()
>   83         while (IPMI_KCS_CMDREG_GET_OBF() == 0) {
>   >>>     CID 1432368:  Incorrect expression  (ASSERT_SIDE_EFFECT)
>   >>>     Argument "--count" of g_assert() has a side effect.  The containing function might work differently in a non-debug build.
>   84             g_assert(--count != 0);
> 
> Reported-by: Coverity (CID 1432368)
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  tests/qtest/ipmi-kcs-test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/qtest/ipmi-kcs-test.c b/tests/qtest/ipmi-kcs-test.c
> index 693a6aacb52..fc0a918c8d1 100644
> --- a/tests/qtest/ipmi-kcs-test.c
> +++ b/tests/qtest/ipmi-kcs-test.c
> @@ -81,7 +81,8 @@ static void kcs_wait_obf(void)
>  {
>      unsigned int count = 1000;
>      while (IPMI_KCS_CMDREG_GET_OBF() == 0) {
> -        g_assert(--count != 0);
> +        --count;
> +        g_assert(count != 0);
>      }
>  }
>  
> 

You could also use g_assert_true() instead. Anyway:

Reviewed-by: Thomas Huth <thuth@redhat.com>