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

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

[Philippe Mathieu-Daudé]

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

-- 
2.26.2

[PATCH 1/5] tests/qtest/ipmi-kcs: Fix assert side-effect

[Philippe Mathieu-Daudé]

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);
     }
 }
 
-- 
2.26.2

[PATCH 2/5] hw/pci-bridge: Do not declare local variable only used for assertion

[Philippe Mathieu-Daudé]

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>
---
 hw/pci-bridge/pci_bridge_dev.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
index 4a080b7c7bf..10c521085d7 100644
--- a/hw/pci-bridge/pci_bridge_dev.c
+++ b/hw/pci-bridge/pci_bridge_dev.c
@@ -224,9 +224,7 @@ void pci_bridge_dev_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
 void pci_bridge_dev_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
-    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
-
-    g_assert(shpc_present(pci_hotplug_dev));
+    g_assert(shpc_present(PCI_DEVICE(hotplug_dev)));
     shpc_device_unplug_cb(hotplug_dev, dev, errp);
 }
 
-- 
2.26.2

[PATCH 3/5] hw/ppc/spapr: Do not declare local variable only used for assertion

[Philippe Mathieu-Daudé]

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>
---
 hw/ppc/spapr.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index dd2fa4826b3..6447a5b3808 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -628,7 +628,6 @@ static int spapr_dt_dynamic_reconfiguration_memory(SpaprMachineState *spapr,
 static int spapr_dt_memory(SpaprMachineState *spapr, void *fdt)
 {
     MachineState *machine = MACHINE(spapr);
-    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
     hwaddr mem_start, node_size;
     int i, nb_nodes = machine->numa_state->num_nodes;
     NodeInfo *nodes = machine->numa_state->nodes;
@@ -670,7 +669,7 @@ static int spapr_dt_memory(SpaprMachineState *spapr, void *fdt)
     if (spapr_ovec_test(spapr->ov5_cas, OV5_DRCONF_MEMORY)) {
         int ret;
 
-        g_assert(smc->dr_lmb_enabled);
+        g_assert(SPAPR_MACHINE_GET_CLASS(spapr)->dr_lmb_enabled);
         ret = spapr_dt_dynamic_reconfiguration_memory(spapr, fdt);
         if (ret) {
             return ret;
-- 
2.26.2

[PATCH 4/5] tcg/tcg: Do not declare local variable only used for assertion

[Philippe Mathieu-Daudé]

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>
---
 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) {
-- 
2.26.2

[PATCH 5/5] util/qsp: Do not declare local variable only used for assertion

[Philippe Mathieu-Daudé]

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>
---
 util/qsp.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/util/qsp.c b/util/qsp.c
index 7d5147f1b20..1db044ecedd 100644
--- a/util/qsp.c
+++ b/util/qsp.c
@@ -560,9 +560,7 @@ static void qsp_iter_diff(void *p, uint32_t hash, void *htp)
 
     /* No point in reporting an empty entry */
     if (new->n_acqs == 0 && new->ns == 0) {
-        bool removed = qht_remove(ht, new, hash);
-
-        g_assert(removed);
+        g_assert(qht_remove(ht, new, hash));
         g_free(new);
     }
 }
-- 
2.26.2

Re: [PATCH 1/5] tests/qtest/ipmi-kcs: Fix assert side-effect

[Greg Kurz]

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

Re: [PATCH 2/5] hw/pci-bridge: Do not declare local variable only used for assertion

[Greg Kurz]

On Wed,  2 Sep 2020 10:07:58 +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.
> 

Not sure how this relates to what the patch is doing, ie. get rid
of a local variable with a single user...

> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/pci-bridge/pci_bridge_dev.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/hw/pci-bridge/pci_bridge_dev.c b/hw/pci-bridge/pci_bridge_dev.c
> index 4a080b7c7bf..10c521085d7 100644
> --- a/hw/pci-bridge/pci_bridge_dev.c
> +++ b/hw/pci-bridge/pci_bridge_dev.c
> @@ -224,9 +224,7 @@ void pci_bridge_dev_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>  void pci_bridge_dev_unplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                Error **errp)
>  {
> -    PCIDevice *pci_hotplug_dev = PCI_DEVICE(hotplug_dev);
> -
> -    g_assert(shpc_present(pci_hotplug_dev));
> +    g_assert(shpc_present(PCI_DEVICE(hotplug_dev)));

I don't think this is correct because PCI_DEVICE() has a side-effect: it
causes QEMU to abort() if dev doesn't point to a PCI device. I guess that
if a non-PCI device reaches this code, something is clearly wrong and we
should abort(), even if QEMU was built without assertions.

>      shpc_device_unplug_cb(hotplug_dev, dev, errp);
>  }
>  

Re: [PATCH 3/5] hw/ppc/spapr: Do not declare local variable only used for assertion

[Greg Kurz]

On Wed,  2 Sep 2020 10:07:59 +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>
> ---

Same remarks as patch 2/5.

>  hw/ppc/spapr.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index dd2fa4826b3..6447a5b3808 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -628,7 +628,6 @@ static int spapr_dt_dynamic_reconfiguration_memory(SpaprMachineState *spapr,
>  static int spapr_dt_memory(SpaprMachineState *spapr, void *fdt)
>  {
>      MachineState *machine = MACHINE(spapr);
> -    SpaprMachineClass *smc = SPAPR_MACHINE_GET_CLASS(spapr);
>      hwaddr mem_start, node_size;
>      int i, nb_nodes = machine->numa_state->num_nodes;
>      NodeInfo *nodes = machine->numa_state->nodes;
> @@ -670,7 +669,7 @@ static int spapr_dt_memory(SpaprMachineState *spapr, void *fdt)
>      if (spapr_ovec_test(spapr->ov5_cas, OV5_DRCONF_MEMORY)) {
>          int ret;
>  
> -        g_assert(smc->dr_lmb_enabled);
> +        g_assert(SPAPR_MACHINE_GET_CLASS(spapr)->dr_lmb_enabled);
>          ret = spapr_dt_dynamic_reconfiguration_memory(spapr, fdt);
>          if (ret) {
>              return ret;

Re: [PATCH 4/5] tcg/tcg: Do not declare local variable only used for assertion

[Greg Kurz]

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

Re: [PATCH 5/5] util/qsp: Do not declare local variable only used for assertion

[Greg Kurz]

On Wed,  2 Sep 2020 10:08:01 +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>
> ---
>  util/qsp.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/util/qsp.c b/util/qsp.c
> index 7d5147f1b20..1db044ecedd 100644
> --- a/util/qsp.c
> +++ b/util/qsp.c
> @@ -560,9 +560,7 @@ static void qsp_iter_diff(void *p, uint32_t hash, void *htp)
>  
>      /* No point in reporting an empty entry */
>      if (new->n_acqs == 0 && new->ns == 0) {
> -        bool removed = qht_remove(ht, new, hash);
> -
> -        g_assert(removed);
> +        g_assert(qht_remove(ht, new, hash));

Urgh... this is doing exactly the opposite of the "qemu/osdep.h"
recommandations above. We still want to remove new from the
hash table even if QEMU was built without assertions.

>          g_free(new);
>      }
>  }

Re: [PATCH 5/5] util/qsp: Do not declare local variable only used for assertion

[Philippe Mathieu-Daudé]

On 9/2/20 11:11 AM, Greg Kurz wrote:
> On Wed,  2 Sep 2020 10:08:01 +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>
>> ---
>>  util/qsp.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/util/qsp.c b/util/qsp.c
>> index 7d5147f1b20..1db044ecedd 100644
>> --- a/util/qsp.c
>> +++ b/util/qsp.c
>> @@ -560,9 +560,7 @@ static void qsp_iter_diff(void *p, uint32_t hash, void *htp)
>>  
>>      /* No point in reporting an empty entry */
>>      if (new->n_acqs == 0 && new->ns == 0) {
>> -        bool removed = qht_remove(ht, new, hash);
>> -
>> -        g_assert(removed);
>> +        g_assert(qht_remove(ht, new, hash));
> 
> Urgh... this is doing exactly the opposite of the "qemu/osdep.h"
> recommandations above. We still want to remove new from the
> hash table even if QEMU was built without assertions.

Oops you are right :/

> 
>>          g_free(new);
>>      }
>>  }
> 

Re: [PATCH 1/5] tests/qtest/ipmi-kcs: Fix assert side-effect

[Thomas Huth]

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>