[PATCH 00/11] trivial patchs for static code analyzer fixes

[PATCH 00/11] trivial patchs for static code analyzer fixes

[Chen Qun]

Hi All,
   This series fix trivial warnings reported by the Clang static code analyzer.

Chen Qun (11):
  hw/arm/virt-acpi-build:Remove dead assignment in build_madt()
  hw/arm/omap1:Remove redundant statement in omap_clkdsp_read()
  target/arm/translate-a64:Remove dead assignment in
    handle_scalar_simd_shli()
  target/arm/translate-a64:Remove redundant statement in
    disas_simd_two_reg_misc_fp16()
  hw/virtio/vhost-user:Remove dead assignment in scrub_shadow_regions()
  hw/net/virtio-net:Remove redundant statement in
    virtio_net_rsc_tcp_ctrl_check()
  vfio/platform: Remove dead assignment in vfio_intp_interrupt()
  tcg/optimize: Remove redundant statement in tcg_optimize()
  usb/bus: Remove dead assignment in usb_get_fw_dev_path()
  hw/intc: Remove redundant statement in exynos4210_combiner_read()
  hw/display/vga:Remove redundant statement in vga_draw_graphic()

 hw/arm/omap1.c                | 1 -
 hw/arm/virt-acpi-build.c      | 3 +--
 hw/display/vga.c              | 1 -
 hw/intc/exynos4210_combiner.c | 3 +--
 hw/net/virtio-net.c           | 1 -
 hw/usb/bus.c                  | 4 ++--
 hw/vfio/platform.c            | 2 +-
 hw/virtio/vhost-user.c        | 2 +-
 target/arm/translate-a64.c    | 7 ++-----
 tcg/optimize.c                | 1 -
 10 files changed, 8 insertions(+), 17 deletions(-)

-- 
2.23.0

[PATCH 01/11] hw/arm/virt-acpi-build:Remove dead assignment in build_madt()

[Chen Qun]

Clang static code analyzer show warning:
hw/arm/virt-acpi-build.c:641:5: warning: Value stored to 'madt' is never read
    madt = acpi_data_push(table_data, sizeof *madt);
    ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: qemu-arm@nongnu.org
---
 hw/arm/virt-acpi-build.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 91f0df7b13..f830f9b779 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -633,12 +633,11 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
     int madt_start = table_data->len;
     const MemMapEntry *memmap = vms->memmap;
     const int *irqmap = vms->irqmap;
-    AcpiMultipleApicTable *madt;
     AcpiMadtGenericDistributor *gicd;
     AcpiMadtGenericMsiFrame *gic_msi;
     int i;
 
-    madt = acpi_data_push(table_data, sizeof *madt);
+    acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
 
     gicd = acpi_data_push(table_data, sizeof *gicd);
     gicd->type = ACPI_APIC_GENERIC_DISTRIBUTOR;
-- 
2.23.0

[PATCH 02/11] hw/arm/omap1:Remove redundant statement in omap_clkdsp_read()

[Chen Qun]

Clang static code analyzer show warning:
hw/arm/omap1.c:1760:15: warning: Value stored to 'cpu' during its
initialization is never read
    CPUState *cpu = CPU(s->cpu);
              ^~~   ~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
---
 hw/arm/omap1.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/arm/omap1.c b/hw/arm/omap1.c
index 6ba0df6b6d..02c0f66431 100644
--- a/hw/arm/omap1.c
+++ b/hw/arm/omap1.c
@@ -1774,7 +1774,6 @@ static uint64_t omap_clkdsp_read(void *opaque, hwaddr addr,
         return s->clkm.dsp_rstct2;
 
     case 0x18:	/* DSP_SYSST */
-        cpu = CPU(s->cpu);
         return (s->clkm.clocking_scheme << 11) | s->clkm.cold_start |
                 (cpu->halted << 6);      /* Quite useless... */
     }
-- 
2.23.0

[PATCH 03/11] target/arm/translate-a64:Remove dead assignment in handle_scalar_simd_shli()

[Chen Qun]

Clang static code analyzer show warning:
target/arm/translate-a64.c:8635:14: warning: Value stored to 'tcg_rn' during its
 initialization is never read
    TCGv_i64 tcg_rn = new_tmp_a64(s);
             ^~~~~~   ~~~~~~~~~~~~~~
target/arm/translate-a64.c:8636:14: warning: Value stored to 'tcg_rd' during its
 initialization is never read
    TCGv_i64 tcg_rd = new_tmp_a64(s);
             ^~~~~~   ~~~~~~~~~~~~~~

There is a memory leak for the variable new_tmp_a64 "s".
We should delete the assignment.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
---
 target/arm/translate-a64.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index 534c3ff5f3..c83bb85e4e 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -8632,8 +8632,8 @@ static void handle_scalar_simd_shli(DisasContext *s, bool insert,
     int size = 32 - clz32(immh) - 1;
     int immhb = immh << 3 | immb;
     int shift = immhb - (8 << size);
-    TCGv_i64 tcg_rn = new_tmp_a64(s);
-    TCGv_i64 tcg_rd = new_tmp_a64(s);
+    TCGv_i64 tcg_rn;
+    TCGv_i64 tcg_rd;
 
     if (!extract32(immh, 3, 1)) {
         unallocated_encoding(s);
-- 
2.23.0

[PATCH 04/11] target/arm/translate-a64:Remove redundant statement in disas_simd_two_reg_misc_fp16()

[Chen Qun]

Clang static code analyzer show warning:
target/arm/translate-a64.c:13007:5: warning: Value stored to 'rd' is never read
    rd = extract32(insn, 0, 5);
    ^    ~~~~~~~~~~~~~~~~~~~~~
target/arm/translate-a64.c:13008:5: warning: Value stored to 'rn' is never read
    rn = extract32(insn, 5, 5);
    ^    ~~~~~~~~~~~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-arm@nongnu.org
---
 target/arm/translate-a64.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/target/arm/translate-a64.c b/target/arm/translate-a64.c
index c83bb85e4e..47cce160d8 100644
--- a/target/arm/translate-a64.c
+++ b/target/arm/translate-a64.c
@@ -13016,9 +13016,6 @@ static void disas_simd_two_reg_misc_fp16(DisasContext *s, uint32_t insn)
     fpop = deposit32(opcode, 5, 1, a);
     fpop = deposit32(fpop, 6, 1, u);
 
-    rd = extract32(insn, 0, 5);
-    rn = extract32(insn, 5, 5);
-
     switch (fpop) {
     case 0x1d: /* SCVTF */
     case 0x5d: /* UCVTF */
-- 
2.23.0

[PATCH 05/11] hw/virtio/vhost-user:Remove dead assignment in scrub_shadow_regions()

[Chen Qun]

Clang static code analyzer show warning:
hw/virtio/vhost-user.c:606:9: warning: Value stored to 'mr' is never read
        mr = vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);
        ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>
---
 hw/virtio/vhost-user.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index d7e2423762..9c5b4f7fbc 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -603,7 +603,7 @@ static void scrub_shadow_regions(struct vhost_dev *dev,
      */
     for (i = 0; i < dev->mem->nregions; i++) {
         reg = &dev->mem->regions[i];
-        mr = vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);
+        vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);
         if (fd > 0) {
             ++fd_num;
         }
-- 
2.23.0

[PATCH 06/11] hw/net/virtio-net:Remove redundant statement in virtio_net_rsc_tcp_ctrl_check()

[Chen Qun]

Clang static code analyzer show warning:
hw/net/virtio-net.c:2077:5: warning: Value stored to 'tcp_flag' is never read
    tcp_flag &= VIRTIO_NET_TCP_FLAG;
    ^           ~~~~~~~~~~~~~~~~~~~

The 'VIRTIO_NET_TCP_FLAG' is '0x3F'. The last ‘tcp_flag’ assignment statement is
 the same as that of the first two statements.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
---
 hw/net/virtio-net.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a1fe9e9285..cb0d27084c 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -2075,7 +2075,6 @@ static int virtio_net_rsc_tcp_ctrl_check(VirtioNetRscChain *chain,
     tcp_flag = htons(tcp->th_offset_flags);
     tcp_hdr = (tcp_flag & VIRTIO_NET_TCP_HDR_LENGTH) >> 10;
     tcp_flag &= VIRTIO_NET_TCP_FLAG;
-    tcp_flag = htons(tcp->th_offset_flags) & 0x3F;
     if (tcp_flag & TH_SYN) {
         chain->stat.tcp_syn++;
         return RSC_BYPASS;
-- 
2.23.0

[PATCH 07/11] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

[Chen Qun]

Clang static code analyzer show warning:
hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
        ret = event_notifier_test_and_clear(intp->interrupt);
        ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Eric Auger <eric.auger@redhat.com>
---
 hw/vfio/platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index ac2cefc9b1..869ed2c39d 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
         trace_vfio_intp_interrupt_set_pending(intp->pin);
         QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
                              intp, pqnext);
-        ret = event_notifier_test_and_clear(intp->interrupt);
+        event_notifier_test_and_clear(intp->interrupt);
         return;
     }
 
-- 
2.23.0

[PATCH 08/11] tcg/optimize: Remove redundant statement in tcg_optimize()

[Chen Qun]

Clang static code analyzer show warning:
tcg/optimize.c:1267:17: warning: Value stored to 'nb_iargs' is never read
                nb_iargs = 2;
                ^          ~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Richard Henderson <richard.henderson@linaro.org>
Cc: Richard Henderson <rth@twiddle.net>
---
 tcg/optimize.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tcg/optimize.c b/tcg/optimize.c
index 53aa8e5329..d5bea37290 100644
--- a/tcg/optimize.c
+++ b/tcg/optimize.c
@@ -1264,7 +1264,6 @@ void tcg_optimize(TCGContext *s)
                 op->opc = opc = (opc == INDEX_op_movcond_i32
                                  ? INDEX_op_setcond_i32
                                  : INDEX_op_setcond_i64);
-                nb_iargs = 2;
             }
             goto do_default;
 
-- 
2.23.0

[PATCH 09/11] usb/bus: Remove dead assignment in usb_get_fw_dev_path()

[Chen Qun]

Clang static code analyzer show warning:
qemu/hw/usb/bus.c:615:13: warning: Value stored to 'pos' is never read
            pos += snprintf(fw_path + pos, fw_len - pos, "%s@%lx",

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
---
 hw/usb/bus.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index b17bda3b29..77d3f7ddb8 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -612,8 +612,8 @@ static char *usb_get_fw_dev_path(DeviceState *qdev)
             in++;
         } else {
             /* the device itself */
-            pos += snprintf(fw_path + pos, fw_len - pos, "%s@%lx",
-                            qdev_fw_name(qdev), nr);
+            snprintf(fw_path + pos, fw_len - pos, "%s@%lx",qdev_fw_name(qdev),
+                     nr);
             break;
         }
     }
-- 
2.23.0

[PATCH 10/11] hw/intc: Remove redundant statement in exynos4210_combiner_read()

[Chen Qun]

Clang static code analyzer show warning:
hw/intc/exynos4210_combiner.c:231:9: warning: Value stored to 'val' is never read
        val = s->reg_set[offset >> 2];

The default value of 'val' is '0', so we can break the 'default' branch and return 'val'.

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Igor Mitsyanko <i.mitsyanko@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
---
 hw/intc/exynos4210_combiner.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/intc/exynos4210_combiner.c b/hw/intc/exynos4210_combiner.c
index b8561e4180..e2e745bbaa 100644
--- a/hw/intc/exynos4210_combiner.c
+++ b/hw/intc/exynos4210_combiner.c
@@ -228,8 +228,7 @@ exynos4210_combiner_read(void *opaque, hwaddr offset, unsigned size)
             hw_error("exynos4210.combiner: overflow of reg_set by 0x"
                     TARGET_FMT_plx "offset\n", offset);
         }
-        val = s->reg_set[offset >> 2];
-        return 0;
+        break;
     }
     return val;
 }
-- 
2.23.0

[PATCH 11/11] hw/display/vga:Remove redundant statement in vga_draw_graphic()

[Chen Qun]

Clang static code analyzer show warning:
hw/display/vga.c:1677:9: warning: Value stored to 'update' is never read
        update = full_update;
        ^        ~~~~~~~~~~~

Reported-by: Euler Robot <euler.robot@huawei.com>
Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
---
Cc: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/display/vga.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/display/vga.c b/hw/display/vga.c
index 061fd9ab8f..836ad50c7b 100644
--- a/hw/display/vga.c
+++ b/hw/display/vga.c
@@ -1674,7 +1674,6 @@ static void vga_draw_graphic(VGACommonState *s, int full_update)
         if (!(s->cr[VGA_CRTC_MODE] & 2)) {
             addr = (addr & ~0x8000) | ((y1 & 2) << 14);
         }
-        update = full_update;
         page0 = addr & s->vbe_size_mask;
         page1 = (addr + bwidth - 1) & s->vbe_size_mask;
         if (full_update) {
-- 
2.23.0

Re: [PATCH 06/11] hw/net/virtio-net:Remove redundant statement in virtio_net_rsc_tcp_ctrl_check()

[Philippe Mathieu-Daudé]

On 8/13/20 9:37 AM, Chen Qun wrote:
> Clang static code analyzer show warning:
> hw/net/virtio-net.c:2077:5: warning: Value stored to 'tcp_flag' is never read
>     tcp_flag &= VIRTIO_NET_TCP_FLAG;
>     ^           ~~~~~~~~~~~~~~~~~~~
> 
> The 'VIRTIO_NET_TCP_FLAG' is '0x3F'. The last ‘tcp_flag’ assignment statement is
>  the same as that of the first two statements.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/virtio-net.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a1fe9e9285..cb0d27084c 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2075,7 +2075,6 @@ static int virtio_net_rsc_tcp_ctrl_check(VirtioNetRscChain *chain,
>      tcp_flag = htons(tcp->th_offset_flags);
>      tcp_hdr = (tcp_flag & VIRTIO_NET_TCP_HDR_LENGTH) >> 10;
>      tcp_flag &= VIRTIO_NET_TCP_FLAG;
> -    tcp_flag = htons(tcp->th_offset_flags) & 0x3F;
>      if (tcp_flag & TH_SYN) {
>          chain->stat.tcp_syn++;
>          return RSC_BYPASS;
> 

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

Re: [PATCH 00/11] trivial patchs for static code analyzer fixes

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200813073712.4001404-1-kuhn.chenqun@huawei.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20200813073712.4001404-1-kuhn.chenqun@huawei.com
Subject: [PATCH 00/11] trivial patchs for static code analyzer fixes

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20200813073712.4001404-1-kuhn.chenqun@huawei.com -> patchew/20200813073712.4001404-1-kuhn.chenqun@huawei.com
Switched to a new branch 'test'
4f73727 hw/display/vga:Remove redundant statement in vga_draw_graphic()
a501d39 hw/intc: Remove redundant statement in exynos4210_combiner_read()
084ebd8 usb/bus: Remove dead assignment in usb_get_fw_dev_path()
5b26d6a tcg/optimize: Remove redundant statement in tcg_optimize()
e628a22 vfio/platform: Remove dead assignment in vfio_intp_interrupt()
565efc1 hw/net/virtio-net:Remove redundant statement in virtio_net_rsc_tcp_ctrl_check()
7b65ac4 hw/virtio/vhost-user:Remove dead assignment in scrub_shadow_regions()
2dd718c target/arm/translate-a64:Remove redundant statement in disas_simd_two_reg_misc_fp16()
47833df target/arm/translate-a64:Remove dead assignment in handle_scalar_simd_shli()
e1bb6c3 hw/arm/omap1:Remove redundant statement in omap_clkdsp_read()
39a7d8f hw/arm/virt-acpi-build:Remove dead assignment in build_madt()

=== OUTPUT BEGIN ===
1/11 Checking commit 39a7d8f2d62d (hw/arm/virt-acpi-build:Remove dead assignment in build_madt())
2/11 Checking commit e1bb6c3e178c (hw/arm/omap1:Remove redundant statement in omap_clkdsp_read())
3/11 Checking commit 47833df935a6 (target/arm/translate-a64:Remove dead assignment in handle_scalar_simd_shli())
4/11 Checking commit 2dd718cc1926 (target/arm/translate-a64:Remove redundant statement in disas_simd_two_reg_misc_fp16())
5/11 Checking commit 7b65ac4ccb9d (hw/virtio/vhost-user:Remove dead assignment in scrub_shadow_regions())
6/11 Checking commit 565efc12ec2a (hw/net/virtio-net:Remove redundant statement in virtio_net_rsc_tcp_ctrl_check())
7/11 Checking commit e628a2236e95 (vfio/platform: Remove dead assignment in vfio_intp_interrupt())
8/11 Checking commit 5b26d6af75bd (tcg/optimize: Remove redundant statement in tcg_optimize())
9/11 Checking commit 084ebd878ce6 (usb/bus: Remove dead assignment in usb_get_fw_dev_path())
ERROR: space required after that ',' (ctx:VxV)
#25: FILE: hw/usb/bus.c:615:
+            snprintf(fw_path + pos, fw_len - pos, "%s@%lx",qdev_fw_name(qdev),
                                                           ^

total: 1 errors, 0 warnings, 10 lines checked

Patch 9/11 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

10/11 Checking commit a501d39c72e6 (hw/intc: Remove redundant statement in exynos4210_combiner_read())
11/11 Checking commit 4f73727eb19d (hw/display/vga:Remove redundant statement in vga_draw_graphic())
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200813073712.4001404-1-kuhn.chenqun@huawei.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

RE: [PATCH 09/11] usb/bus: Remove dead assignment in usb_get_fw_dev_path()

[Chenqun (kuhn)]

>  hw/usb/bus.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c index b17bda3b29..77d3f7ddb8
> 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -612,8 +612,8 @@ static char *usb_get_fw_dev_path(DeviceState *qdev)
>              in++;
>          } else {
>              /* the device itself */
> -            pos += snprintf(fw_path + pos, fw_len - pos, "%s@%lx",
> -                            qdev_fw_name(qdev), nr);
> +            snprintf(fw_path + pos, fw_len - pos,
> "%s@%lx",qdev_fw_name(qdev),
Sorry, a space is missing here. I will add it later in V2.

Thanks.
> +                     nr);
>              break;
>          }
>      }
> --
> 2.23.0

Re: [PATCH 00/11] trivial patchs for static code analyzer fixes

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200813073712.4001404-1-kuhn.chenqun@huawei.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

Unexpected error in object_property_try_add() at /tmp/qemu-test/src/qom/object.c:1181:
attempt to add duplicate property 'serial-id' to object (type 'container')
  TEST    iotest-qcow2: 024
ERROR test-char - too few tests run (expected 38, got 9)
make: *** [check-unit] Error 1
make: *** Waiting for unfinished jobs....
qemu-system-aarch64: -accel kvm: invalid accelerator kvm
qemu-system-aarch64: falling back to tcg
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=fd298b6e40de496b90a85d10f907a880', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-xchonqx5/src/docker-src.2020-08-13-05.07.11.29699:/var/tmp/qemu:z,ro', 'qemu/centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=fd298b6e40de496b90a85d10f907a880
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-xchonqx5/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    16m29.883s
user    0m8.672s


The full log is available at
http://patchew.org/logs/20200813073712.4001404-1-kuhn.chenqun@huawei.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 07/11] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

[Auger Eric]

Hi,

On 8/13/20 9:37 AM, Chen Qun wrote:
> Clang static code analyzer show warning:
> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
>         ret = event_notifier_test_and_clear(intp->interrupt);
>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric

> ---
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/vfio/platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index ac2cefc9b1..869ed2c39d 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
>          trace_vfio_intp_interrupt_set_pending(intp->pin);
>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
>                               intp, pqnext);
> -        ret = event_notifier_test_and_clear(intp->interrupt);
> +        event_notifier_test_and_clear(intp->interrupt);
>          return;
>      }
>  
> 

Re: [PATCH 08/11] tcg/optimize: Remove redundant statement in tcg_optimize()

[Richard Henderson]

On 8/13/20 12:37 AM, Chen Qun wrote:
> Clang static code analyzer show warning:
> tcg/optimize.c:1267:17: warning: Value stored to 'nb_iargs' is never read
>                 nb_iargs = 2;
>                 ^          ~
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Richard Henderson <richard.henderson@linaro.org>
> Cc: Richard Henderson <rth@twiddle.net>
> ---
>  tcg/optimize.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tcg/optimize.c b/tcg/optimize.c
> index 53aa8e5329..d5bea37290 100644
> --- a/tcg/optimize.c
> +++ b/tcg/optimize.c
> @@ -1264,7 +1264,6 @@ void tcg_optimize(TCGContext *s)
>                  op->opc = opc = (opc == INDEX_op_movcond_i32
>                                   ? INDEX_op_setcond_i32
>                                   : INDEX_op_setcond_i64);
> -                nb_iargs = 2;
>              }
>              goto do_default;

I prefer not to make this change.

nb_iargs is the cached value that corresponds to opc, which we have just
changed.  If we later make a change at "do_default" that uses nb_iargs, failure
to update the value here will be a very hard bug to find.

I am happy to let the compiler remove this as dead code itself.


r~

Re: [PATCH 07/11] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

[Alex Williamson]

On Thu, 13 Aug 2020 15:37:08 +0800
Chen Qun <kuhn.chenqun@huawei.com> wrote:

> Clang static code analyzer show warning:
> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
>         ret = event_notifier_test_and_clear(intp->interrupt);
>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Eric Auger <eric.auger@redhat.com>
> ---
>  hw/vfio/platform.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> index ac2cefc9b1..869ed2c39d 100644
> --- a/hw/vfio/platform.c
> +++ b/hw/vfio/platform.c
> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
>          trace_vfio_intp_interrupt_set_pending(intp->pin);
>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
>                               intp, pqnext);
> -        ret = event_notifier_test_and_clear(intp->interrupt);
> +        event_notifier_test_and_clear(intp->interrupt);
>          return;
>      }

Testing that an event is pending in our notifier is generally a
prerequisite to doing anything in the interrupt handler, I don't
understand why we're just consuming it and ignoring the return value.
The above is in the delayed handling branch of the function, but the
normal non-delayed path would only go on to error_report() if the
notifier is not pending and then inject an interrupt anyway.  This all
seems rather suspicious and it's a unique pattern among the vfio
callers of this function.  Is there a more fundamental bug that this
function should perform this test once and return without doing
anything if it's called spuriously, ie. without a notifier pending?
Thanks,

Alex

Re: [PATCH 05/11] hw/virtio/vhost-user:Remove dead assignment in scrub_shadow_regions()

[Raphael Norwitz]

On Thu, Aug 13, 2020 at 2:39 AM Chen Qun <kuhn.chenqun@huawei.com> wrote:
>
> Clang static code analyzer show warning:
> hw/virtio/vhost-user.c:606:9: warning: Value stored to 'mr' is never read
>         mr = vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);
>         ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
>  hw/virtio/vhost-user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index d7e2423762..9c5b4f7fbc 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -603,7 +603,7 @@ static void scrub_shadow_regions(struct vhost_dev *dev,
>       */
>      for (i = 0; i < dev->mem->nregions; i++) {
>          reg = &dev->mem->regions[i];
> -        mr = vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);

Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.com>

> +        vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);
>          if (fd > 0) {
>              ++fd_num;
>          }
> --
> 2.23.0
>
>

Re: [PATCH 07/11] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

[Auger Eric]

Hi Alex,

On 8/13/20 6:59 PM, Alex Williamson wrote:
> On Thu, 13 Aug 2020 15:37:08 +0800
> Chen Qun <kuhn.chenqun@huawei.com> wrote:
> 
>> Clang static code analyzer show warning:
>> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
>>         ret = event_notifier_test_and_clear(intp->interrupt);
>>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> Reported-by: Euler Robot <euler.robot@huawei.com>
>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>> ---
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Cc: Eric Auger <eric.auger@redhat.com>
>> ---
>>  hw/vfio/platform.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>> index ac2cefc9b1..869ed2c39d 100644
>> --- a/hw/vfio/platform.c
>> +++ b/hw/vfio/platform.c
>> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
>>          trace_vfio_intp_interrupt_set_pending(intp->pin);
>>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
>>                               intp, pqnext);
>> -        ret = event_notifier_test_and_clear(intp->interrupt);
>> +        event_notifier_test_and_clear(intp->interrupt);
>>          return;
>>      }
> 
> Testing that an event is pending in our notifier is generally a
> prerequisite to doing anything in the interrupt handler, I don't
> understand why we're just consuming it and ignoring the return value.
> The above is in the delayed handling branch of the function, but the
> normal non-delayed path would only go on to error_report() if the
> notifier is not pending and then inject an interrupt anyway.  This all
> seems rather suspicious and it's a unique pattern among the vfio
> callers of this function.  Is there a more fundamental bug that this
> function should perform this test once and return without doing
> anything if it's called spuriously, ie. without a notifier pending?
> Thanks,

Hum that's correct that other VFIO call sites do the check. My
understanding was that this could not fail in this case as, if we
entered the handler there was something to be cleared. In which
situation can this fail?

Thanks

Eric
> 
> Alex
> 
> 

Re: [PATCH 07/11] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

[Alex Williamson]

On Thu, 13 Aug 2020 20:02:45 +0200
Auger Eric <eric.auger@redhat.com> wrote:

> Hi Alex,
> 
> On 8/13/20 6:59 PM, Alex Williamson wrote:
> > On Thu, 13 Aug 2020 15:37:08 +0800
> > Chen Qun <kuhn.chenqun@huawei.com> wrote:
> >   
> >> Clang static code analyzer show warning:
> >> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
> >>         ret = event_notifier_test_and_clear(intp->interrupt);
> >>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>
> >> Reported-by: Euler Robot <euler.robot@huawei.com>
> >> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> >> ---
> >> Cc: Alex Williamson <alex.williamson@redhat.com>
> >> Cc: Eric Auger <eric.auger@redhat.com>
> >> ---
> >>  hw/vfio/platform.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> >> index ac2cefc9b1..869ed2c39d 100644
> >> --- a/hw/vfio/platform.c
> >> +++ b/hw/vfio/platform.c
> >> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
> >>          trace_vfio_intp_interrupt_set_pending(intp->pin);
> >>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
> >>                               intp, pqnext);
> >> -        ret = event_notifier_test_and_clear(intp->interrupt);
> >> +        event_notifier_test_and_clear(intp->interrupt);
> >>          return;
> >>      }  
> > 
> > Testing that an event is pending in our notifier is generally a
> > prerequisite to doing anything in the interrupt handler, I don't
> > understand why we're just consuming it and ignoring the return value.
> > The above is in the delayed handling branch of the function, but the
> > normal non-delayed path would only go on to error_report() if the
> > notifier is not pending and then inject an interrupt anyway.  This all
> > seems rather suspicious and it's a unique pattern among the vfio
> > callers of this function.  Is there a more fundamental bug that this
> > function should perform this test once and return without doing
> > anything if it's called spuriously, ie. without a notifier pending?
> > Thanks,  
> 
> Hum that's correct that other VFIO call sites do the check. My
> understanding was that this could not fail in this case as, if we
> entered the handler there was something to be cleared. In which
> situation can this fail?

I'm not sure what the right answer is, I see examples either way
looking outside of vfio code.  On one hand, maybe we never get called
spuriously, on the other if it's the callee's responsibility to drain
events from the fd and we have it readily accessible whether there were
any events pending, why would we inject an interrupt if the result that
we have in hand shows no pending events?  The overhead of returning
based on that result is minuscule.

qemu_set_fd_handler() is a wrapper for aio_set_fd_handler().  Stefan is
a possible defacto maintainer of some of the aio code.  Stefan, do you
have thoughts on whether callbacks from event notifier fds should
consider spurious events?  Thanks,

Alex

Re: [PATCH 07/11] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

[Auger Eric]

Hi Alex,

On 8/13/20 9:15 PM, Alex Williamson wrote:
> On Thu, 13 Aug 2020 20:02:45 +0200
> Auger Eric <eric.auger@redhat.com> wrote:
> 
>> Hi Alex,
>>
>> On 8/13/20 6:59 PM, Alex Williamson wrote:
>>> On Thu, 13 Aug 2020 15:37:08 +0800
>>> Chen Qun <kuhn.chenqun@huawei.com> wrote:
>>>   
>>>> Clang static code analyzer show warning:
>>>> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
>>>>         ret = event_notifier_test_and_clear(intp->interrupt);
>>>>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>
>>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>>>> ---
>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
>>>> Cc: Eric Auger <eric.auger@redhat.com>
>>>> ---
>>>>  hw/vfio/platform.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>>>> index ac2cefc9b1..869ed2c39d 100644
>>>> --- a/hw/vfio/platform.c
>>>> +++ b/hw/vfio/platform.c
>>>> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
>>>>          trace_vfio_intp_interrupt_set_pending(intp->pin);
>>>>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
>>>>                               intp, pqnext);
>>>> -        ret = event_notifier_test_and_clear(intp->interrupt);
>>>> +        event_notifier_test_and_clear(intp->interrupt);
>>>>          return;
>>>>      }  
>>>
>>> Testing that an event is pending in our notifier is generally a
>>> prerequisite to doing anything in the interrupt handler, I don't
>>> understand why we're just consuming it and ignoring the return value.
>>> The above is in the delayed handling branch of the function, but the
>>> normal non-delayed path would only go on to error_report() if the
>>> notifier is not pending and then inject an interrupt anyway.  This all
>>> seems rather suspicious and it's a unique pattern among the vfio
>>> callers of this function.  Is there a more fundamental bug that this
>>> function should perform this test once and return without doing
>>> anything if it's called spuriously, ie. without a notifier pending?
>>> Thanks,  
>>
>> Hum that's correct that other VFIO call sites do the check. My
>> understanding was that this could not fail in this case as, if we
>> entered the handler there was something to be cleared. In which
>> situation can this fail?
> 
> I'm not sure what the right answer is, I see examples either way
> looking outside of vfio code.  On one hand, maybe we never get called
> spuriously, on the other if it's the callee's responsibility to drain
> events from the fd and we have it readily accessible whether there were
> any events pending, why would we inject an interrupt if the result that
> we have in hand shows no pending events?  The overhead of returning
> based on that result is minuscule.

I agree
> 
> qemu_set_fd_handler() is a wrapper for aio_set_fd_handler().  Stefan is
> a possible defacto maintainer of some of the aio code.  Stefan, do you
> have thoughts on whether callbacks from event notifier fds should
> consider spurious events?  Thanks,

Indeed I saw that for instance block/nvme.c nvme_handle_event is not
checking the result.

Let's wait for Stefan's answer ...

Thanks

Eric
> 
> Alex
> 

Re: [PATCH 11/11] hw/display/vga:Remove redundant statement in vga_draw_graphic()

[Gerd Hoffmann]

On Thu, Aug 13, 2020 at 03:37:12PM +0800, Chen Qun wrote:
> Clang static code analyzer show warning:
> hw/display/vga.c:1677:9: warning: Value stored to 'update' is never read
>         update = full_update;
>         ^        ~~~~~~~~~~~
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>

Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>

RE: [PATCH 08/11] tcg/optimize: Remove redundant statement in tcg_optimize()

[Chenqun (kuhn)]

> > diff --git a/tcg/optimize.c b/tcg/optimize.c index
> > 53aa8e5329..d5bea37290 100644
> > --- a/tcg/optimize.c
> > +++ b/tcg/optimize.c
> > @@ -1264,7 +1264,6 @@ void tcg_optimize(TCGContext *s)
> >                  op->opc = opc = (opc == INDEX_op_movcond_i32
> >                                   ? INDEX_op_setcond_i32
> >                                   : INDEX_op_setcond_i64);
> > -                nb_iargs = 2;
> >              }
> >              goto do_default;
> 
> I prefer not to make this change.
> 
> nb_iargs is the cached value that corresponds to opc, which we have just
> changed.  If we later make a change at "do_default" that uses nb_iargs,
> failure to update the value here will be a very hard bug to find.
Hi Richard,

  I think you're thinking more carefully, even though ' nb_iargs' not used in 'do_default' now.

However, I have only one small question. Why does 'nb_iargs' need to be assigned a value for this case branch?
Other branches(eg:' CASE_OP_32_64(brcond)') have changed the opc value too. Do we need to assign a value to nb_iargs for it?

Thanks.

Re: [PATCH 08/11] tcg/optimize: Remove redundant statement in tcg_optimize()

[Richard Henderson]

On 8/17/20 6:04 AM, Chenqun (kuhn) wrote:
> Other branches(eg:' CASE_OP_32_64(brcond)') have changed the opc value too. Do we need to assign a value to nb_iargs for it?

In those cases nb_iargs does not change.


r~

Re: [PATCH 07/11] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 4219 bytes --]

On Thu, Aug 13, 2020 at 09:18:59PM +0200, Auger Eric wrote:
> Hi Alex,
> 
> On 8/13/20 9:15 PM, Alex Williamson wrote:
> > On Thu, 13 Aug 2020 20:02:45 +0200
> > Auger Eric <eric.auger@redhat.com> wrote:
> > 
> >> Hi Alex,
> >>
> >> On 8/13/20 6:59 PM, Alex Williamson wrote:
> >>> On Thu, 13 Aug 2020 15:37:08 +0800
> >>> Chen Qun <kuhn.chenqun@huawei.com> wrote:
> >>>   
> >>>> Clang static code analyzer show warning:
> >>>> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
> >>>>         ret = event_notifier_test_and_clear(intp->interrupt);
> >>>>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>>
> >>>> Reported-by: Euler Robot <euler.robot@huawei.com>
> >>>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> >>>> ---
> >>>> Cc: Alex Williamson <alex.williamson@redhat.com>
> >>>> Cc: Eric Auger <eric.auger@redhat.com>
> >>>> ---
> >>>>  hw/vfio/platform.c | 2 +-
> >>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
> >>>> index ac2cefc9b1..869ed2c39d 100644
> >>>> --- a/hw/vfio/platform.c
> >>>> +++ b/hw/vfio/platform.c
> >>>> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
> >>>>          trace_vfio_intp_interrupt_set_pending(intp->pin);
> >>>>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
> >>>>                               intp, pqnext);
> >>>> -        ret = event_notifier_test_and_clear(intp->interrupt);
> >>>> +        event_notifier_test_and_clear(intp->interrupt);
> >>>>          return;
> >>>>      }  
> >>>
> >>> Testing that an event is pending in our notifier is generally a
> >>> prerequisite to doing anything in the interrupt handler, I don't
> >>> understand why we're just consuming it and ignoring the return value.
> >>> The above is in the delayed handling branch of the function, but the
> >>> normal non-delayed path would only go on to error_report() if the
> >>> notifier is not pending and then inject an interrupt anyway.  This all
> >>> seems rather suspicious and it's a unique pattern among the vfio
> >>> callers of this function.  Is there a more fundamental bug that this
> >>> function should perform this test once and return without doing
> >>> anything if it's called spuriously, ie. without a notifier pending?
> >>> Thanks,  
> >>
> >> Hum that's correct that other VFIO call sites do the check. My
> >> understanding was that this could not fail in this case as, if we
> >> entered the handler there was something to be cleared. In which
> >> situation can this fail?
> > 
> > I'm not sure what the right answer is, I see examples either way
> > looking outside of vfio code.  On one hand, maybe we never get called
> > spuriously, on the other if it's the callee's responsibility to drain
> > events from the fd and we have it readily accessible whether there were
> > any events pending, why would we inject an interrupt if the result that
> > we have in hand shows no pending events?  The overhead of returning
> > based on that result is minuscule.
> 
> I agree
> > 
> > qemu_set_fd_handler() is a wrapper for aio_set_fd_handler().  Stefan is
> > a possible defacto maintainer of some of the aio code.  Stefan, do you
> > have thoughts on whether callbacks from event notifier fds should
> > consider spurious events?  Thanks,
> 
> Indeed I saw that for instance block/nvme.c nvme_handle_event is not
> checking the result.
> 
> Let's wait for Stefan's answer ...

vfio_intp_interrupt() will always read a non-zero eventfd value, based
on these assumptions:

intp->interrupt is "readable" since vfio_intp_interrupt() is called by
the AioContext (event loop). "readable" does not guarantee that data can
actually be read because it also includes error events:

  new_node->pfd.events = (io_read ? G_IO_IN | G_IO_HUP | G_IO_ERR : 0);

However, I think we can exclude the error case for the VFIO interrupt
eventfds because there are no error cases for eventfds (unlike socket
disconnection, for example).

The other important assumption is that only one thread on the host is
monitoring the eventfd for activity.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 07/11] vfio/platform: Remove dead assignment in vfio_intp_interrupt()

[Auger Eric]

Hi Stefan,

On 8/18/20 2:54 PM, Stefan Hajnoczi wrote:
> On Thu, Aug 13, 2020 at 09:18:59PM +0200, Auger Eric wrote:
>> Hi Alex,
>>
>> On 8/13/20 9:15 PM, Alex Williamson wrote:
>>> On Thu, 13 Aug 2020 20:02:45 +0200
>>> Auger Eric <eric.auger@redhat.com> wrote:
>>>
>>>> Hi Alex,
>>>>
>>>> On 8/13/20 6:59 PM, Alex Williamson wrote:
>>>>> On Thu, 13 Aug 2020 15:37:08 +0800
>>>>> Chen Qun <kuhn.chenqun@huawei.com> wrote:
>>>>>   
>>>>>> Clang static code analyzer show warning:
>>>>>> hw/vfio/platform.c:239:9: warning: Value stored to 'ret' is never read
>>>>>>         ret = event_notifier_test_and_clear(intp->interrupt);
>>>>>>         ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>
>>>>>> Reported-by: Euler Robot <euler.robot@huawei.com>
>>>>>> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
>>>>>> ---
>>>>>> Cc: Alex Williamson <alex.williamson@redhat.com>
>>>>>> Cc: Eric Auger <eric.auger@redhat.com>
>>>>>> ---
>>>>>>  hw/vfio/platform.c | 2 +-
>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
>>>>>> index ac2cefc9b1..869ed2c39d 100644
>>>>>> --- a/hw/vfio/platform.c
>>>>>> +++ b/hw/vfio/platform.c
>>>>>> @@ -236,7 +236,7 @@ static void vfio_intp_interrupt(VFIOINTp *intp)
>>>>>>          trace_vfio_intp_interrupt_set_pending(intp->pin);
>>>>>>          QSIMPLEQ_INSERT_TAIL(&vdev->pending_intp_queue,
>>>>>>                               intp, pqnext);
>>>>>> -        ret = event_notifier_test_and_clear(intp->interrupt);
>>>>>> +        event_notifier_test_and_clear(intp->interrupt);
>>>>>>          return;
>>>>>>      }  
>>>>>
>>>>> Testing that an event is pending in our notifier is generally a
>>>>> prerequisite to doing anything in the interrupt handler, I don't
>>>>> understand why we're just consuming it and ignoring the return value.
>>>>> The above is in the delayed handling branch of the function, but the
>>>>> normal non-delayed path would only go on to error_report() if the
>>>>> notifier is not pending and then inject an interrupt anyway.  This all
>>>>> seems rather suspicious and it's a unique pattern among the vfio
>>>>> callers of this function.  Is there a more fundamental bug that this
>>>>> function should perform this test once and return without doing
>>>>> anything if it's called spuriously, ie. without a notifier pending?
>>>>> Thanks,  
>>>>
>>>> Hum that's correct that other VFIO call sites do the check. My
>>>> understanding was that this could not fail in this case as, if we
>>>> entered the handler there was something to be cleared. In which
>>>> situation can this fail?
>>>
>>> I'm not sure what the right answer is, I see examples either way
>>> looking outside of vfio code.  On one hand, maybe we never get called
>>> spuriously, on the other if it's the callee's responsibility to drain
>>> events from the fd and we have it readily accessible whether there were
>>> any events pending, why would we inject an interrupt if the result that
>>> we have in hand shows no pending events?  The overhead of returning
>>> based on that result is minuscule.
>>
>> I agree
>>>
>>> qemu_set_fd_handler() is a wrapper for aio_set_fd_handler().  Stefan is
>>> a possible defacto maintainer of some of the aio code.  Stefan, do you
>>> have thoughts on whether callbacks from event notifier fds should
>>> consider spurious events?  Thanks,
>>
>> Indeed I saw that for instance block/nvme.c nvme_handle_event is not
>> checking the result.
>>
>> Let's wait for Stefan's answer ...
> 
> vfio_intp_interrupt() will always read a non-zero eventfd value, based
> on these assumptions:
> 
> intp->interrupt is "readable" since vfio_intp_interrupt() is called by
> the AioContext (event loop). "readable" does not guarantee that data can
> actually be read because it also includes error events:
> 
>   new_node->pfd.events = (io_read ? G_IO_IN | G_IO_HUP | G_IO_ERR : 0);
> 
> However, I think we can exclude the error case for the VFIO interrupt
> eventfds because there are no error cases for eventfds (unlike socket
> disconnection, for example).
> 
> The other important assumption is that only one thread on the host is
> monitoring the eventfd for activity.

Thank for your the confirmation. So this patch should be safe.

Best Regards

Eric
> 
> Stefan
> 

Re: [PATCH 01/11] hw/arm/virt-acpi-build:Remove dead assignment in build_madt()

[Igor Mammedov]

On Thu, 13 Aug 2020 15:37:02 +0800
Chen Qun <kuhn.chenqun@huawei.com> wrote:

> Clang static code analyzer show warning:
> hw/arm/virt-acpi-build.c:641:5: warning: Value stored to 'madt' is never read
>     madt = acpi_data_push(table_data, sizeof *madt);
>     ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Reviewed-by: Igor Mammedov <imammedo@redhat.com>

> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: qemu-arm@nongnu.org
> ---
>  hw/arm/virt-acpi-build.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 91f0df7b13..f830f9b779 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -633,12 +633,11 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      int madt_start = table_data->len;
>      const MemMapEntry *memmap = vms->memmap;
>      const int *irqmap = vms->irqmap;
> -    AcpiMultipleApicTable *madt;
>      AcpiMadtGenericDistributor *gicd;
>      AcpiMadtGenericMsiFrame *gic_msi;
>      int i;
>  
> -    madt = acpi_data_push(table_data, sizeof *madt);
> +    acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
>  
>      gicd = acpi_data_push(table_data, sizeof *gicd);
>      gicd->type = ACPI_APIC_GENERIC_DISTRIBUTOR;

Re: [PATCH 01/11] hw/arm/virt-acpi-build:Remove dead assignment in build_madt()

[Michael S. Tsirkin]

On Thu, Aug 13, 2020 at 03:37:02PM +0800, Chen Qun wrote:
> Clang static code analyzer show warning:
> hw/arm/virt-acpi-build.c:641:5: warning: Value stored to 'madt' is never read
>     madt = acpi_data_push(table_data, sizeof *madt);
>     ^      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

feel free to merge through the trivial tree.

> ---
> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
> Cc: Peter Maydell <peter.maydell@linaro.org>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: qemu-arm@nongnu.org
> ---
>  hw/arm/virt-acpi-build.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 91f0df7b13..f830f9b779 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -633,12 +633,11 @@ build_madt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>      int madt_start = table_data->len;
>      const MemMapEntry *memmap = vms->memmap;
>      const int *irqmap = vms->irqmap;
> -    AcpiMultipleApicTable *madt;
>      AcpiMadtGenericDistributor *gicd;
>      AcpiMadtGenericMsiFrame *gic_msi;
>      int i;
>  
> -    madt = acpi_data_push(table_data, sizeof *madt);
> +    acpi_data_push(table_data, sizeof(AcpiMultipleApicTable));
>  
>      gicd = acpi_data_push(table_data, sizeof *gicd);
>      gicd->type = ACPI_APIC_GENERIC_DISTRIBUTOR;
> -- 
> 2.23.0

Re: [PATCH 05/11] hw/virtio/vhost-user:Remove dead assignment in scrub_shadow_regions()

[Michael S. Tsirkin]

On Thu, Aug 13, 2020 at 03:37:06PM +0800, Chen Qun wrote:
> Clang static code analyzer show warning:
> hw/virtio/vhost-user.c:606:9: warning: Value stored to 'mr' is never read
>         mr = vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);
>         ^    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>
> ---
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

feel free to merge through the trivial tree

> ---
>  hw/virtio/vhost-user.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index d7e2423762..9c5b4f7fbc 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -603,7 +603,7 @@ static void scrub_shadow_regions(struct vhost_dev *dev,
>       */
>      for (i = 0; i < dev->mem->nregions; i++) {
>          reg = &dev->mem->regions[i];
> -        mr = vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);
> +        vhost_user_get_mr_data(reg->userspace_addr, &offset, &fd);
>          if (fd > 0) {
>              ++fd_num;
>          }
> -- 
> 2.23.0

Re: [PATCH 06/11] hw/net/virtio-net:Remove redundant statement in virtio_net_rsc_tcp_ctrl_check()

[Michael S. Tsirkin]

On Thu, Aug 13, 2020 at 03:37:07PM +0800, Chen Qun wrote:
> Clang static code analyzer show warning:
> hw/net/virtio-net.c:2077:5: warning: Value stored to 'tcp_flag' is never read
>     tcp_flag &= VIRTIO_NET_TCP_FLAG;
>     ^           ~~~~~~~~~~~~~~~~~~~
> 
> The 'VIRTIO_NET_TCP_FLAG' is '0x3F'. The last ‘tcp_flag’ assignment statement is
>  the same as that of the first two statements.
> 
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Chen Qun <kuhn.chenqun@huawei.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

feel free to merge through the trivial tree.

> ---
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/virtio-net.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a1fe9e9285..cb0d27084c 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -2075,7 +2075,6 @@ static int virtio_net_rsc_tcp_ctrl_check(VirtioNetRscChain *chain,
>      tcp_flag = htons(tcp->th_offset_flags);
>      tcp_hdr = (tcp_flag & VIRTIO_NET_TCP_HDR_LENGTH) >> 10;
>      tcp_flag &= VIRTIO_NET_TCP_FLAG;
> -    tcp_flag = htons(tcp->th_offset_flags) & 0x3F;
>      if (tcp_flag & TH_SYN) {
>          chain->stat.tcp_syn++;
>          return RSC_BYPASS;
> -- 
> 2.23.0