RE: [PATCH] qtest: delete redundant qtest.h header files
> -Original Message- > From: Markus Armbruster [mailto:arm...@redhat.com] > Sent: Thursday, February 25, 2021 5:08 PM > To: Chenqun (kuhn) > Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org; lviv...@redhat.com; > th...@redhat.com; Zhanghailiang ; ganqixin > > Subject: Re: [PATCH] qtest: delete redundant qtest.h header files > > Chen Qun writes: > > > There are 23 files that include the "sysemu/qtest.h", but they do not > > use any qtest functions. > > > > Signed-off-by: Chen Qun > > The subject sounds as if you were deleting file include/sysemu/qtest.h, which > would be wrong. You're actually deleting inclusions. Suggest to say > > qtest: delete superfluous inclusions of qtest.h This subject is good to me. Thanks for your point ! I will change it in v2. > > or > > delete superfluous #include "sysemu/qtest.h" > > Perhaps the maintainer merging your patch can do that for you.
RE: [PATCH RESEND v2 5/7] migration/colo: Plug memleaks in colo_process_incoming_thread
Kindly ping!
Hi all,
It's a bug, though it's trivial.
Could you review it and add your queues or add trivial queues?
Thanks,
Chen Qun
> -Original Message-
> From: Chenqun (kuhn)
> Sent: Friday, October 23, 2020 2:12 PM
> To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org
> Cc: Pannengyuan ; lviv...@redhat.com;
> Zhanghailiang ; ganqixin
> ; Euler Robot ; Li Qiang
> ; Chenqun (kuhn) ; Juan
> Quintela ; Dr. David Alan Gilbert
>
> Subject: [PATCH RESEND v2 5/7] migration/colo: Plug memleaks in
> colo_process_incoming_thread
>
> From: Pan Nengyuan
>
> 'local_err' forgot to free in colo_process_incoming_thread error path.
> Fix that.
>
> Reported-by: Euler Robot
> Signed-off-by: Pan Nengyuan
> Reviewed-by: Li Qiang
> Signed-off-by: Chen Qun
> ---
> Cc: Hailiang Zhang
> Cc: Juan Quintela
> Cc: "Dr. David Alan Gilbert"
> ---
> migration/colo.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/migration/colo.c b/migration/colo.c index 3f1d3dfd95..7cc5a37192
> 100644
> --- a/migration/colo.c
> +++ b/migration/colo.c
> @@ -886,7 +886,6 @@ void *colo_process_incoming_thread(void *opaque)
> while (mis->state == MIGRATION_STATUS_COLO) {
> colo_wait_handle_message(mis, fb, bioc, _err);
> if (local_err) {
> -error_report_err(local_err);
> break;
> }
>
> @@ -922,6 +921,10 @@ out:
> qemu_fclose(fb);
> }
>
> +if (local_err) {
> +error_report_err(local_err);
> +}
> +
> /* Hope this not to be too long to loop here */
> qemu_sem_wait(>colo_incoming_sem);
> qemu_sem_destroy(>colo_incoming_sem);
> --
> 2.23.0
RE: [PATCH RESEND v2 3/7] elf2dmp/qemu_elf: Plug memleak in QEMU_Elf_init
> -Original Message-
> From: Laurent Vivier [mailto:laur...@vivier.eu]
> Sent: Monday, December 14, 2020 1:44 AM
> To: Chenqun (kuhn) ; qemu-devel@nongnu.org;
> qemu-triv...@nongnu.org
> Cc: lviv...@redhat.com; Zhanghailiang ;
> Viktor Prutyanov ; Li Qiang
> ; Pannengyuan ; ganqixin
> ; Euler Robot
> Subject: Re: [PATCH RESEND v2 3/7] elf2dmp/qemu_elf: Plug memleak in
> QEMU_Elf_init
>
> Le 23/10/2020 à 08:12, Chen Qun a écrit :
> > From: Pan Nengyuan
> >
> > Missing g_error_free in QEMU_Elf_init() error path. Fix that.
> >
> > Reported-by: Euler Robot
> > Signed-off-by: Pan Nengyuan
> > Reviewed-by: Viktor Prutyanov
> > Reviewed-by: Li Qiang
> > Signed-off-by: Chen Qun
> > ---
> > contrib/elf2dmp/qemu_elf.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/contrib/elf2dmp/qemu_elf.c b/contrib/elf2dmp/qemu_elf.c
> > index 0db7816586..b601b6d7ba 100644
> > --- a/contrib/elf2dmp/qemu_elf.c
> > +++ b/contrib/elf2dmp/qemu_elf.c
> > @@ -126,6 +126,7 @@ int QEMU_Elf_init(QEMU_Elf *qe, const char
> *filename)
> > qe->gmf = g_mapped_file_new(filename, TRUE, );
> > if (gerr) {
> > eprintf("Failed to map ELF dump file \'%s\'\n", filename);
> > +g_error_free(gerr);
> > return 1;
> > }
> >
> >
>
> Applied to my trivial-patches branch.
>
Hi Laurent,
Thank you for picked them up!
But, there are two patches that have not been picked up.
Maybe I should ping the corresponding Maintainer first.
The missing patches are：
[PATCH RESEND v2 2/7] qga/channel-posix: Plug memory leak in
ga_channel_write_all()
[PATCH RESEND v2 5/7] migration/colo: Plug memleaks in
colo_process_incoming_thread
Thanks,
Chen Qun
RE: [PATCH RESEND v2 2/7] qga/channel-posix: Plug memory leak in ga_channel_write_all()
Kindly ping!
Hi Michael,
It's a bug, though it's trivial.
Could you review it and add your queues or add trivial queues?
Thanks,
Chen Qun
> -Original Message-
> From: Chenqun (kuhn)
> Sent: Friday, October 23, 2020 2:12 PM
> To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org
> Cc: Pannengyuan ; lviv...@redhat.com;
> Zhanghailiang ; ganqixin
> ; Euler Robot ; Li Qiang
> ; Chenqun (kuhn) ; Michael
> Roth
> Subject: [PATCH RESEND v2 2/7] qga/channel-posix: Plug memory leak in
> ga_channel_write_all()
>
> From: Pan Nengyuan
>
> Missing g_error_free on error path in ga_channel_write_all(). Fix that.
>
> Reported-by: Euler Robot
> Signed-off-by: Pan Nengyuan
> Reviewed-by: Li Qiang
> Signed-off-by: Chen Qun
> ---
> Cc: Michael Roth
> ---
> qga/channel-posix.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/qga/channel-posix.c b/qga/channel-posix.c index
> 0373975360..8f3821af6d 100644
> --- a/qga/channel-posix.c
> +++ b/qga/channel-posix.c
> @@ -249,7 +249,7 @@ GIOStatus ga_channel_write_all(GAChannel *c, const
> gchar *buf, gsize size)
> buf += written;
> } else if (status != G_IO_STATUS_AGAIN) {
> g_warning("error writing to channel: %s", err->message);
> -return status;
> +goto out;
> }
> }
>
> @@ -261,6 +261,10 @@ GIOStatus ga_channel_write_all(GAChannel *c,
> const gchar *buf, gsize size)
> g_warning("error flushing channel: %s", err->message);
> }
>
> +out:
> +if (err) {
> +g_error_free(err);
> +}
> return status;
> }
>
> --
> 2.23.0
RE: [PATCH 12/12] configure: Compile with -Wimplicit-fallthrough=2
> -Original Message- > From: Thomas Huth [mailto:th...@redhat.com] > Sent: Friday, December 11, 2020 11:24 PM > To: Peter Maydell ; qemu-devel@nongnu.org > Cc: Chenqun (kuhn) ; Richard Henderson > ; Paolo Bonzini > Subject: [PATCH 12/12] configure: Compile with -Wimplicit-fallthrough=2 > > Coverity always complains about switch-case statements that fall through the > next one when there is no comment in between - which could indicate a > forgotten "break" statement. Instead of handling these issues after they have > been committed, it would be better to avoid them in the build process already. > Thus let's enable the -Wimplicit-fallthrough warning now. > > Signed-off-by: Thomas Huth Reviewed-by: Chen Qun Good job, we'll never see such warnings again. Thanks, Chen Qun > --- > configure | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/configure b/configure > index 18c26e0389..dc2bc3c2f0 100755 > --- a/configure > +++ b/configure > @@ -2007,6 +2007,7 @@ add_to warn_flags -Wempty-body add_to > warn_flags -Wnested-externs add_to warn_flags -Wendif-labels add_to > warn_flags -Wexpansion-to-defined > +add_to warn_flags -Wimplicit-fallthrough=2 > > nowarn_flags= > add_to nowarn_flags -Wno-initializer-overrides > -- > 2.27.0
RE: [PATCH 11/12] tests/fp: Do not emit implicit-fallthrough warnings in the softfloat tests
> -Original Message- > From: Thomas Huth [mailto:th...@redhat.com] > Sent: Friday, December 11, 2020 11:24 PM > To: Peter Maydell ; qemu-devel@nongnu.org > Cc: Chenqun (kuhn) ; Richard Henderson > ; Paolo Bonzini > Subject: [PATCH 11/12] tests/fp: Do not emit implicit-fallthrough warnings in > the softfloat tests > > The softfloat tests are external repositories, so we do not care about > implicit > fallthrough warnings in this code. > > Signed-off-by: Thomas Huth > --- Reviewed-by: Chen Qun The warnings are frequently generated for files in this directory. This is a good solution for the warnings. Thanks, Chen Qun > tests/fp/meson.build | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/tests/fp/meson.build b/tests/fp/meson.build index > 3d4fb00f9d..8d739c4d59 100644 > --- a/tests/fp/meson.build > +++ b/tests/fp/meson.build > @@ -27,6 +27,7 @@ tfdir = 'berkeley-testfloat-3/source' > sfinc = include_directories(sfdir / 'include', sfspedir) > > tfcflags = [ > + '-Wno-implicit-fallthrough', >'-Wno-strict-prototypes', >'-Wno-unknown-pragmas', >'-Wno-uninitialized', > @@ -209,6 +210,7 @@ libtestfloat = static_library( > ) > > sfcflags = [ > + '-Wno-implicit-fallthrough', >'-Wno-missing-prototypes', >'-Wno-redundant-decls', >'-Wno-return-type', > -- > 2.27.0
RE: [PATCH v2 0/5] fix uninitialized variable warning
Kindly ping! Hi folks, Patch 1 to Patch 4 are not in the queue. Could someone help pick them up? Patch1~Patch4: hw/rdma/rdma_backend: fix uninitialized variable warning in rdma_poll_cq() util/qemu-timer: fix uninitialized variable warning in timer_mod_anticipate_ns() util/qemu-timer: fix uninitialized variable warning for expire_time plugins/loader: fix uninitialized variable warning in plugin_reset_uninstall() Thanks, Chen Qun > -Original Message- > From: Chenqun (kuhn) > Sent: Wednesday, November 11, 2020 10:22 PM > To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org > Cc: peter.mayd...@linaro.org; Zhanghailiang > ; ganqixin ; > Chenqun (kuhn) > Subject: [PATCH v2 0/5] fix uninitialized variable warning > > Hi all, > There are some variables initialized warnings reported by the GCC_9.3 > compiler. > This serial has added some default values or changed the assignment places for > the variablies to fix them. > > v1->v2: > --patch1: Drop it base on Max Filippov comment. > --patch2->patch1: Add Marcel Apfelbaum and Yuval Shaia review comment. > --patch3->patch2: Add Philippe Mathieu-Daudé review comment. > --patch6->patch5: Add Philippe Mathieu-Daudé review comment. > > > Chen Qun (5): > hw/rdma/rdma_backend: fix uninitialized variable warning in > rdma_poll_cq() > util/qemu-timer: fix uninitialized variable warning in > timer_mod_anticipate_ns() > util/qemu-timer: fix uninitialized variable warning for expire_time > plugins/loader: fix uninitialized variable warning in > plugin_reset_uninstall() > migration: fix uninitialized variable warning in > migrate_send_rp_req_pages() > > hw/rdma/rdma_backend.c | 2 +- > migration/migration.c | 2 +- > plugins/loader.c | 2 +- > util/qemu-timer.c | 8 +++- > 4 files changed, 6 insertions(+), 8 deletions(-) > > -- > 2.27.0
RE: [PATCH v3 0/7] silence the compiler warnings
Kindly ping! Hi all, Patch 1 to Patch 5 are not in the queue. Could someone pick them up？ Patch1~Patch5: target/i386: silence the compiler warnings in gen_shiftd_rm_T1 hw/intc/arm_gicv3_kvm: silence the compiler warnings accel/tcg/user-exec: silence the compiler warnings target/sparc/translate: silence the compiler warnings target/sparc/win_helper: silence the compiler warnings Thanks, Chen Qun > -Original Message- > From: Chenqun (kuhn) > Sent: Monday, November 16, 2020 10:48 AM > To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org > Cc: Zhanghailiang ; ganqixin > ; Chenqun (kuhn) > Subject: [PATCH v3 0/7] silence the compiler warnings > > Hi folks, > This series fix some "fall through" warnings reported by GCC_9.3. They've > been reviewed for a long time. Some of these patchs may be important for > QEMU 5.2. > Such as the Patch6 miss a break statement. Others only add "fall through" > comments and may not have a negative impact for QEMU 5.2. > > > Thanks, > Chen Qun > > > Since v2: > - Patch3：Add Richard Henderson、Philippe Mathieu-Daudé and Thomas Huth > reviewed tag. > - Patch4: Laurent pull it to master, remove it. > - Patch6->Patch5: Add Richard Henderson and Philippe Mathieu-Daudé > reviewed tag. > - Patch7->Patch6: Add Thomas Huth reviewed tag and David Gibson acked tag. > - Patch8->Patch7: Tweak LOG_UNIMP message base on Thomas Huth > comment; > Add Philippe Mathieu-Daudé and Thomas Huth reviewed tag; Add David > Gibson acked tag. > > Since v1: > - Patch1: Add comments to explain the two case of fall through. > Addressed Richard Henderson and Thomas Huth review comment. > - Patch2: Addressed Peter Maydell review comment. > - Patch3: Add QEMU_NORETURN to cpu_exit_tb_from_sighandler() function to > avoid the compiler warnings. > - Patch4: Addressed Thomas Huth review comment. > - Patch5: Addressed Artyom Tarasenko and Philippe Mathieu-Daudé review > comment. > - Patch6: Combine the /* fall through */ to the preceding comments. > Addressed Artyom Tarasenko review comment. > - Patch7: Add a "break" statement here instead of /* fall through */ > comments. > - Patch8: Replace the TODO by a LOG_UNIMP call and add break statement > - Patch9: Discard this patch since a patch already exists for fix this > > issue(https://lore.kernel.org/qemu-devel/20200711154242.41222-1-ysato@us > ers) > > > Chen Qun (7): > target/i386: silence the compiler warnings in gen_shiftd_rm_T1 > hw/intc/arm_gicv3_kvm: silence the compiler warnings > accel/tcg/user-exec: silence the compiler warnings > target/sparc/translate: silence the compiler warnings > target/sparc/win_helper: silence the compiler warnings > ppc: Add a missing break for PPC6xx_INPUT_TBEN > target/ppc: replaced the TODO with LOG_UNIMP and add break for silence > warnings > > accel/tcg/user-exec.c | 3 ++- > hw/intc/arm_gicv3_kvm.c | 8 > hw/ppc/ppc.c | 1 + > target/i386/translate.c | 7 +-- > target/ppc/mmu_helper.c | 5 +++-- > target/sparc/translate.c | 2 +- > target/sparc/win_helper.c | 2 +- > 7 files changed, 21 insertions(+), 7 deletions(-) > > -- > 2.27.0
RE: [PATCH] bugfix: hostmem: Free host_nodes list right after visited
> -Original Message- > From: zhukeqian > Sent: Wednesday, December 9, 2020 10:57 AM > To: Peter Maydell ; Igor Mammedov > ; Eduardo Habkost > Cc: qemu-devel@nongnu.org; qemu-...@nongnu.org; Wanghaibin (D) > ; Chenqun (kuhn) > ; Chenzhendong (alex) > ; zhukeqian > Subject: [PATCH] bugfix: hostmem: Free host_nodes list right after visited > > In host_memory_backend_get_host_nodes, we build host_nodes list and > output it to v (a StringOutputVisitor) but forget to free the list. This > fixes the > memory leak. > > The memory leak stack: > > ==qemu-kvm==209357==ERROR: LeakSanitizer: detected memory leaks Direct > leak of 32 byte(s) in 2 object(s) allocated from: > #0 0xfffe430e3393 in __interceptor_calloc > (/lib64/libasan.so.4+0xd3393) ??:? > #1 0xfffe41d58b9b in g_malloc0 (/lib64/libglib-2.0.so.0+0x58b9b) ??:? > #2 0xaaac0cdb6e43 (/usr/libexec/qemu-kvm+0xe16e43) > backends/hostmem.c:94 > #3 0xaaac0d2edf83 (/usr/libexec/qemu-kvm+0x134df83) qom/object.c:1478 > #4 0xaaac0c976513 (/usr/libexec/qemu-kvm+0x9d6513) > hw/core/machine-qmp-cmds.c:312 > #5 0xaaac0d2e980b (/usr/libexec/qemu-kvm+0x134980b) qom/object.c:1001 > #6 0xaaac0c97779b (/usr/libexec/qemu-kvm+0x9d779b) > hw/core/machine-qmp-cmds.c:328 (discriminator 1) > #7 0xaaac0d26ed3f (/usr/libexec/qemu-kvm+0x12ced3f) > qapi/qapi-commands-machine.c:327 > #8 0xaaac0d43d647 (/usr/libexec/qemu-kvm+0x149d647) > qapi/qmp-dispatch.c:147 > #9 0xaaac0d21f74b (/usr/libexec/qemu-kvm+0x127f74b) monitor/qmp.c:120 > #10 0xaaac0d22074b (/usr/libexec/qemu-kvm+0x128074b) > monitor/qmp.c:209 (discriminator 4) > #11 0xaaac0d4daefb (/usr/libexec/qemu-kvm+0x153aefb) util/async.c:117 > #12 0xaaac0d4e30fb (/usr/libexec/qemu-kvm+0x15430fb) > util/aio-posix.c:459 > #13 0xaaac0d4dac8f (/usr/libexec/qemu-kvm+0x153ac8f) util/async.c:268 > #14 0xfffe41d52a6b in g_main_context_dispatch > (/lib64/libglib-2.0.so.0+0x52a6b) ??:? > #15 0xaaac0d4e0e97 (/usr/libexec/qemu-kvm+0x1540e97) > util/main-loop.c:218 > #16 0xaaac0cd9bfa7 (/usr/libexec/qemu-kvm+0xdfbfa7) vl.c:1791 > #17 0xaaac0c823bc3 (/usr/libexec/qemu-kvm+0x883bc3) vl.c:4473 > #18 0xfffe40ab3ebf in __libc_start_main (/lib64/libc.so.6+0x23ebf) ??:? > #19 0xaaac0c82ed5f (/usr/libexec/qemu-kvm+0x88ed5f) ??:? > SUMMARY: AddressSanitizer: 32 byte(s) leaked in 2 allocation(s). > > Fixes: 4cf1b76bf1e2 (hostmem: add properties for NUMA memory policy) > Reported-by: Euler Robot > Signed-off-by: Keqian Zhu > --- Tested-by: Chen Qun By the way, the bug detailed stack is as follows: Direct leak of 32 byte(s) in 2 object(s) allocated from: #0 0xfffda30b3393 in __interceptor_calloc (/usr/lib64/libasan.so.4+0xd3393) #1 0xfffda1d28b9b in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x58b9b) #2 0xaaab05ca6e43 in host_memory_backend_get_host_nodes backends/hostmem.c:94 #3 0xaaab061ddf83 in object_property_get_uint16List qom/object.c:1478 #4 0xaaab05866513 in query_memdev hw/core/machine-qmp-cmds.c:312 #5 0xaaab061d980b in do_object_child_foreach qom/object.c:1001 #6 0xaaab0586779b in qmp_query_memdev hw/core/machine-qmp-cmds.c:328 #7 0xaaab0615ed3f in qmp_marshal_query_memdev qapi/qapi-commands-machine.c:327 #8 0xaaab0632d647 in do_qmp_dispatch qapi/qmp-dispatch.c:147 #9 0xaaab0632d647 in qmp_dispatch qapi/qmp-dispatch.c:190 #10 0xaaab0610f74b in monitor_qmp_dispatch monitor/qmp.c:120 #11 0xaaab0611074b in monitor_qmp_bh_dispatcher monitor/qmp.c:209 #12 0xaaab063caefb in aio_bh_poll util/async.c:117 #13 0xaaab063d30fb in aio_dispatch util/aio-posix.c:459 #14 0xaaab063cac8f in aio_ctx_dispatch util/async.c:268 #15 0xfffda1d22a6b in g_main_context_dispatch (/usr/lib64/libglib-2.0.so.0+0x52a6b) #16 0xaaab063d0e97 in glib_pollfds_poll util/main-loop.c:218 #17 0xaaab063d0e97 in os_host_main_loop_wait util/main-loop.c:241 #18 0xaaab063d0e97 in main_loop_wait util/main-loop.c:517 #19 0xaaab05c8bfa7 in main_loop /root/rpmbuild/BUILD/qemu-4.1.0/vl.c:1791 #20 0xaaab05713bc3 in main /root/rpmbuild/BUILD/qemu-4.1.0/vl.c:4473 #21 0xfffda0a83ebf in __libc_start_main (/usr/lib64/libc.so.6+0x23ebf) #22 0xaaab0571ed5f (aarch64-softmmu/qemu-system-aarch64+0x88ed5f) SUMMARY: AddressSanitizer: 32 byte(s) leaked in 2 allocation(s). > backends/hostmem.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/backends/hostmem.c b/backends/hostmem.c index > 4bde00e8e7..9f9ac95edd 100644 > --- a/backends/hostmem.c > +++ b/backends/hostmem.c > @@ -105,6 +105,7 @@ host_memory_backend_get_host_nodes(Object *obj, > Visitor *v, const char *name, > > ret: > visit_type_uint16List(v, name, _nodes, errp); > +qapi_free_uint16List(host_nodes); > } > > static void > -- > 2.23.0
RE: [PATCH RESEND v2 0/7] some memleak trivial patchs
Kindly ping! Hi all, This series has been sunk for a long time. Could someone pick them up？ Six patches are sunk here: qga/channel-posix: Plug memory leak in ga_channel_write_all() elf2dmp/qemu_elf: Plug memleak in QEMU_Elf_init elf2dmp/pdb: Plug memleak in pdb_init_from_file migration/colo: Plug memleaks in colo_process_incoming_thread blockdev: Fix a memleak in drive_backup_prepare() block/file-posix: fix a possible undefined behavior Thanks, Chen Qun > -Original Message- > From: Chenqun (kuhn) > Sent: Friday, October 30, 2020 6:23 PM > To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org > Cc: Pannengyuan ; lviv...@redhat.com; > Zhanghailiang ; ganqixin > > Subject: RE: [PATCH RESEND v2 0/7] some memleak trivial patchs > > Ping! > > Hi all, > The patche2 ~7 have been reviewed for some time. > Could someone please pick it up？ > > Thanks, > Chen Qun > > > -Original Message- > > From: Chenqun (kuhn) > > Sent: Friday, October 23, 2020 2:12 PM > > To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org > > Cc: Pannengyuan ; lviv...@redhat.com; > > Zhanghailiang ; ganqixin > > ; Chenqun (kuhn) > > Subject: [PATCH RESEND v2 0/7] some memleak trivial patchs > > > > Hi all, > > > > Here are some memory leak patches reported by EulerRobot. > > Some patch submissions have been unattended for a while and I resend > them. > > > > Thanks, > > Chen Qun > > > > > > Chen Qun (1): > > tests/migration: fix memleak in wait_command/wait_command_fd > > > > Pan Nengyuan (6): > > qga/channel-posix: Plug memory leak in ga_channel_write_all() > > elf2dmp/qemu_elf: Plug memleak in QEMU_Elf_init > > elf2dmp/pdb: Plug memleak in pdb_init_from_file > > migration/colo: Plug memleaks in colo_process_incoming_thread > > blockdev: Fix a memleak in drive_backup_prepare() > > block/file-posix: fix a possible undefined behavior > > > > block/file-posix.c | 2 +- > > blockdev.c | 1 + > > contrib/elf2dmp/pdb.c | 1 + > > contrib/elf2dmp/qemu_elf.c | 1 + > > migration/colo.c| 5 - > > qga/channel-posix.c | 6 +- > > tests/qtest/migration-helpers.c | 16 > > 7 files changed, 25 insertions(+), 7 deletions(-) > > > > -- > > 2.23.0
RE: Proposal for a regular upstream performance testing
> On Tue, Dec 01, 2020 at 01:06:35PM +0100, Lukáš Doktor wrote: > > Dne 01. 12. 20 v 11:22 Stefan Hajnoczi napsal(a): > > > On Tue, Dec 01, 2020 at 09:05:49AM +0100, Lukáš Doktor wrote: > > > > Dne 30. 11. 20 v 14:25 Stefan Hajnoczi napsal(a): > > > > > On Thu, Nov 26, 2020 at 09:10:14AM +0100, Lukáš Doktor wrote: > > > > > What is the minimal environment needed for bare metal hosts? > > > > > > > > > > > > > Not sure what you mean by that. For provisioning I have a beaker plugin, > other plugins can be added if needed. Even without beaker one can also provide > an installed machine and skip the provisioning step. Runperf would then only > apply the profiles (including fetching the VM images from public sources) and > run the tests on them. Note that for certain profiles might need to reboot the > machine and in such case the tested machine can not be the one running > run-perf, other profiles can use the current machine but it's still not a > very good > idea as the additional overhead might spoil the results. > > > > > > > > Note that for a very simple issue which do not require a special setup > > > > I am > usually just running a custom VM on my laptop and use a Localhost profile on > that VM, which basically results in testing that custom-setup VM's > performance. It's dirty but very fast for the first-level check. > > > > > > I was thinking about reprovisioning the machine to ensure each run > > > starts from the same clean state. This requires reprovisioning. > > > > > > Stefan > > > > > > > Sure, I probably shorten it unnecessary too much. In my setup I am using a > beaker plugin that reprovisions the machine. As for others they can either use > beaker plugin as well or they can just prepare the machine prior to the > execution as described in the previous paragraph. > > FWIW I'm not aware of anyone else taking on this work upstream. Whatever > you can do for upstream will be the QEMU disk/network/etc preformance > regression testing effort. Someone might show up with engineering time and > machine resources, but the chance is low. Maybe we could provide CI platforms, machine resources and some other possible help : ) Currently, there is only a short readme, we will complete the documents as soon as possible. https://gitee.com/wu_fengguang/compass-ci/blob/master/README.en.md Thanks, Chen Qun
RE: Proposal for a regular upstream performance testing
> -Original Message- > From: Qemu-devel > [mailto:qemu-devel-bounces+kuhn.chenqun=huawei@nongnu.org] On > Behalf Of Luká? Doktor > Sent: Thursday, November 26, 2020 4:10 PM > To: QEMU Developers > Cc: Charles Shih ; Aleksandar Markovic > ; Stefan Hajnoczi > > Subject: Proposal for a regular upstream performance testing > > Hello guys, > > I had been around qemu on the Avocado-vt side for quite some time and a while > ago I shifted my focus on performance testing. Currently I am not aware of any > upstream CI that would continuously monitor the upstream qemu performance > and I'd like to change that. There is a lot to cover so please bear with me. > > Goal > > > The goal of this initiative is to detect system-wide performance regressions > as > well as improvements early, ideally pin-point the individual commits and > notify > people that they should fix things. All in upstream and ideally with least > human > interaction possible. > > Unlike the recent work of Ahmed Karaman's > https://ahmedkrmn.github.io/TCG-Continuous-Benchmarking/ my aim is on the > system-wide performance inside the guest (like fio, uperf, ...) > > Tools > = > > In house we have several different tools used by various teams and I bet there > are tons of other tools out there that can do that. I can not speak for all > teams > but over the time many teams at Red Hat have come to like pbench > https://distributed-system-analysis.github.io/pbench/ to run the tests and > produce machine readable results and use other tools (Ansible, scripts, ...) > to > provision the systems and to generate the comparisons. > > As for myself I used python for PoC and over the last year I pushed hard to > turn > it into a usable and sensible tool which I'd like to offer: > https://run-perf.readthedocs.io/en/latest/ anyway I am open to suggestions > and comparisons. As I am using it downstream to watch regressions I do plan > on keep developing the tool as well as the pipelines (unless a better tool is > found that would replace it or it's parts). > > How > === > > This is a tough question. Ideally this should be a standalone service that > would > only notify the author of the patch that caused the change with a bunch of > useful data so they can either address the issue or just be aware of this > change > and mark it as expected. > > Ideally the community should have a way to also issue their custom builds in > order to verify their patches so they can debug and address issues better than > just commit to qemu-master. > > The problem with those is that we can not simply use travis/gitlab/... > machines > for running those tests, because we are measuring in-guest actual > performance. We can't just stop the time when the machine decides to > schedule another container/vm. I briefly checked the public bare-metal > offerings like rackspace but these are most probably not sufficient either > because (unless I'm wrong) they only give you a machine but it is not > guaranteed that it will be the same machine the next time. If we are to > compare the results we don't need just the same model, we really need the > very same machine. Any change to the machine might lead to a significant > difference (disk replacement, even firmware update...). Hi Lukáš, It's nice to see a discussion of QEMU performance topic. If you have a need for CI platform and physical machine environments, maybe compass-ci can help you. Compass-ci is an open CI platform of the openEuler community and is growing. Here's a brief reame： https://gitee.com/wu_fengguang/compass-ci/blob/master/README.en.md Thanks, Chen Qun > > Solution 1 > -- > > Doing this for downstream builds I can start doing this for upstream as well. > At > this point I can offer a single pipeline watching only changes in qemu > (downstream we are checking distro/kernel changes as well but that would > require too much time at this point) on a single x86_64 machine. I can not > offer > a public access to the testing machine, not even checking custom builds > (unless > someone provides me a publicly available machine(s) that I would use for > this). > What I can offer is running the checks on the latest qemu master, publishing > the reports, bisecting issues and notifying people about the changes. An > example of a report can be found here: > https://drive.google.com/file/d/1V2w7QpSuybNusUaGxnyT5zTUvtZDOfsb/view > ?usp=sharing a documentation of the format is here: > https://run-perf.readthedocs.io/en/latest/scripts.html#html-results I can also > attach the raw pbench results if needed (as well as details about the tests > that > were executed and the params and other details). > > Currently the covered scenarios would be a default libvirt machine with qcow2 > storage and tuned libvirt machine (cpus, hugepages, numa, raw disk...) running > fio, uperf and linpack on the latest GA RHEL. In the future I can add/tweak > the > scenarios as well as tests selection
RE: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree need to be initialized
> > On Thu, 19 Nov 2020 at 08:35, Chenqun (kuhn)
>
> > wrote:
> > >
> > > > -Original Message-
> > > > > static void tim_add_test(const char *name, const TestData *td,
> > > > > GTestDataFunc fn) {
> > > > > -g_autofree char *full_name;
> > > > > -
> > > > > -full_name =
> > g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s",
> > > > > -tim_index(td->tim),
> > > > timer_index(td->timer),
> > > > > -name);
> > > > > +g_autofree char *full_name = g_strdup_printf(
> > > > > +"npcm7xx_timer/tim[%d]/timer[%d]/%s",
> tim_index(td->tim),
> > > > > +timer_index(td->timer), name);
> > > >
> > > > Which compiler is so unintelligent that it cannot see that a
> > > > declaration immediately followed by an assignment must always
> > > > initialize
> > the variable ???
> > > >
> > > Hi Peter,
> > > Glib requires that all g_auto* macros must be initialized.
> > >
> > > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.ht
> > > ml
> > > #g-autofree
> >
> > Yes, and we initialize it with the "full_name = ..." line.
> > The g_autofree documentation says "this macro has similar constraints
> > as g_autoptr()", and the g_autoptr() documentation says "You must
> > initialise the variable in some way — either by use of an initialiser
> > or by ensuring that it is assigned to unconditionally before it goes out of
> scope."
> >
> > In this case the test code is doing the second of those two things.
>
> Emm, maybe I didn't get it right. I've tried something as following:
> There are three pieces of code complied in GCC9.3 and GCC7.3.
> Code1：
>g_autofree char *full_name;
>full_name = g_strdup_printf("npcm7xx_timer");
I guess the GCC thinks that g_strdup_printf() may fail to be executed. After
the function fails to be executed, and not give a NULL return value.
If this occurs, g_autofree will still executes g_free(*pp). So, an warning is
generated in the Code1 case.
In the example code provided by g-autoptr() documentation, the value assignment
statement of the 'dirname' variable uses g_variant_lookup_value().
If the g_variant_lookup_value() function fails to be executed, a NULL value is
returned. So, this example code is safe.
https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autoptr
>
> Code2:
>g_autofree char *full_name = g_strdup_printf("npcm7xx_timer");
In this case, if g_strdup_printf() fails, the variable definition also fails.
So, the Code2 is safe.
>
> Code3：
>g_autofree char *full_name;
>full_name = NULL;
>
> The result is as follows:
> Code1: An warnig is generated for GCC7.3 or GCC9.3.
>
> Code2 and Code3: no any warnig whether compiler is GCC7.3 or GCC9.3.
>
> I cannot explain why the Code1 warning is generated. But it always generates
> warning on the GCC compiler.
The above analysis is just my own guess. That may not be the truth.
Thanks,
Chen Qun
RE: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree need to be initialized
> -Original Message-
> From: Peter Maydell [mailto:peter.mayd...@linaro.org]
> Sent: Thursday, November 19, 2020 4:39 PM
> To: Chenqun (kuhn)
> Cc: QEMU Developers ; QEMU Trivial
> ; Hao Wu ; Havard
> Skinnemoen ; Zhanghailiang
> ; Thomas Huth ;
> Laurent Vivier ; Euler Robot
> Subject: Re: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree
> need to be initialized
>
> On Thu, 19 Nov 2020 at 08:35, Chenqun (kuhn)
> wrote:
> >
> > > -Original Message-
> > > > static void tim_add_test(const char *name, const TestData *td,
> > > > GTestDataFunc fn) {
> > > > -g_autofree char *full_name;
> > > > -
> > > > -full_name =
> g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s",
> > > > -tim_index(td->tim),
> > > timer_index(td->timer),
> > > > -name);
> > > > +g_autofree char *full_name = g_strdup_printf(
> > > > +"npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim),
> > > > +timer_index(td->timer), name);
> > >
> > > Which compiler is so unintelligent that it cannot see that a
> > > declaration immediately followed by an assignment must always initialize
> the variable ???
> > >
> > Hi Peter,
> > Glib requires that all g_auto* macros must be initialized.
> >
> > https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html
> > #g-autofree
>
> Yes, and we initialize it with the "full_name = ..." line.
> The g_autofree documentation says "this macro has similar constraints as
> g_autoptr()", and the g_autoptr() documentation says "You must initialise the
> variable in some way — either by use of an initialiser or by ensuring that it
> is
> assigned to unconditionally before it goes out of scope."
>
> In this case the test code is doing the second of those two things.
Emm, maybe I didn't get it right. I've tried something as following:
There are three pieces of code complied in GCC9.3 and GCC7.3.
Code1：
g_autofree char *full_name;
full_name = g_strdup_printf("npcm7xx_timer");
Code2:
g_autofree char *full_name = g_strdup_printf("npcm7xx_timer");
Code3：
g_autofree char *full_name;
full_name = NULL;
The result is as follows:
Code1: An warnig is generated for GCC7.3 or GCC9.3.
Code2 and Code3: no any warnig whether compiler is GCC7.3 or GCC9.3.
I cannot explain why the Code1 warning is generated. But it always generates
warning on the GCC compiler.
Thanks,
Chen Qun
RE: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree need to be initialized
> -Original Message-
> From: Peter Maydell [mailto:peter.mayd...@linaro.org]
> Sent: Thursday, November 19, 2020 4:26 PM
> To: Chenqun (kuhn)
> Cc: QEMU Developers ; QEMU Trivial
> ; Hao Wu ; Havard
> Skinnemoen ; Zhanghailiang
> ; Thomas Huth ;
> Laurent Vivier ; Euler Robot
> Subject: Re: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree
> need to be initialized
>
> On Wed, 18 Nov 2020 at 11:57, Chen Qun
> wrote:
> >
> > According to the glib function requirements, we need initialise the
> > variable. Otherwise there will be compilation warnings:
> >
> > glib-autocleanups.h:28:3: warning: ‘full_name’ may be used
> > uninitialized in this function [-Wmaybe-uninitialized]
> >28 | g_free (*pp);
> > | ^~~~
> >
> > static void tim_add_test(const char *name, const TestData *td,
> > GTestDataFunc fn) {
> > -g_autofree char *full_name;
> > -
> > -full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s",
> > -tim_index(td->tim),
> timer_index(td->timer),
> > -name);
> > +g_autofree char *full_name = g_strdup_printf(
> > +"npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim),
> > +timer_index(td->timer), name);
>
> Which compiler is so unintelligent that it cannot see that a declaration
> immediately followed by an assignment must always initialize the variable ???
>
Hi Peter,
Glib requires that all g_auto* macros must be initialized.
https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree
Thanks,
Chen Qun
RE: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree need to be initialized
> -Original Message-
> From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com]
> Sent: Wednesday, November 18, 2020 8:14 PM
> To: Chenqun (kuhn) ; qemu-devel@nongnu.org;
> qemu-triv...@nongnu.org
> Cc: lviv...@redhat.com; peter.mayd...@linaro.org; th...@redhat.com;
> Zhanghailiang ;
> hskinnem...@google.com; wuhao...@google.com; Euler Robot
>
> Subject: Re: [PATCH-for-5.2? 1/2] tests/qtest: variable defined by g_autofree
> need to be initialized
>
> On 11/18/20 12:56 PM, Chen Qun wrote:
> > According to the glib function requirements, we need initialise the
> > variable. Otherwise there will be compilation warnings:
> >
> > glib-autocleanups.h:28:3: warning: ‘full_name’ may be used
> > uninitialized in this function [-Wmaybe-uninitialized]
> >28 | g_free (*pp);
> > | ^~~~
> >
> > Reported-by: Euler Robot
> > Signed-off-by: Chen Qun
> > ---
> > tests/qtest/npcm7xx_timer-test.c | 8 +++-
> > 1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/tests/qtest/npcm7xx_timer-test.c
> > b/tests/qtest/npcm7xx_timer-test.c
> > index f08b0cd62a..83774a5b90 100644
> > --- a/tests/qtest/npcm7xx_timer-test.c
> > +++ b/tests/qtest/npcm7xx_timer-test.c
> > @@ -512,11 +512,9 @@ static void
> test_disable_on_expiration(gconstpointer test_data)
> > */
> > static void tim_add_test(const char *name, const TestData *td,
> > GTestDataFunc fn) {
>
> Or:
>
> > -g_autofree char *full_name;
> +g_autofree char *full_name = NULL;
>
Yes, this also meets the glib requirements.
But, the assignment statement following is not complex, so we could do both of
variable definition and assignment in a statement.
Thanks,
Chen Qun
> > -
> > -full_name = g_strdup_printf("npcm7xx_timer/tim[%d]/timer[%d]/%s",
> > -tim_index(td->tim),
> timer_index(td->timer),
> > -name);
> > +g_autofree char *full_name = g_strdup_printf(
> > +"npcm7xx_timer/tim[%d]/timer[%d]/%s", tim_index(td->tim),
> > +timer_index(td->timer), name);
> > qtest_add_data_func(full_name, td, fn); }
> >
> >
RE: [PATCH-for-5.2 v2] hw/intc: fix heap-buffer-overflow in rxicu_realize()
Kindly ping！
Maybe it should be need for version 5.2.
> -Original Message-
> From: Chenqun (kuhn)
> Sent: Wednesday, November 11, 2020 10:18 PM
> To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org
> Cc: Zhanghailiang ; ganqixin
> ; f4...@amsat.org; Chenqun (kuhn)
> ; Peter Maydell ;
> Euler Robot ; Yoshinori Sato
>
> Subject: [PATCH-for-5.2 v2] hw/intc: fix heap-buffer-overflow in
> rxicu_realize()
>
> When 'j = icu->nr_sense – 1', the 'j < icu->nr_sense' condition is true, then
> 'j =
> icu->nr_sense', the'icu->init_sense[j]' has out-of-bounds access.
>
> The asan showed stack:
> ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60404d7d at
> pc 0x55852cd26a76 bp 0x7ffe39f26200 sp 0x7ffe39f261f0 READ of size 1 at
> 0x60404d7d thread T0
> #0 0x55852cd26a75 in rxicu_realize ../hw/intc/rx_icu.c:311
> #1 0x55852cf075f7 in device_set_realized ../hw/core/qdev.c:886
> #2 0x55852cd4a32f in property_set_bool ../qom/object.c:2251
> #3 0x55852cd4f9bb in object_property_set ../qom/object.c:1398
> #4 0x55852cd54f3f in
> object_property_set_qobject ../qom/qom-qobject.c:28
> #5 0x55852cd4fc3f in object_property_set_bool ../qom/object.c:1465
> #6 0x55852cbf0b27 in register_icu ../hw/rx/rx62n.c:156
> #7 0x55852cbf12a6 in rx62n_realize ../hw/rx/rx62n.c:261
> #8 0x55852cf075f7 in device_set_realized ../hw/core/qdev.c:886
> #9 0x55852cd4a32f in property_set_bool ../qom/object.c:2251
> #10 0x55852cd4f9bb in object_property_set ../qom/object.c:1398
> #11 0x55852cd54f3f in
> object_property_set_qobject ../qom/qom-qobject.c:28
> #12 0x55852cd4fc3f in object_property_set_bool ../qom/object.c:1465
> #13 0x55852cbf1a85 in rx_gdbsim_init ../hw/rx/rx-gdbsim.c:109
> #14 0x55852cd22de0 in qemu_init ../softmmu/vl.c:4380
> #15 0x55852ca57088 in main ../softmmu/main.c:49
> #16 0x7feefafa5d42 in __libc_start_main (/lib64/libc.so.6+0x26d42)
>
> Add the 'ice->src[i].sense' initialize to the default value, and then process
> init_sense array to identify which irqs should be level-triggered.
>
> Suggested-by: Peter Maydell
> Reported-by: Euler Robot
> Signed-off-by: Chen Qun
> ---
> Cc: Yoshinori Sato
>
> v1->v2:
> Modify the code logic based on Peter's suggestions.
> We first initialize everything to the default before processing the init_sense
> array to identify which irqs should be level-triggered.
> ---
> hw/intc/rx_icu.c | 18 --
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/hw/intc/rx_icu.c b/hw/intc/rx_icu.c index 94e17a9dea..e5c01807b9
> 100644
> --- a/hw/intc/rx_icu.c
> +++ b/hw/intc/rx_icu.c
> @@ -300,22 +300,20 @@ static const MemoryRegionOps icu_ops = { static
> void rxicu_realize(DeviceState *dev, Error **errp) {
> RXICUState *icu = RX_ICU(dev);
> -int i, j;
> +int i;
>
> if (icu->init_sense == NULL) {
> qemu_log_mask(LOG_GUEST_ERROR,
>"rx_icu: trigger-level property must be set.");
> return;
> }
> -for (i = j = 0; i < NR_IRQS; i++) {
> -if (icu->init_sense[j] == i) {
> -icu->src[i].sense = TRG_LEVEL;
> -if (j < icu->nr_sense) {
> -j++;
> -}
> -} else {
> -icu->src[i].sense = TRG_PEDGE;
> -}
> +
> +for (i = 0; i < NR_IRQS; i++) {
> +icu->src[i].sense = TRG_PEDGE;
> +}
> +for (i = 0; i < icu->nr_sense; i++) {
> +uint8_t irqno = icu->init_sense[i];
> +icu->src[irqno].sense = TRG_LEVEL;
> }
> icu->req_irq = -1;
> }
> --
> 2.27.0
RE: [PATCH v2] json: Fix a memleak in parse_pair()
> -Original Message-
> From: Chenzhendong (alex)
> Sent: Friday, November 13, 2020 10:55 PM
> To: arm...@redhat.com
> Cc: Chenzhendong (alex) ; qemu-devel@nongnu.org;
> qemu-triv...@nongnu.org; Zhanghailiang ;
> Chenqun (kuhn)
> Subject: [PATCH v2] json: Fix a memleak in parse_pair()
>
> In qobject_type(), NULL is returned when the 'QObject' returned from
> parse_value() is not of QString type, and this 'QObject' memory will leaked.
> So we need to first cache the 'QObject' returned from parse_value(), and
> finally
> free 'QObject' memory at the end of the function.
> Also, we add a testcast about invalid dict key.
>
> The memleak stack is as follows:
> Direct leak of 32 byte(s) in 1 object(s) allocated from:
> #0 0xfffe4b3c34fb in __interceptor_malloc (/lib64/libasan.so.4+0xd34fb)
> #1 0xfffe4ae48aa3 in g_malloc (/lib64/libglib-2.0.so.0+0x58aa3)
> #2 0xaaab3557d9f7 in qnum_from_int
> /Images/source_org/qemu_master/qemu/qobject/qnum.c:25
> #3 0xaaab35584d23 in parse_literal
> /Images/source_org/qemu_master/qemu/qobject/json-parser.c:511
> #4 0xaaab35584d23 in parse_value
> /Images/source_org/qemu_master/qemu/qobject/json-parser.c:554
> #5 0xaaab35583d77 in parse_pair
> /Images/source_org/qemu_master/qemu/qobject/json-parser.c:270
> #6 0xaaab355845db in parse_object
> /Images/source_org/qemu_master/qemu/qobject/json-parser.c:327
> #7 0xaaab355845db in parse_value
> /Images/source_org/qemu_master/qemu/qobject/json-parser.c:546
> #8 0xaaab35585b1b in json_parser_parse
> /Images/source_org/qemu_master/qemu/qobject/json-parser.c:580
> #9 0xaaab35583703 in json_message_process_token
> /Images/source_org/qemu_master/qemu/qobject/json-streamer.c:92
> #10 0xaaab355ddccf in json_lexer_feed_char
> /Images/source_org/qemu_master/qemu/qobject/json-lexer.c:313
> #11 0xaaab355de0eb in json_lexer_feed
> /Images/source_org/qemu_master/qemu/qobject/json-lexer.c:350
> #12 0xaaab354aff67 in tcp_chr_read
> /Images/source_org/qemu_master/qemu/chardev/char-socket.c:525
> #13 0xfffe4ae429db in g_main_context_dispatch
> (/lib64/libglib-2.0.so.0+0x529db)
> #14 0xfffe4ae42d8f (/lib64/libglib-2.0.so.0+0x52d8f)
> #15 0xfffe4ae430df in g_main_loop_run (/lib64/libglib-2.0.so.0+0x530df)
> #16 0xaaab34d70bff in iothread_run
> /Images/source_org/qemu_master/qemu/iothread.c:82
> #17 0xaaab3559d71b in qemu_thread_start
> /Images/source_org/qemu_master/qemu/util/qemu-thread-posix.c:519
>
> Fixes: 532fb5328473 ("qapi: Make more of qobject_to()")
> Reported-by: Euler Robot
> Signed-off-by: Alex Chen
> Signed-off-by: Chen Qun
> Signed-off-by: Markus Armbruster
> ---
> qobject/json-parser.c | 12 ++--
> tests/check-qjson.c | 9 +
> 2 files changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/qobject/json-parser.c b/qobject/json-parser.c index
> d083810d37..c0f521b56b 100644
> --- a/qobject/json-parser.c
> +++ b/qobject/json-parser.c
> @@ -257,8 +257,9 @@ static JSONToken
> *parser_context_peek_token(JSONParserContext *ctxt)
> */
> static int parse_pair(JSONParserContext *ctxt, QDict *dict) {
> +QObject *key_obj = NULL;
> +QString *key;
> QObject *value;
> -QString *key = NULL;
> JSONToken *peek, *token;
>
> peek = parser_context_peek_token(ctxt); @@ -267,7 +268,8 @@ static
> int parse_pair(JSONParserContext *ctxt, QDict *dict)
> goto out;
> }
>
> -key = qobject_to(QString, parse_value(ctxt));
> +key_obj = parse_value(ctxt);
> +key = qobject_to(QString, key_obj);
> if (!key) {
> parse_error(ctxt, peek, "key is not a string in object");
> goto out;
> @@ -297,13 +299,11 @@ static int parse_pair(JSONParserContext *ctxt,
> QDict *dict)
>
> qdict_put_obj(dict, qstring_get_str(key), value);
>
> -qobject_unref(key);
> -
> +qobject_unref(key_obj);
> return 0;
>
> out:
> -qobject_unref(key);
> -
> +qobject_unref(key_obj);
> return -1;
> }
>
Hi Alex,
Perhaps it would be more appropriate to provide the testcase as a separate
patch which from Markus.
Thanks,
Chen Qun
> diff --git a/tests/check-qjson.c b/tests/check-qjson.c index
> 07a773e653..9a02079099 100644
> --- a/tests/check-qjson.c
> +++ b/tests/check-qjson.c
> @@ -1415,6 +1415,14 @@ static void invalid_dict_comma(void)
> g_assert(obj == NULL);
> }
>
> +static void invalid_dict_key(void)
> +{
> +Error *err = NULL;
> +QObject *obj = qobject_from_json("{32:'abc'}", );
> +error_free_or_abort();
> +g_assert(obj == NULL);
> +}
> +
> static void unterminat
RE: [PATCH v2 4/5] plugins/loader: fix uninitialized variable warning in plugin_reset_uninstall()
> -Original Message-
> From: Alex Bennée [mailto:alex.ben...@linaro.org]
> Sent: Thursday, November 12, 2020 1:23 AM
> To: Chenqun (kuhn)
> Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org;
> peter.mayd...@linaro.org; Zhanghailiang ;
> ganqixin ; Euler Robot
> Subject: Re: [PATCH v2 4/5] plugins/loader: fix uninitialized variable
> warning in
> plugin_reset_uninstall()
>
>
> Chen Qun writes:
>
> > After the WITH_QEMU_LOCK_GUARD macro is added, the compiler cannot
> > identify that the statements in the macro must be executed. As a
> > result, some variables assignment statements in the macro may be
> considered as unexecuted by the compiler.
> >
> > When the -Wmaybe-uninitialized capability is enabled on GCC9,the compiler
> showed warning:
> > plugins/loader.c: In function ‘plugin_reset_uninstall’:
> > plugins/loader.c:382:15: warning: ‘ctx’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]
> > 382 | data->ctx = ctx;
> > | ~~^
> >
> > Add a default value for 'expire_time' to prevented the warning.
> >
> > Reported-by: Euler Robot
> > Signed-off-by: Chen Qun
> > ---
> > Cc: "Alex Bennée"
> > ---
> > plugins/loader.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/plugins/loader.c b/plugins/loader.c index
> > 8ac5dbc20f..88593fe138 100644
> > --- a/plugins/loader.c
> > +++ b/plugins/loader.c
> > @@ -367,7 +367,7 @@ void plugin_reset_uninstall(qemu_plugin_id_t id,
> > bool reset) {
> > struct qemu_plugin_reset_data *data;
> > -struct qemu_plugin_ctx *ctx;
> > +struct qemu_plugin_ctx *ctx = NULL;
>
> This doesn't really fix anything because you would end up faulting if you
> attempted to de-ref a NULL ctx. However...
>
> >
> > WITH_QEMU_LOCK_GUARD() {
> > ctx = plugin_id_to_ctx_locked(id);
>
> ...this can't fail. If the lookup failed and returned a NULL plugin then we
> would
> abort(). So why can't the Euler Robot see that?
>
Hi Alex ,
As the commit message says, this warning is reported by GCC 9.3 compilation.
EulerRobot configures various compilation options and uses GCC or Clang to
compilation tests.
This warning has occurred since WITH_QEMU_LOCK_GUARD was added, because the
current compiler thinks that the statements in WITH_QEMU_LOCK_GUARD{ } may not
be executed successfully.
In fact, it may be wrong, but the current compiler is not very smart. So, this
patch only adds an initialization, if it does not have a bad effect, it can fix
the warning which may exist for a long time.
Thanks,
Chen Qun
RE: [PATCH] hw/intc: fix heap-buffer-overflow in rxicu_realize()
> -Original Message-
> From: Peter Maydell [mailto:peter.mayd...@linaro.org]
> Sent: Tuesday, November 10, 2020 11:30 PM
> To: Chenqun (kuhn)
> Cc: QEMU Developers ; QEMU Trivial
> ; Yoshinori Sato ;
> Zhanghailiang ; ganqixin
> ; Euler Robot
> Subject: Re: [PATCH] hw/intc: fix heap-buffer-overflow in rxicu_realize()
>
> On Thu, 5 Nov 2020 at 07:08, Chen Qun wrote:
> >
> > When 'j = icu->nr_sense – 1', the 'j < icu->nr_sense' condition is
> > true, then 'j = icu->nr_sense', the'icu->init_sense[j]' has out-of-bounds
> > access.
>
> Yes, this is a bug...
>
> > Maybe this could lead to some security problems.
>
> ...but it's not a security bug, because this device can't be used with KVM,
> so it's
> not on the QEMU security boundary.
>
>
> > hw/intc/rx_icu.c | 6 ++
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/hw/intc/rx_icu.c b/hw/intc/rx_icu.c index
> > 94e17a9dea..692a4c78e0 100644
> > --- a/hw/intc/rx_icu.c
> > +++ b/hw/intc/rx_icu.c
> > @@ -308,11 +308,9 @@ static void rxicu_realize(DeviceState *dev, Error
> **errp)
> > return;
> > }
> > for (i = j = 0; i < NR_IRQS; i++) {
> > -if (icu->init_sense[j] == i) {
> > +if (j < icu->nr_sense && icu->init_sense[j] == i) {
> > icu->src[i].sense = TRG_LEVEL;
> > -if (j < icu->nr_sense) {
> > -j++;
> > -}
> > +j++;
> > } else {
> > icu->src[i].sense = TRG_PEDGE;
> > }
>
> This works, so:
>
> Reviewed-by: Peter Maydell
>
> but to be honest I think this would be more readable:
>
> for (i = 0; i < NR_IRQS; i++) {
> ice->src[i].sense = TRG_PEDGE;
> }
> for (i = 0; i < icu->nr_sense; i++) {
> uint8_t irqno = icu->init_sense[i];
> if (irqno < NR_IRQS) {
> icu->src[irqno].sense = TRG_LEVEL;
> }
> }
>
It is a good point!
I tried to modify and compile it, and the test results are exactly the same.
Only GCC9 reports a warning：
../hw/intc/rx_icu.c: In function ‘rxicu_realize’:
../hw/intc/rx_icu.c:317:19: warning: comparison is always true due to limited
range of data type [-Wtype-limits]
317 | if (irqno < NR_IRQS) {
| ^
The 'NR_IRQS = 256' ,the ' if (irqno < NR_IRQS)' condition is always true.
So,maybe we should remove this condition. I'll modify it later.
Thanks,
Chen Qun
RE: [PATCH] hw/intc: fix heap-buffer-overflow in rxicu_realize()
Ping,
Fix: e78597cc457ff7611
Maybe this bug needs to qemu-5.2 version.
The "icu->nr_sense" is array length. It's a typical out-of-bounds array bug.
Thanks,
Chen Qun
> -Original Message-
> From: Chenqun (kuhn)
> Sent: Thursday, November 5, 2020 3:06 PM
> To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org
> Cc: Zhanghailiang ; ganqixin
> ; Chenqun (kuhn) ;
> Euler Robot ; Yoshinori Sato
>
> Subject: [PATCH] hw/intc: fix heap-buffer-overflow in rxicu_realize()
>
> When 'j = icu->nr_sense – 1', the 'j < icu->nr_sense' condition is true, then
> 'j =
> icu->nr_sense', the'icu->init_sense[j]' has out-of-bounds access.
> Maybe this could lead to some security problems.
>
> The asan showed stack:
> ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60404d7d at
> pc 0x55852cd26a76 bp 0x7ffe39f26200 sp 0x7ffe39f261f0 READ of size 1 at
> 0x60404d7d thread T0
> #0 0x55852cd26a75 in rxicu_realize ../hw/intc/rx_icu.c:311
> #1 0x55852cf075f7 in device_set_realized ../hw/core/qdev.c:886
> #2 0x55852cd4a32f in property_set_bool ../qom/object.c:2251
> #3 0x55852cd4f9bb in object_property_set ../qom/object.c:1398
> #4 0x55852cd54f3f in
> object_property_set_qobject ../qom/qom-qobject.c:28
> #5 0x55852cd4fc3f in object_property_set_bool ../qom/object.c:1465
> #6 0x55852cbf0b27 in register_icu ../hw/rx/rx62n.c:156
> #7 0x55852cbf12a6 in rx62n_realize ../hw/rx/rx62n.c:261
> #8 0x55852cf075f7 in device_set_realized ../hw/core/qdev.c:886
> #9 0x55852cd4a32f in property_set_bool ../qom/object.c:2251
> #10 0x55852cd4f9bb in object_property_set ../qom/object.c:1398
> #11 0x55852cd54f3f in
> object_property_set_qobject ../qom/qom-qobject.c:28
> #12 0x55852cd4fc3f in object_property_set_bool ../qom/object.c:1465
> #13 0x55852cbf1a85 in rx_gdbsim_init ../hw/rx/rx-gdbsim.c:109
> #14 0x55852cd22de0 in qemu_init ../softmmu/vl.c:4380
> #15 0x55852ca57088 in main ../softmmu/main.c:49
> #16 0x7feefafa5d42 in __libc_start_main (/lib64/libc.so.6+0x26d42)
>
> Change the 'j < icu->nr_sense' condition place to fix it.
>
> Reported-by: Euler Robot
> Signed-off-by: Chen Qun
> ---
> Cc: Yoshinori Sato
> ---
> hw/intc/rx_icu.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/hw/intc/rx_icu.c b/hw/intc/rx_icu.c index 94e17a9dea..692a4c78e0
> 100644
> --- a/hw/intc/rx_icu.c
> +++ b/hw/intc/rx_icu.c
> @@ -308,11 +308,9 @@ static void rxicu_realize(DeviceState *dev, Error
> **errp)
> return;
> }
> for (i = j = 0; i < NR_IRQS; i++) {
> -if (icu->init_sense[j] == i) {
> +if (j < icu->nr_sense && icu->init_sense[j] == i) {
> icu->src[i].sense = TRG_LEVEL;
> -if (j < icu->nr_sense) {
> -j++;
> -}
> +j++;
> } else {
> icu->src[i].sense = TRG_PEDGE;
> }
> --
> 2.27.0
RE: [PATCH RESEND v2 0/7] some memleak trivial patchs
Ping! Hi Laurent, These patches have been reviewed by many people 2 months ago. Could you help add them to the trivial branch? Pan Nengyuan (6): qga/channel-posix: Plug memory leak in ga_channel_write_all() elf2dmp/qemu_elf: Plug memleak in QEMU_Elf_init elf2dmp/pdb: Plug memleak in pdb_init_from_file migration/colo: Plug memleaks in colo_process_incoming_thread blockdev: Fix a memleak in drive_backup_prepare() block/file-posix: fix a possible undefined behavior Thanks， Chen Qun > -Original Message- > From: Chenqun (kuhn) > Sent: Friday, October 30, 2020 6:23 PM > To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org > Cc: Pannengyuan ; lviv...@redhat.com; > Zhanghailiang ; ganqixin > > Subject: RE: [PATCH RESEND v2 0/7] some memleak trivial patchs > > Ping! > > Hi all, > The patche2 ~7 have been reviewed for some time. > Could someone please pick it up？ > > Thanks, > Chen Qun > > > -Original Message- > > From: Chenqun (kuhn) > > Sent: Friday, October 23, 2020 2:12 PM > > To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org > > Cc: Pannengyuan ; lviv...@redhat.com; > > Zhanghailiang ; ganqixin > > ; Chenqun (kuhn) > > Subject: [PATCH RESEND v2 0/7] some memleak trivial patchs > > > > Hi all, > > > > Here are some memory leak patches reported by EulerRobot. > > Some patch submissions have been unattended for a while and I resend them. > > > > Thanks, > > Chen Qun > > > > > > Chen Qun (1): > > tests/migration: fix memleak in wait_command/wait_command_fd > > > > Pan Nengyuan (6): > > qga/channel-posix: Plug memory leak in ga_channel_write_all() > > elf2dmp/qemu_elf: Plug memleak in QEMU_Elf_init > > elf2dmp/pdb: Plug memleak in pdb_init_from_file > > migration/colo: Plug memleaks in colo_process_incoming_thread > > blockdev: Fix a memleak in drive_backup_prepare() > > block/file-posix: fix a possible undefined behavior > > > > block/file-posix.c | 2 +- > > blockdev.c | 1 + > > contrib/elf2dmp/pdb.c | 1 + > > contrib/elf2dmp/qemu_elf.c | 1 + > > migration/colo.c| 5 - > > qga/channel-posix.c | 6 +- > > tests/qtest/migration-helpers.c | 16 > > 7 files changed, 25 insertions(+), 7 deletions(-) > > > > -- > > 2.23.0
RE: [PATCH 1/6] target/xtensa: fix uninitialized variable warning
> -Original Message-
> From: Max Filippov [mailto:jcmvb...@gmail.com]
> Sent: Tuesday, November 3, 2020 5:22 PM
> To: Chenqun (kuhn)
> Cc: qemu-devel ; QEMU Trivial
> ; Zhanghailiang
> ; ganqixin ; Euler
> Robot
> Subject: Re: [PATCH 1/6] target/xtensa: fix uninitialized variable warning
>
> On Mon, Nov 2, 2020 at 5:52 PM Chen Qun
> wrote:
> >
> > The compiler cannot determine whether the return values of the
> > xtensa_operand_is_register(isa, opc, opnd) and
> xtensa_operand_is_visible(isa, opc, opnd) functions are the same.
>
> It doesn't have to because 1) they definitely are not the same, but
> 2) it doesn't matter.
>
> > So,it assumes that 'rf' is not assigned, but it's used.
>
> The assumption is wrong. rf is used under the 'if (register_file)'
> condition and register_file is initialized to NULL and then set to something
> non-NULL based on the value of rf here:
>
Hi Max,
Yeah, your analysis is correct. This rf is used only when register_file is
non-NULL. When this condition is met, the rf must have been assigned a value.
The GCC 9.3 compilation I use contains the Wmaybe-uninitialized parameter by
default. It cannot recognize this complex logic judgment.
This warning may be frequently encountered by developers who compile this part
of code.
> 958 if (xtensa_operand_is_register(isa, opc, opnd)) {
> 959 rf = xtensa_operand_regfile(isa, opc, opnd);
> 960 register_file = dc->config->regfile[rf];
>
> > The compiler showed warning:
> > target/xtensa/translate.c: In function ‘disas_xtensa_insn’:
> > target/xtensa/translate.c:985:43: warning: ‘rf’ may be used uninitialized in
> this function [-Wmaybe-uninitialized]
> > 985 | arg[vopnd].num_bits =
> xtensa_regfile_num_bits(isa, rf);
> > |
> ^~~~
> >
> > Add a default value for 'rf' to prevented the warning.
>
> I don't see it doing default build with gcc 8.3. But then I don't see
> -Wmaybe-uninitialized in the compiler command line either.
>
Maybe it's available after GCC9, or some CFLAG configuration.
The -Wmaybe-uninitialized parameter has this description：
"These warnings are only possible in optimizing compilation, because otherwise
GCC does not keep track of the state of variables."
From：https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#Warning-Options
I have tried to configure only "-O2 -fexceptions" for the CFLAG on GCC9, and
this warning will occur.
> > Reported-by: Euler Robot
> > Signed-off-by: Chen Qun
> > ---
> > Cc: Max Filippov
> > ---
> > target/xtensa/translate.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/xtensa/translate.c b/target/xtensa/translate.c
> > index 944a157747..eea851bbe7 100644
> > --- a/target/xtensa/translate.c
> > +++ b/target/xtensa/translate.c
> > @@ -953,7 +953,7 @@ static void disas_xtensa_insn(CPUXtensaState *env,
> > DisasContext *dc)
> >
> > for (opnd = vopnd = 0; opnd < opnds; ++opnd) {
> > void **register_file = NULL;
> > -xtensa_regfile rf;
> > +xtensa_regfile rf = -1;
>
> Please use XTENSA_UNDEFINED instead if you still think this is worth changing.
>
I don't think it's wrong, it's just a bit of trouble for the compiler :)
Thanks,
Chen Qun
RE: [PATCH 0/6] fix uninitialized variable warning
> -Original Message- > From: Peter Maydell [mailto:peter.mayd...@linaro.org] > Sent: Tuesday, November 3, 2020 6:15 PM > To: Chenqun (kuhn) > Cc: QEMU Developers ; QEMU Trivial > ; Zhanghailiang > ; ganqixin > Subject: Re: [PATCH 0/6] fix uninitialized variable warning > > On Tue, 3 Nov 2020 at 02:07, Chen Qun wrote: > > > > Hi all, > > There are some variables initialized warnings reported by the > > compiler, even if the default CFLAG for the compiler parameters are uesed. > > > > This serial has added some default values or changed the assignment places > for the variablies to fix them. > > > Hi; if you're reporting/fixing compiler errors/warnings please can you include > the compiler version that produced the error in the commit message? This is > helpful for telling us whether it's a problem with older compilers or whether > this > is a new compiler that checks for some things more carefully. > Oops, I didn't consider that. These warnings were reported by GCC 9.3, Thanks, Chen Qun
RE: [PATCH 5/6] plugins/loader: fix uninitialized variable warning in plugin_reset_uninstall()
> -Original Message-
> From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com]
> Sent: Tuesday, November 3, 2020 10:18 AM
> To: Chenqun (kuhn) ; qemu-devel@nongnu.org;
> qemu-triv...@nongnu.org
> Cc: Alex Bennée ; Zhanghailiang
> ; ganqixin ; Euler
> Robot
> Subject: Re: [PATCH 5/6] plugins/loader: fix uninitialized variable warning in
> plugin_reset_uninstall()
>
> On 11/3/20 2:52 AM, Chen Qun wrote:
> > After the WITH_QEMU_LOCK_GUARD macro is added, the compiler cannot
> > identify that the statements in the macro must be executed. As a
> > result, some variables assignment statements in the macro may be
> considered as unexecuted by the compiler.
> >
> > The compiler showed warning:
> > plugins/loader.c: In function ‘plugin_reset_uninstall’:
> > plugins/loader.c:382:15: warning: ‘ctx’ may be used uninitialized in
> > this function [-Wmaybe-uninitialized]
>
> This shouldn't happen as the function returns before (else there is a NULL
> deref).
>
Yes, in fact, it shouldn't have happened when the program was running.
But after added 'WITH_QEMU_LOCK_GUARD', let the compiler think it might happen.
So, we add a default value, make the compiler happy.
Thanks,
Chen Qun
> > 382 | data->ctx = ctx;
> > | ~~^
> >
> > Add a default value for 'expire_time' to prevented the warning.
> >
> > Reported-by: Euler Robot
> > Signed-off-by: Chen Qun
> > ---
> > Cc: "Alex Bennée"
> > ---
> > plugins/loader.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/plugins/loader.c b/plugins/loader.c index
> > 8ac5dbc20f..88593fe138 100644
> > --- a/plugins/loader.c
> > +++ b/plugins/loader.c
> > @@ -367,7 +367,7 @@ void plugin_reset_uninstall(qemu_plugin_id_t id,
> > bool reset) {
> > struct qemu_plugin_reset_data *data;
> > -struct qemu_plugin_ctx *ctx;
> > +struct qemu_plugin_ctx *ctx = NULL;
> >
> > WITH_QEMU_LOCK_GUARD() {
> > ctx = plugin_id_to_ctx_locked(id);
> >
RE: [PATCH] vhost: fix up trailing whitespace
> -Original Message-
> From: Qemu-devel
> [mailto:qemu-devel-bounces+kuhn.chenqun=huawei@nongnu.org] On
> Behalf Of Michael S. Tsirkin
> Sent: Monday, November 2, 2020 6:13 PM
> To: qemu-devel@nongnu.org
> Subject: [PATCH] vhost: fix up trailing whitespace
>
> Drop trailing whitespace in vhost.c
>
> Signed-off-by: Michael S. Tsirkin
> ---
> hw/virtio/vhost.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index
> f2482378c6..2e904bbd62 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1648,12 +1648,12 @@ int vhost_dev_load_inflight(struct vhost_inflight
> *inflight, QEMUFile *f) int vhost_dev_prepare_inflight(struct vhost_dev
> *hdev)
> {
> int r;
> -
> +
> if (hdev->vhost_ops->vhost_get_inflight_fd == NULL ||
> hdev->vhost_ops->vhost_set_inflight_fd == NULL) {
> return 0;
> }
> -
> +
> r = vhost_dev_set_features(hdev, hdev->log_enabled);
> if (r < 0) {
> VHOST_OPS_DEBUG("vhost_dev_prepare_inflight failed");
> --
> MST
>
Reviewed-by: Chen Qun
RE: [PATCH v2] pc: comment style fixup
> -Original Message-
> From: Qemu-devel
> [mailto:qemu-devel-bounces+kuhn.chenqun=huawei@nongnu.org] On
> Behalf Of Michael S. Tsirkin
> Sent: Monday, November 2, 2020 6:29 PM
> To: qemu-devel@nongnu.org
> Cc: Paolo Bonzini ; Eduardo Habkost
> ; Richard Henderson
> Subject: [PATCH v2] pc: comment style fixup
>
> Fix up checkpatch comment style warnings.
>
> Signed-off-by: Michael S. Tsirkin
> ---
>
> changes from v1:
> address philippe's comments
> :w
>
> hw/i386/pc.c | 9 +
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 416fb0e0f6..aae45ba779 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -1149,10 +1149,11 @@ void pc_basic_device_init(struct PCMachineState
> *pcms,
> error_report("couldn't create HPET device");
> exit(1);
> }
> -/* For pc-piix-*, hpet's intcap is always IRQ2. For pc-q35-1.7
> -* and earlier, use IRQ2 for compat. Otherwise, use IRQ16~23,
> -* IRQ8 and IRQ2.
> -*/
> +/*
> + * For pc-piix-*, hpet's intcap is always IRQ2. For pc-q35-1.7 and
> + * earlier, use IRQ2 for compat. Otherwise, use IRQ16~23, IRQ8 and
> + * IRQ2.
> + */
> uint8_t compat = object_property_get_uint(OBJECT(hpet),
> HPET_INTCAP, NULL);
> if (!compat) {
> --
> MST
>
Reviewed-by: Chen Qun
RE: [PATCH RESEND v2 0/7] some memleak trivial patchs
Ping! Hi all, The patche2 ~7 have been reviewed for some time. Could someone please pick it up？ Thanks, Chen Qun > -Original Message- > From: Chenqun (kuhn) > Sent: Friday, October 23, 2020 2:12 PM > To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org > Cc: Pannengyuan ; lviv...@redhat.com; > Zhanghailiang ; ganqixin > ; Chenqun (kuhn) > Subject: [PATCH RESEND v2 0/7] some memleak trivial patchs > > Hi all, > > Here are some memory leak patches reported by EulerRobot. > Some patch submissions have been unattended for a while and I resend them. > > Thanks, > Chen Qun > > > Chen Qun (1): > tests/migration: fix memleak in wait_command/wait_command_fd > > Pan Nengyuan (6): > qga/channel-posix: Plug memory leak in ga_channel_write_all() > elf2dmp/qemu_elf: Plug memleak in QEMU_Elf_init > elf2dmp/pdb: Plug memleak in pdb_init_from_file > migration/colo: Plug memleaks in colo_process_incoming_thread > blockdev: Fix a memleak in drive_backup_prepare() > block/file-posix: fix a possible undefined behavior > > block/file-posix.c | 2 +- > blockdev.c | 1 + > contrib/elf2dmp/pdb.c | 1 + > contrib/elf2dmp/qemu_elf.c | 1 + > migration/colo.c| 5 - > qga/channel-posix.c | 6 +- > tests/qtest/migration-helpers.c | 16 > 7 files changed, 25 insertions(+), 7 deletions(-) > > -- > 2.23.0
RE: [PATCH] tcg/optimize: Add fallthrough annotations
> -Original Message- > From: Richard Henderson [mailto:richard.hender...@linaro.org] > Sent: Friday, October 30, 2020 4:07 AM > To: Thomas Huth ; Richard Henderson ; > qemu-devel@nongnu.org > Cc: Chenqun (kuhn) > Subject: Re: [PATCH] tcg/optimize: Add fallthrough annotations > > On 10/29/20 5:28 AM, Thomas Huth wrote: > > To be able to compile this file with -Werror=implicit-fallthrough, we > > need to add some fallthrough annotations to the case statements that > > might fall through. Unfortunately, the typical "/* fallthrough */" > > comments do not work here as expected since some case labels are > > wrapped in macros and the compiler fails to match the comments in this > > case. But using __attribute__((fallthrough)) seems to work fine, so > > let's use that instead. > > Why would the macro matter? It expands to two case statements with > nothing in between them. > > This sounds like a compiler bug that should be reported. > Hi all, I have queried the GCC options description about the Wimplicit-fallthrough and verified it. The value of Wimplicit-fallthrough ranges from 0 to 5. The value 0 is to ignore all warnings, which is certainly not what we need. If the value is set to 1 or 2, most fall through on the QEMU can take effect. Eg：/* FALLTHRU */、/* fallthru */、/* fall-through */、/* FALLTHOUGH */、/* fall through */、/* fallthrough */.. When the value ranges from 3 to 5, more fallthrough comments become invalid as the value increases. So, I agree with Philippe's suggestion to add a QEMU_FALLTHROUGH to unify this compiler property. Thanks, Chen Qun Additional gcc information is as follows: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html -Wimplicit-fallthrough is the same as -Wimplicit-fallthrough=3 and -Wno-implicit-fallthrough is the same as -Wimplicit-fallthrough=0. The option argument n specifies what kind of comments are accepted: -Wimplicit-fallthrough=0 disables the warning altogether. -Wimplicit-fallthrough=1 matches .* regular expression, any comment is used as fallthrough comment. -Wimplicit-fallthrough=2 case insensitively matches .*falls?[ \t-]*thr(ough|u).* regular expression. -Wimplicit-fallthrough=3 case sensitively matches one of the following regular expressions: -fallthrough @fallthrough@ lint -fallthrough[ \t]* [ \t.!]*(ELSE,? |INTENTIONAL(LY)? )? FALL(S | |-)?THR(OUGH|U)[ \t.!]*(-[^\n\r]*)? [ \t.!]*(Else,? |Intentional(ly)? )? Fall((s | |-)[Tt]|t)hr(ough|u)[ \t.!]*(-[^\n\r]*)? [ \t.!]*([Ee]lse,? |[Ii]ntentional(ly)? )? fall(s | |-)?thr(ough|u)[ \t.!]*(-[^\n\r]*)? -Wimplicit-fallthrough=4 case sensitively matches one of the following regular expressions: -fallthrough @fallthrough@ lint -fallthrough[ \t]* [ \t]*FALLTHR(OUGH|U)[ \t]* -Wimplicit-fallthrough=5 doesn’t recognize any comments as fallthrough comments, only attributes disable the warning.
RE: [PATCH 9/9] hw/timer/renesas_tmr: silence the compiler warnings
> -Original Message-
> From: Peter Maydell [mailto:peter.mayd...@linaro.org]
> Sent: Thursday, October 29, 2020 4:15 AM
> To: Thomas Huth
> Cc: Philippe Mathieu-Daudé ; Chenqun (kuhn)
> ; QEMU Developers ;
> QEMU Trivial ; Yoshinori Sato
> ; Magnus Damm ;
> Zhanghailiang ; ganqixin
> ; Euler Robot
> Subject: Re: [PATCH 9/9] hw/timer/renesas_tmr: silence the compiler warnings
>
> On Wed, 28 Oct 2020 at 15:07, Thomas Huth wrote:
> >
> > On 28/10/2020 10.59, Philippe Mathieu-Daudé wrote:
> > > On 10/28/20 5:18 AM, Chen Qun wrote:
> > >> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed
> warning:
> > >> ../hw/timer/renesas_tmr.c: In function ‘tmr_read’:
> > >> ../hw/timer/renesas_tmr.c:221:19: warning: this statement may fall
> through [-Wimplicit-fallthrough=]
> > >> 221 | } else if (ch == 0) {i
> > >> | ^
> > >> ../hw/timer/renesas_tmr.c:224:5: note: here
> > >> 224 | case A_TCORB:
> > >> | ^~~~
> > >>
> > >> Add the corresponding "fall through" comment to fix it.
> > >>
> > >> Reported-by: Euler Robot
> > >> Signed-off-by: Chen Qun
> > >> ---
> > >> Cc: Yoshinori Sato
> > >> Cc: Magnus Damm
> > >> ---
> > >> hw/timer/renesas_tmr.c | 1 +
> > >> 1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c index
> > >> 446f2eacdd..e03a8155b2 100644
> > >> --- a/hw/timer/renesas_tmr.c
> > >> +++ b/hw/timer/renesas_tmr.c
> > >> @@ -221,6 +221,7 @@ static uint64_t tmr_read(void *opaque, hwaddr
> addr, unsigned size)
> > >> } else if (ch == 0) {
> > >> return concat_reg(tmr->tcora);
> > >> }
> > >> +/* fall through */
> > >> case A_TCORB:
> > >> if (size == 1) {
> > >> return tmr->tcorb[ch];
> > >>
> > >
> > > You fixed A_TCORA but not A_TCORB...
> >
> > A_TCORB cannot fall through, since it always does a "return" in both
> > branches of the if-statement.
> >
> > However, it also looks really odd that A_TCORA falls through to
> > A_TCORB here and return that register value instead. Is this really
> > intended? Yoshinori, could you please double-check whether this is a bug
> > here,
> or intended?
>
> See the discussion in this thread:
> https://lore.kernel.org/qemu-devel/CAFEAcA8c2dywr=Zxz1ExAV-48JwFU5vbBD
> DfA=_ke98xamn...@mail.gmail.com/
> from when Coverity spotting the fall-through.
>
> I think this cleanup patch from Yoshinori deals with the fall-through stuff by
> cleaning up that area of the timer device:
> https://lore.kernel.org/qemu-devel/20200711154242.41222-1-ys...@users.so
> urceforge.jp/
> It just needs somebody to code-review it :-)
This cleanup patch has been modified more thoroughly, so let's wait for it
merge :-)
Thanks,
Chen Qun
RE: [PATCH 9/9] hw/timer/renesas_tmr: silence the compiler warnings
> -Original Message-
> From: Philippe Mathieu-Daudé [mailto:philippe.mathieu.da...@gmail.com]
> On Behalf Of Philippe Mathieu-Daudé
> Sent: Wednesday, October 28, 2020 6:00 PM
> To: Chenqun (kuhn) ; qemu-devel@nongnu.org;
> qemu-triv...@nongnu.org
> Cc: Zhanghailiang ; Yoshinori Sato
> ; Magnus Damm ;
> ganqixin ; Euler Robot
> Subject: Re: [PATCH 9/9] hw/timer/renesas_tmr: silence the compiler warnings
>
> On 10/28/20 5:18 AM, Chen Qun wrote:
> > When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed
> warning:
> > ../hw/timer/renesas_tmr.c: In function ‘tmr_read’:
> > ../hw/timer/renesas_tmr.c:221:19: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
> > 221 | } else if (ch == 0) {i
> > | ^
> > ../hw/timer/renesas_tmr.c:224:5: note: here
> > 224 | case A_TCORB:
> > | ^~~~
> >
> > Add the corresponding "fall through" comment to fix it.
> >
> > Reported-by: Euler Robot
> > Signed-off-by: Chen Qun
> > ---
> > Cc: Yoshinori Sato
> > Cc: Magnus Damm
> > ---
> > hw/timer/renesas_tmr.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/timer/renesas_tmr.c b/hw/timer/renesas_tmr.c index
> > 446f2eacdd..e03a8155b2 100644
> > --- a/hw/timer/renesas_tmr.c
> > +++ b/hw/timer/renesas_tmr.c
> > @@ -221,6 +221,7 @@ static uint64_t tmr_read(void *opaque, hwaddr addr,
> unsigned size)
> > } else if (ch == 0) {
> > return concat_reg(tmr->tcora);
> > }
> > +/* fall through */
> > case A_TCORB:
> > if (size == 1) {
> > return tmr->tcorb[ch];
> >
>
> You fixed A_TCORA but not A_TCORB...
>
Hi Philippe,
My first feeling is the same as you when this warning found.
But, the number of branches for A_TCORA and A_TCORB is different.
In A_TCORA case："} else if (ch == 0) {"
In A_TCORB case："} else {"
Obviously, A_TCOB have all return values. But A_TCOA is not, it need to fall
through or break.
> How the Euler Robot works? Like Coverity?
No, unlike Coverity, Coverity is essentially a good discovery bug tool.
But EulerRobot is a virtualization software quality automation project that
integrates some tools and test suites such as gcc/clang make test, qemu ut,
qtest, coccinelle scripts and avocado-vt.
Thanks,
Chen Qun
RE: [PATCH 8/9] target/ppc: silence the compiler warnings
> -Original Message-
> From: David Gibson [mailto:da...@gibson.dropbear.id.au]
> Sent: Thursday, October 29, 2020 7:39 AM
> To: Thomas Huth
> Cc: Philippe Mathieu-Daudé ; Chenqun (kuhn)
> ; qemu-devel@nongnu.org;
> qemu-triv...@nongnu.org; Euler Robot ;
> Zhanghailiang ; ganqixin
>
> Subject: Re: [PATCH 8/9] target/ppc: silence the compiler warnings
>
> On Wed, Oct 28, 2020 at 04:06:03PM +0100, Thomas Huth wrote:
> > On 28/10/2020 10.56, Philippe Mathieu-Daudé wrote:
> > > On 10/28/20 5:18 AM, Chen Qun wrote:
> > >> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed
> warning:
> > >> target/ppc/mmu_helper.c: In function ‘dump_mmu’:
> > >> target/ppc/mmu_helper.c:1351:12: warning: this statement may fall
> through [-Wimplicit-fallthrough=]
> > >> 1351 | if (ppc64_v3_radix(env_archcpu(env))) {
> > >> |^
> > >> target/ppc/mmu_helper.c:1358:5: note: here
> > >> 1358 | default:
> > >> | ^~~
> > >>
> > >> Add the corresponding "fall through" comment to fix it.
> > >>
> > >> Reported-by: Euler Robot
> > >> Signed-off-by: Chen Qun
> > >> ---
> > >> Cc: David Gibson
> > >> ---
> > >> target/ppc/mmu_helper.c | 1 +
> > >> 1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/target/ppc/mmu_helper.c b/target/ppc/mmu_helper.c
> > >> index 8972714775..51749b62df 100644
> > >> --- a/target/ppc/mmu_helper.c
> > >> +++ b/target/ppc/mmu_helper.c
> > >> @@ -1355,6 +1355,7 @@ void dump_mmu(CPUPPCState *env)
> > >> break;
> > >> }
> > >> #endif
> > >> +/* fall through */
> > >
> > > I'm surprise the compiler emit a warning for missing comment, but
> > > don't emit one for superfluous and confusing ones (when building a
> > > ppc32-only target). You'd need to put this before the #endif.
> > >
> > > But instead of this band-aid to silent warning, replace the TODO by
> > > a LOG_UNIMP call, and add a break before the #endif.
> >
> > +1 for replacing the TODO with a LOG_UNIMP call and adding a break
> > +instead,
> > that would look way less messy than the current code.
>
> True, that would be a better approach.
Yeah, that's a good idea.
I'll modify it in the v2.
Thanks,
Chen Qun
>
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au| minimalist, thank you. NOT _the_
> _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
RE: [PATCH 7/9] ppc: silence the compiler warnings
> -Original Message-
> From: David Gibson [mailto:da...@gibson.dropbear.id.au]
> Sent: Thursday, October 29, 2020 7:39 AM
> To: Thomas Huth
> Cc: Chenqun (kuhn) ; qemu-triv...@nongnu.org;
> Euler Robot ; qemu-devel@nongnu.org; ganqixin
> ; Zhanghailiang ;
> qemu-...@nongnu.org
> Subject: Re: [PATCH 7/9] ppc: silence the compiler warnings
>
> On Wed, Oct 28, 2020 at 03:42:31PM +0100, Thomas Huth wrote:
> > On 28/10/2020 05.29, David Gibson wrote:
> > > On Wed, Oct 28, 2020 at 12:18:17PM +0800, Chen Qun wrote:
> > >> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed
> warning:
> > >> hw/ppc/ppc.c: In function ‘ppc6xx_set_irq’:
> > >> hw/ppc/ppc.c:118:16: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
> > >> 118 | if (level) {
> > >> |^
> > >> hw/ppc/ppc.c:123:9: note: here
> > >> 123 | case PPC6xx_INPUT_INT:
> > >> | ^~~~
> > >>
> > >> Add the corresponding "fall through" comment to fix it.
> > >>
> > >> Reported-by: Euler Robot
> > >> Signed-off-by: Chen Qun
> > >
> > > Acked-by: David Gibson
> > >
> > >> ---
> > >> Cc: David Gibson
> > >> ---
> > >> hw/ppc/ppc.c | 1 +
> > >> 1 file changed, 1 insertion(+)
> > >>
> > >> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c index
> > >> 4a11fb1640..f9eb8f21b4 100644
> > >> --- a/hw/ppc/ppc.c
> > >> +++ b/hw/ppc/ppc.c
> > >> @@ -120,6 +120,7 @@ static void ppc6xx_set_irq(void *opaque, int pin,
> int level)
> > >> } else {
> > >> cpu_ppc_tb_stop(env);
> > >> }
> > >> +/* fall through */
> > >> case PPC6xx_INPUT_INT:
> > >> /* Level sensitive - active high */
> > >> LOG_IRQ("%s: set the external IRQ state to %d\n",
> > >
> >
> > Is that fall through actually really the right thing to do here? I'd
> > rather expect to see a PPC_INTERRUPT_DECR instead of a
> > PPC_INTERRUPT_EXT in case someone messes with the TBEN pin? So I
> > assume this is likely rather bug and we should a "break" statement here
> instead?
>
> Oh.. good catch, I think I misread this. I thought the change was correct,
> because DECRs look somewhat like external interrupts. But this is TBEN, not
> a DECR interrupt per se. So, yes, I think this was a bug and it should be a
> break instead.
>
This bug looks like it's been hidden for years. Thanks for your point.
According to your opinion, I will modify it in the next version.
Thanks,
Chen Qun
> --
> David Gibson | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au| minimalist, thank you. NOT _the_
> _other_
> | _way_ _around_!
> http://www.ozlabs.org/~dgibson
RE: [PATCH 3/9] accel/tcg/user-exec: silence the compiler warnings
> -Original Message- > From: Richard Henderson [mailto:richard.hender...@linaro.org] > Sent: Wednesday, October 28, 2020 11:38 PM > To: Thomas Huth ; Chenqun (kuhn) > ; qemu-devel@nongnu.org; > qemu-triv...@nongnu.org > Cc: Zhanghailiang ; Riku Voipio > ; Paolo Bonzini ; ganqixin > ; Euler Robot > Subject: Re: [PATCH 3/9] accel/tcg/user-exec: silence the compiler warnings > > On 10/28/20 6:52 AM, Thomas Huth wrote: > > On 28/10/2020 05.18, Chen Qun wrote: > >> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed > warning: > >> ../accel/tcg/user-exec.c: In function ‘handle_cpu_signal’: > >> ../accel/tcg/user-exec.c:169:13: warning: this statement may fall through > [-Wimplicit-fallthrough=] > >> 169 | cpu_exit_tb_from_sighandler(cpu, old_set); > >> | ^ > >> ../accel/tcg/user-exec.c:172:9: note: here > >> 172 | default: > >> > >> This exception branch fall through the 'default' branch and run the > 'g_assert_not_reached' statement. > >> So we could use "fall through" instead of "NORETURN" here. > >> > >> Reported-by: Euler Robot > >> Signed-off-by: Chen Qun > >> --- > >> Cc: Riku Voipio > >> Cc: Richard Henderson > >> Cc: Paolo Bonzini > >> --- > >> accel/tcg/user-exec.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/accel/tcg/user-exec.c b/accel/tcg/user-exec.c index > >> 4ebe25461a..330468e990 100644 > >> --- a/accel/tcg/user-exec.c > >> +++ b/accel/tcg/user-exec.c > >> @@ -167,7 +167,7 @@ static inline int handle_cpu_signal(uintptr_t pc, > siginfo_t *info, > >> */ > >> clear_helper_retaddr(); > >> cpu_exit_tb_from_sighandler(cpu, old_set); > >> -/* NORETURN */ > >> +/* fall through */ > > > > There should not be a fall through here since the previous function > > should never return. Does the warning go away if you mark the > > cpu_exit_tb_from_sighandler() function with QEMU_NORETURN ? If so, I > > think that would be the better fix. > > The compiler should have figured that out itself, due to cpu_loop_exit_noexc > being marked QEMU_NORETURN. However, if adding a second > QEMU_NORETURN works, I'm fine with that. > I tried to add QEMU_NORETURN to the cpu_exit_tb_from_sighandler() function. And then the compiler warning was cleared. It seems to be a better fix. Thanks, Chen Qun > As a very last resort, we can change the comment to > > /* no return, but fall through to assert not reached */ > > which correctly documents both the function preceding and also contains the > regexp that the compiler is using for the warning. >
RE: [PATCH 6/9] target/sparc/win_helper: silence the compiler warnings
> -Original Message- > From: Philippe Mathieu-Daudé [mailto:philippe.mathieu.da...@gmail.com] > On Behalf Of Philippe Mathieu-Daudé > Sent: Wednesday, October 28, 2020 5:51 PM > To: Chenqun (kuhn) ; qemu-devel@nongnu.org; > qemu-triv...@nongnu.org > Cc: Zhanghailiang ; Mark Cave-Ayland > ; ganqixin ; Euler > Robot ; Artyom Tarasenko > > Subject: Re: [PATCH 6/9] target/sparc/win_helper: silence the compiler > warnings > > On 10/28/20 5:18 AM, Chen Qun wrote: > > When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed > warning: > > target/sparc/win_helper.c: In function ‘get_gregset’: > > target/sparc/win_helper.c:304:9: warning: this statement may fall through > [-Wimplicit-fallthrough=] > > 304 | trace_win_helper_gregset_error(pstate); > > | ^~ > > target/sparc/win_helper.c:306:5: note: here > > 306 | case 0: > > | ^~~~ > > > > Add the corresponding "fall through" comment to fix it. > > > > Reported-by: Euler Robot > > Signed-off-by: Chen Qun > > --- > > Cc: Mark Cave-Ayland > > Cc: Artyom Tarasenko > > --- > > target/sparc/win_helper.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/target/sparc/win_helper.c b/target/sparc/win_helper.c > > index 8290a21142..32eacc05e6 100644 > > --- a/target/sparc/win_helper.c > > +++ b/target/sparc/win_helper.c > > @@ -303,6 +303,7 @@ static inline uint64_t *get_gregset(CPUSPARCState > *env, uint32_t pstate) > > default: > > trace_win_helper_gregset_error(pstate); > > /* pass through to normal set of global registers */ > > +/* fall through */ > > Can you keep the comment, doing s/pass/fall/? > That looks good, too. Thanks, Chen Qun > > case 0: > > return env->bgregs; > > case PS_AG: > >
RE: [PATCH 1/9] target/i386: silence the compiler warnings in gen_shiftd_rm_T1
> -Original Message-
> From: Thomas Huth [mailto:th...@redhat.com]
> Sent: Thursday, October 29, 2020 12:52 AM
> To: Richard Henderson ; Chenqun (kuhn)
> ; qemu-devel@nongnu.org;
> qemu-triv...@nongnu.org
> Cc: Eduardo Habkost ; Zhanghailiang
> ; ganqixin ; Euler
> Robot ; Paolo Bonzini ;
> Richard Henderson
> Subject: Re: [PATCH 1/9] target/i386: silence the compiler warnings in
> gen_shiftd_rm_T1
>
> On 28/10/2020 16.31, Richard Henderson wrote:
> > On 10/28/20 5:57 AM, Thomas Huth wrote:
> >> On 28/10/2020 05.18, Chen Qun wrote:
> >>> The current "#ifdef TARGET_X86_64" statement affects the compiler's
> >>> determination of fall through.
> >>>
> >>> When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed
> warning:
> >>> target/i386/translate.c: In function ‘gen_shiftd_rm_T1’:
> >>> target/i386/translate.c:1773:12: warning: this statement may fall through
> [-Wimplicit-fallthrough=]
> >>> if (is_right) {
> >>> ^
> >>> target/i386/translate.c:1782:5: note: here
> >>> case MO_32:
> >>> ^~~~
> >>>
> >>> Reported-by: Euler Robot
> >>> Signed-off-by: Chen Qun
> >>> ---
> >>> Cc: Paolo Bonzini
> >>> Cc: Richard Henderson
> >>> Cc: Eduardo Habkost
> >>> ---
> >>> target/i386/translate.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/target/i386/translate.c b/target/i386/translate.c index
> >>> caea6f5fb1..4c353427d7 100644
> >>> --- a/target/i386/translate.c
> >>> +++ b/target/i386/translate.c
> >>> @@ -1777,9 +1777,9 @@ static void gen_shiftd_rm_T1(DisasContext *s,
> MemOp ot, int op1,
> >>> } else {
> >>> tcg_gen_deposit_tl(s->T1, s->T0, s->T1, 16, 16);
> >>> }
> >>> -/* FALLTHRU */
> >>> -#ifdef TARGET_X86_64
> >>> +/* fall through */
> >>> case MO_32:
> >>> +#ifdef TARGET_X86_64
> >>> /* Concatenate the two 32-bit values and use a 64-bit shift.
> */
> >>> tcg_gen_subi_tl(s->tmp0, count, 1);
> >>> if (is_right) {
> >>
> >> The whole code here looks a little bit fishy to me ... in case
> >> TARGET_X86_64 is defined, the MO_16 code falls through to MO_32 ...
> >> but in case it is not defined, it falls through to the default case
> >> that comes after the #ifdef block? Is this really the right thing
> >> here? If so, I think there should be some additional comments explaining
> this behavior.
> >>
> >> Richard, maybe you could help to judge what is right here...?
> >
> > It could definitely be rewritten, but it's not wrong as is.
>
> Ok, thanks for the clarification! In that case:
>
> Reviewed-by: Thomas Huth
Thanks for the discussion!
I might add a little comment to describe this behavior base on this patch.
Just like: /* If TARGET_X86_64 defined then fall through into MO_32, otherwise
fall through default. */
If there is no other suggestion, I'll keep Richard's and Thomas 's
"Reviewed-by" tag.
Thanks,
Chen Qun
RE: [PATCH] target/ppc/excp_helper: Add a missing break for POWERPC_EXCP_HISI
> -Original Message- > From: David Gibson [mailto:da...@gibson.dropbear.id.au] > Sent: Wednesday, October 28, 2020 12:28 PM > To: Chenqun (kuhn) > Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org; c...@kaod.org; > Zhanghailiang ; ganqixin > ; Euler Robot > Subject: Re: [PATCH] target/ppc/excp_helper: Add a missing break for > POWERPC_EXCP_HISI > > On Wed, Oct 28, 2020 at 12:16:20PM +0800, Chen Qun wrote: > > When using -Wimplicit-fallthrough in our CFLAGS, the compiler showed > warning: > > ../target/ppc/excp_helper.c: In function ‘powerpc_excp’: > > ../target/ppc/excp_helper.c:529:13: warning: this statement may fall through > [-Wimplicit-fallthrough=] > > 529 | msr |= env->error_code; > > | ^~ > > ../target/ppc/excp_helper.c:530:5: note: here > > 530 | case POWERPC_EXCP_HDECR: /* Hypervisor > decrementer exception */ > > | ^~~~ > > > > A break statement may be required to enter this exception. > > > > Reported-by: Euler Robot > > Signed-off-by: Chen Qun > > This change is incorrect. We definitely need the fallthrough to set srr[01] > properly. So the correct fix is to annotate the fallthrough, not remove it. > OK, I'll modify it later. ' Thanks, Chen Qun > > > > --- > > I guess there's a break missing in 'POWERPC_EXCP_HISI' branch, just > > like the 'POWERPC_EXCP_ISI' branch. > > --- > > target/ppc/excp_helper.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c index > > a988ba15f4..5e69ac1b33 100644 > > --- a/target/ppc/excp_helper.c > > +++ b/target/ppc/excp_helper.c > > @@ -527,6 +527,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, > int excp_model, int excp) > > break; > > case POWERPC_EXCP_HISI: /* Hypervisor instruction storage > exception */ > > msr |= env->error_code; > > +break; > > case POWERPC_EXCP_HDECR: /* Hypervisor decrementer > exception */ > > case POWERPC_EXCP_HDSI: /* Hypervisor data storage > exception*/ > > case POWERPC_EXCP_HDSEG: /* Hypervisor data segment > exception*/ > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au| minimalist, thank you. NOT _the_ > _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson
RE: [PATCH v3] migration/block-dirty-bitmap: fix uninitialized variable warning
Ping! Hello, Maybe this patch, some people have any other suggestions? Or, maybe missed to queue? Thanks, Chen Qun > -Original Message- > From: Vladimir Sementsov-Ogievskiy [mailto:vsement...@virtuozzo.com] > Sent: Wednesday, October 14, 2020 11:56 PM > To: Chenqun (kuhn) ; qemu-devel@nongnu.org; > qemu-triv...@nongnu.org > Cc: mre...@redhat.com; stefa...@redhat.com; f...@euphon.net; > ebl...@redhat.com; js...@redhat.com; quint...@redhat.com; > dgilb...@redhat.com; Zhanghailiang ; > ganqixin ; qemu-bl...@nongnu.org; Euler Robot > ; Laurent Vivier ; Li Qiang > > Subject: Re: [PATCH v3] migration/block-dirty-bitmap: fix uninitialized > variable > warning > > 14.10.2020 14:44, Chen Qun wrote: > > A default value is provided for the variable 'bitmap_name' to avoid compiler > warning. > > > > The compiler show warning: > > migration/block-dirty-bitmap.c:1090:13: warning: ‘bitmap_name’ > > may be used uninitialized in this function [-Wmaybe-uninitialized] > > g_strlcpy(s->bitmap_name, bitmap_name, > sizeof(s->bitmap_name)); > > > ^~ > > > > Reported-by: Euler Robot > > Signed-off-by: Chen Qun > > Reviewed-by: Vladimir Sementsov-Ogievskiy > > -- > Best regards, > Vladimir
RE: [PATCH v2] migration/block-dirty-bitmap: fix uninitialized variable warning
> -Original Message-
> From: Qemu-devel
> [mailto:qemu-devel-bounces+kuhn.chenqun=huawei@nongnu.org] On
> Behalf Of Max Reitz
> Sent: Wednesday, October 14, 2020 5:36 PM
> To: Chenqun (kuhn) ; qemu-devel@nongnu.org;
> qemu-triv...@nongnu.org
> Cc: f...@euphon.net; ganqixin ;
> vsement...@virtuozzo.com; Zhanghailiang
> ; qemu-bl...@nongnu.org;
> quint...@redhat.com; Li Qiang ; dgilb...@redhat.com;
> Laurent Vivier ; stefa...@redhat.com; Euler Robot
> ; js...@redhat.com
> Subject: Re: [PATCH v2] migration/block-dirty-bitmap: fix uninitialized
> variable
> warning
>
> On 14.10.20 03:03, Chenqun (kuhn) wrote:
> >
> >
> >> -Original Message-
> >> From: Max Reitz [mailto:mre...@redhat.com]
> >> Sent: Tuesday, October 13, 2020 10:47 PM
> >> To: Chenqun (kuhn) ;
> qemu-devel@nongnu.org;
> >> qemu-triv...@nongnu.org
> >> Cc: vsement...@virtuozzo.com; stefa...@redhat.com; f...@euphon.net;
> >> ebl...@redhat.com; js...@redhat.com; quint...@redhat.com;
> >> dgilb...@redhat.com; Zhanghailiang ;
> >> ganqixin ; qemu-bl...@nongnu.org; Euler Robot
> >> ; Laurent Vivier ; Li
> >> Qiang
> >> Subject: Re: [PATCH v2] migration/block-dirty-bitmap: fix
> >> uninitialized variable warning
> >>
> >> On 13.10.20 14:33, Chen Qun wrote:
> >>> A default value is provided for the variable 'bitmap_name' to avoid
> >>> compiler
> >> warning.
> >>>
> >>> The compiler show warning:
> >>> migration/block-dirty-bitmap.c:1090:13: warning: ‘bitmap_name’
> >>> may be used uninitialized in this function [-Wmaybe-uninitialized]
> >>>g_strlcpy(s->bitmap_name, bitmap_name,
> >> sizeof(s->bitmap_name));
> >>>
> >>
> ^~
> >>>
> >>> Reported-by: Euler Robot
> >>> Signed-off-by: Chen Qun
> >>> ---
> >>> Cc: Vladimir Sementsov-Ogievskiy
> >>> Cc: Laurent Vivier
> >>> Cc: Li Qiang
> >>> ---
> >>> migration/block-dirty-bitmap.c | 9 ++---
> >>> 1 file changed, 2 insertions(+), 7 deletions(-)
> >>
> >> No objections, semantically, but...
> >>
> >>> diff --git a/migration/block-dirty-bitmap.c
> >>> b/migration/block-dirty-bitmap.c index 5bef793ac0..bcb79c04ce 100644
> >>> --- a/migration/block-dirty-bitmap.c
> >>> +++ b/migration/block-dirty-bitmap.c
> >>> @@ -1064,15 +1064,13 @@ static int
> dirty_bitmap_load_header(QEMUFile
> >> *f, DBMLoadState *s,
> >>> assert(nothing || s->cancelled || !!alias_map ==
> >>> !!bitmap_alias_map);
> >>>
> >>> if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
> >>> -const char *bitmap_name;
> >>> -
> >>> if (!qemu_get_counted_string(f, s->bitmap_alias)) {
> >>> error_report("Unable to read bitmap alias string");
> >>> return -EINVAL;
> >>> }
> >>>
> >>> -if (!s->cancelled) {
> >>> -if (bitmap_alias_map) {
> >>> +const char *bitmap_name = s->bitmap_alias;
> >>
> >> qemu’s coding style mandates declarations to be placed at the
> >> beginning of their block, so the declaration has to stay where it is.
> >> (Putting the assignment here looks reasonable.)
> >>
> > Hi Max,
> > Declaration variables here to ensure that the above exceptions(Unable to
> read bitmap alias string) are avoided.
> > If the declaration has to stay where it is, is there a possibility that the
> assignment fails?
>
> I don’t understand what you mean.
I think my description is not accurate. Forgive me for being a non-native
English speaker.
The variable 'bitmap_name' assignment maybe failed at the beginning of the
block, because reading the 's->bitmap_alias' maybe failed.
>A declaration without initialization isn’t
> and doesn’t contain an expression, it isn’t even a statement, so it has no
> side
> effects.[1]
>
> Placing the declaration (without an initialization) at the top of the block
> makes
> no semantic difference.
>
I see what you mean. Separate variable declarations from variable assignments.
You're right! I will update it later.
Thanks,
Chen Qun
> (As I said, I’d keep the assignment “bitmap_name = s->bitmap_alias”
> where you put it. I think it would technically actually be correct to put it
> into
> the declaration at the start of the block as an initializer, but that would
> look
> weird.)
>
> Max
>
> [1] I suppose exceptions apply for types with constructors, but those don’t
> exist in plain C.
RE: [PATCH v2] migration/block-dirty-bitmap: fix uninitialized variable warning
> -Original Message-
> From: Max Reitz [mailto:mre...@redhat.com]
> Sent: Tuesday, October 13, 2020 10:47 PM
> To: Chenqun (kuhn) ; qemu-devel@nongnu.org;
> qemu-triv...@nongnu.org
> Cc: vsement...@virtuozzo.com; stefa...@redhat.com; f...@euphon.net;
> ebl...@redhat.com; js...@redhat.com; quint...@redhat.com;
> dgilb...@redhat.com; Zhanghailiang ;
> ganqixin ; qemu-bl...@nongnu.org; Euler Robot
> ; Laurent Vivier ; Li Qiang
>
> Subject: Re: [PATCH v2] migration/block-dirty-bitmap: fix uninitialized
> variable
> warning
>
> On 13.10.20 14:33, Chen Qun wrote:
> > A default value is provided for the variable 'bitmap_name' to avoid compiler
> warning.
> >
> > The compiler show warning:
> > migration/block-dirty-bitmap.c:1090:13: warning: ‘bitmap_name’
> > may be used uninitialized in this function [-Wmaybe-uninitialized]
> >g_strlcpy(s->bitmap_name, bitmap_name,
> sizeof(s->bitmap_name));
> >
> ^~
> >
> > Reported-by: Euler Robot
> > Signed-off-by: Chen Qun
> > ---
> > Cc: Vladimir Sementsov-Ogievskiy
> > Cc: Laurent Vivier
> > Cc: Li Qiang
> > ---
> > migration/block-dirty-bitmap.c | 9 ++---
> > 1 file changed, 2 insertions(+), 7 deletions(-)
>
> No objections, semantically, but...
>
> > diff --git a/migration/block-dirty-bitmap.c
> > b/migration/block-dirty-bitmap.c index 5bef793ac0..bcb79c04ce 100644
> > --- a/migration/block-dirty-bitmap.c
> > +++ b/migration/block-dirty-bitmap.c
> > @@ -1064,15 +1064,13 @@ static int dirty_bitmap_load_header(QEMUFile
> *f, DBMLoadState *s,
> > assert(nothing || s->cancelled || !!alias_map ==
> > !!bitmap_alias_map);
> >
> > if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
> > -const char *bitmap_name;
> > -
> > if (!qemu_get_counted_string(f, s->bitmap_alias)) {
> > error_report("Unable to read bitmap alias string");
> > return -EINVAL;
> > }
> >
> > -if (!s->cancelled) {
> > -if (bitmap_alias_map) {
> > +const char *bitmap_name = s->bitmap_alias;
>
> qemu’s coding style mandates declarations to be placed at the beginning of
> their block, so the declaration has to stay where it is. (Putting the
> assignment
> here looks reasonable.)
>
Hi Max,
Declaration variables here to ensure that the above exceptions(Unable to read
bitmap alias string) are avoided.
If the declaration has to stay where it is, is there a possibility that the
assignment fails?
> > +if (!s->cancelled && bitmap_alias_map) {
> > bitmap_name =
> g_hash_table_lookup(bitmap_alias_map,
> >
> s->bitmap_alias);
>
> This block of course needs to be re-indented.
>
I forgot this. I will update it later.
Thanks,
ChenQun
>
> > if (!bitmap_name) {
> > @@ -1081,9 +1079,6 @@ static int dirty_bitmap_load_header(QEMUFile *f,
> DBMLoadState *s,
> > s->bs->node_name,
> s->node_alias);
> > cancel_incoming_locked(s);
> > }
> > -} else {
> > -bitmap_name = s->bitmap_alias;
> > -}
> > }
> >
> > if (!s->cancelled) {
> >
>
RE: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
> Le 10/10/2020 à 13:07, Chen Qun a écrit :
> > This if statement judgment is redundant and it will cause a warning:
> >
> > migration/block-dirty-bitmap.c:1090:13: warning: ‘bitmap_name’ may
> > be used uninitialized in this function [-Wmaybe-uninitialized]
> > g_strlcpy(s->bitmap_name, bitmap_name,
> >> sizeof(s->bitmap_name));
> >
> >
> >>
> ^~
> >
> > Reported-by: Euler Robot
> > Signed-off-by: Chen Qun
> > ---
> > migration/block-dirty-bitmap.c | 2 --
> > 1 file changed, 2 deletions(-)
> >
> > diff --git a/migration/block-dirty-bitmap.c
> > b/migration/block-dirty-bitmap.c index 5bef793ac0..e09ea4f22b
> > 100644
> > --- a/migration/block-dirty-bitmap.c
> > +++ b/migration/block-dirty-bitmap.c
> > @@ -1084,9 +1084,7 @@ static int
> dirty_bitmap_load_header(QEMUFile
> >> *f, DBMLoadState *s,
> > } else {
> > bitmap_name = s->bitmap_alias;
> > }
> > -}
> >
> > -if (!s->cancelled) {
> > g_strlcpy(s->bitmap_name, bitmap_name,
> >> sizeof(s->bitmap_name));
> > s->bitmap = bdrv_find_dirty_bitmap(s->bs,
> > s->bitmap_name);
> >
> >
>
> I don't think it's correct as "cancel_incoming_locked(s)" can
> change the value of "s->cancelled".
>
> >>>
> >>> Hi Laurent,
> >>>
> >>> You're right. So I think this can simply assign 'bitmap_name' to
> >>> NULL to make compiler happy.
> >>
> >> Yes, and adding a comment before the second "if (!s->cancelled) {" to
> >> explain the value can be changed by "cancel_incoming_locked(s)" would
> >> avoid to have this kind of patch posted regularly to the ML.
> >>
> > Hi Laurent,
> >
> > We give the bitmap_name a default value (s->bitmap_alias) so that we can
> remove the assignment of the else branch statement.
> >
> > And then we merge the two if statements("if (!s->cancelled)", "if
> (bitmap_alias_map)") , avoids confusion the two "if (!s->cancelled)".
> >
> > Thanks,
> > Chen Qun
> >
> >
> > The code show as that:
> > @@ -1064,15 +1064,14 @@ static int dirty_bitmap_load_header(QEMUFile
> *f, DBMLoadState *s,
> > assert(nothing || s->cancelled || !!alias_map ==
> > !!bitmap_alias_map);
> >
> > if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
> > -const char *bitmap_name;
> > +const char *bitmap_name = s->bitmap_alias;
> >
> > if (!qemu_get_counted_string(f, s->bitmap_alias)) {
> > error_report("Unable to read bitmap alias string");
> > return -EINVAL;
> > }
> >
> > -if (!s->cancelled) {
> > -if (bitmap_alias_map) {
> > +if (!s->cancelled && bitmap_alias_map) {
> > bitmap_name =
> g_hash_table_lookup(bitmap_alias_map,
> >
> s->bitmap_alias);
> > if (!bitmap_name) {
> > @@ -1081,9 +1080,6 @@ static int dirty_bitmap_load_header(QEMUFile *f,
> DBMLoadState *s,
> >s->bs->node_name,
> s->node_alias);
> > cancel_incoming_locked(s);
> > }
> > -} else {
> > -bitmap_name = s->bitmap_alias;
> > -}
> > }
> >
> > if (!s->cancelled) {
> >
>
> Sounds good.
>
OK, I'll update it later.
Thanks,
Chen Qun
RE: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
> -Original Message-
> From: Laurent Vivier [mailto:laur...@vivier.eu]
> Sent: Tuesday, October 13, 2020 3:11 PM
> To: Li Qiang
> Cc: Fam Zheng ; ganqixin ;
> vsement...@virtuozzo.com; Zhanghailiang
> ; qemu-bl...@nongnu.org; Juan Quintela
> ; qemu-triv...@nongnu.org; Qemu Developers
> ; Dr. David Alan Gilbert ;
> Stefan Hajnoczi ; Euler Robot
> ; Chenqun (kuhn) ;
> Max Reitz ; John Snow
> Subject: Re: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable
> warning
>
> Le 13/10/2020 à 03:34, Li Qiang a écrit :
> > Laurent Vivier 于2020年10月12日周一 下午11:33
> 写道：
> >>
> >> Le 10/10/2020 à 13:07, Chen Qun a écrit :
> >>> This if statement judgment is redundant and it will cause a warning:
> >>>
> >>> migration/block-dirty-bitmap.c:1090:13: warning: ‘bitmap_name’ may
> >>> be used uninitialized in this function [-Wmaybe-uninitialized]
> >>> g_strlcpy(s->bitmap_name, bitmap_name,
> sizeof(s->bitmap_name));
> >>>
> >>>
> ^~
> >>>
> >>> Reported-by: Euler Robot
> >>> Signed-off-by: Chen Qun
> >>> ---
> >>> migration/block-dirty-bitmap.c | 2 --
> >>> 1 file changed, 2 deletions(-)
> >>>
> >>> diff --git a/migration/block-dirty-bitmap.c
> >>> b/migration/block-dirty-bitmap.c index 5bef793ac0..e09ea4f22b 100644
> >>> --- a/migration/block-dirty-bitmap.c
> >>> +++ b/migration/block-dirty-bitmap.c
> >>> @@ -1084,9 +1084,7 @@ static int dirty_bitmap_load_header(QEMUFile
> *f, DBMLoadState *s,
> >>> } else {
> >>> bitmap_name = s->bitmap_alias;
> >>> }
> >>> -}
> >>>
> >>> -if (!s->cancelled) {
> >>> g_strlcpy(s->bitmap_name, bitmap_name,
> sizeof(s->bitmap_name));
> >>> s->bitmap = bdrv_find_dirty_bitmap(s->bs,
> >>> s->bitmap_name);
> >>>
> >>>
> >>
> >> I don't think it's correct as "cancel_incoming_locked(s)" can change
> >> the value of "s->cancelled".
> >>
> >
> > Hi Laurent,
> >
> > You're right. So I think this can simply assign 'bitmap_name' to NULL
> > to make compiler happy.
>
> Yes, and adding a comment before the second "if (!s->cancelled) {" to explain
> the value can be changed by "cancel_incoming_locked(s)" would avoid to have
> this kind of patch posted regularly to the ML.
>
Hi Laurent,
We give the bitmap_name a default value (s->bitmap_alias) so that we can remove
the assignment of the else branch statement.
And then we merge the two if statements("if (!s->cancelled)", "if
(bitmap_alias_map)") , avoids confusion the two "if (!s->cancelled)".
Thanks,
Chen Qun
The code show as that:
@@ -1064,15 +1064,14 @@ static int dirty_bitmap_load_header(QEMUFile *f,
DBMLoadState *s,
assert(nothing || s->cancelled || !!alias_map == !!bitmap_alias_map);
if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
-const char *bitmap_name;
+const char *bitmap_name = s->bitmap_alias;
if (!qemu_get_counted_string(f, s->bitmap_alias)) {
error_report("Unable to read bitmap alias string");
return -EINVAL;
}
-if (!s->cancelled) {
-if (bitmap_alias_map) {
+if (!s->cancelled && bitmap_alias_map) {
bitmap_name = g_hash_table_lookup(bitmap_alias_map,
s->bitmap_alias);
if (!bitmap_name) {
@@ -1081,9 +1080,6 @@ static int dirty_bitmap_load_header(QEMUFile *f,
DBMLoadState *s,
s->bs->node_name, s->node_alias);
cancel_incoming_locked(s);
}
-} else {
-bitmap_name = s->bitmap_alias;
-}
}
if (!s->cancelled) {
RE: [PATCH] migration/block-dirty-bitmap: fix uninitialized variable warning
migration/block-dirty-bitmap.c
> > > b/migration/block-dirty-bitmap.c index 5bef793ac0..e09ea4f22b 100644
> > > --- a/migration/block-dirty-bitmap.c
> > > +++ b/migration/block-dirty-bitmap.c
> > > @@ -1084,9 +1084,7 @@ static int dirty_bitmap_load_header(QEMUFile
> *f, DBMLoadState *s,
> > > } else {
> > > bitmap_name = s->bitmap_alias;
> > > }
> > > -}
> > >
> > > -if (!s->cancelled) {
> > > g_strlcpy(s->bitmap_name, bitmap_name,
> sizeof(s->bitmap_name));
> > > s->bitmap = bdrv_find_dirty_bitmap(s->bs,
> > > s->bitmap_name);
> > >
> > >
> >
> > I don't think it's correct as "cancel_incoming_locked(s)" can change
> > the value of "s->cancelled".
> >
Hi Laurent,
Yes, you're right. It will be modified value when cancel_incoming_locked is
executed.
Thanks.
> So I think this can simply assign 'bitmap_name' to NULL to make
> compiler happy.
>
Hi Qiang,
I think your suggestion is feasible.
But can we just give it a default value when defining variables?
I think the default value could be "bitmap_name = s->bitmap_alias",
and then we can delete the else statement above.
Thanks,
Chen Qun
RE: [PATCH v2 09/10] hw/intc: Remove redundant statement in exynos4210_combiner_read()
> Subject: Re: [PATCH v2 09/10] hw/intc: Remove redundant statement in
> exynos4210_combiner_read()
>
> On Tue, 25 Aug 2020 at 12:26, Chen Qun wrote:
> >
> > 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
> > Signed-off-by: Chen Qun
> > ---
> > Cc: Igor Mitsyanko
> > Cc: Peter Maydell
> > ---
> > 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;
> > }
>
> The code as it stands is definitely wrong, but I'm not sure this is the
> correct fix.
> Surely the intention must have been to return the actual register value from
> the reg_set[] array, not to return 0 ?
>
> I suspect the correct fix here is simply to delete the "return 0" line and
> leave
> the assignment to val as it is.
Hi Peter,
I think you're right.
> Ideally you should check the h/w datasheet to confirm.
I checked the Exynos 4210 datasheet and found that 'Interrupt Combiner
Operation' had only five types of registers.
Register Description
Type
IESR(0/1/2/3) Interrupt Enable Set Register for Group (0~3/ 4 ~7/ 8~ 11/ 12~
15) RW
IECR(0/1/2/3) Interrupt Enable Clear Register for Group (0~3/ 4 ~7/ 8~ 11/
12~ 15) RW
ISTR(0/1/2/3) Interrupt Status Register for Group (0~3/ 4 ~7/ 8~ 11/ 12~ 15)
R
IMSR(0/1/2/3) Interrupt Masked Status Register for Group(0~3/ 4 ~7/ 8~ 11/ 12~
15) R
CIPSR0Combined Interrupt Pending Status0
R
The exynos4210_combiner_write() function has write operations for IESR and IECR.
The CIPSR, ISTR, and IMSR read operations are specified in the
exynos4210_combiner_read() function.
So, This 'default' branch in exynos4210_combiner_read() function read operation
is only for IESR and IECR.
And, in the exynos4210_combiner_write function, the default assignment
statement for IESR and IECR is " s->reg_set[offset >> 2] = val;".
Therefore, if we read the two register(IESR and IECR) values, we can directly
use this assignment statement "val = s->reg_set[offset >> 2];".
Please confirm that if there is no problem, I will modify it in later version.
Thanks,
Chen Qun
RE: [PATCH v2 03/10] target/arm/translate-a64:Remove dead assignment in handle_scalar_simd_shli()
> On Tue, 25 Aug 2020 at 12:26, Chen Qun wrote: > > > > 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". > > There is not, because TCG temps allocated via new_tmp_a64() are all freed via > free_tmp_a64() at the end of disas_a64_insn(). > OK, I'll delete that description later. Thanks, ChenQun
RE: [PATCH v2 08/10] usb/bus: Remove dead assignment in usb_get_fw_dev_path()
> > > 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..7bab0499ad
> > > 100644
> > > --- a/hw/usb/bus.c
> > > +++ b/hw/usb/bus.c
> > > @@ -612,8 +612,8 @@ static char *usb_get_fw_dev_path(DeviceState
> > > *qdev)
> >if (in[0] == '.') {
> >/* some hub between root port and device */
> >pos += snprintf(fw_path + pos, fw_len - pos,
> > "hub@%lx/", nr);
> > > 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;
> > > }
> > > }
> >
> > I'd prefer to keep the line break where it is:
> >
> > snprintf(fw_path + pos, fw_len - pos, "%s@%lx",
> > qdev_fw_name(qdev), nr);
> >
> > The patch is safe, so
> > Reviewed-by: Markus Armbruster
> >
> > The loss of symmetry betwen the two arms of the if is a bit sad. Up to
> > Gerd.
>
> If symmetry looks better. I should change it later.
Oops, I think I just misunderstood you. I agree with your suggestion to the
formatting of the "snprintf(***)" statement.
If the 'pos' assignment is useless, we delete it avoid warning that are always
detected by some tools.
Thanks,
Chen Qun
RE: [PATCH v2 08/10] usb/bus: Remove dead assignment in usb_get_fw_dev_path()
> > 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..7bab0499ad
> > 100644
> > --- a/hw/usb/bus.c
> > +++ b/hw/usb/bus.c
> > @@ -612,8 +612,8 @@ static char *usb_get_fw_dev_path(DeviceState
> > *qdev)
>if (in[0] == '.') {
>/* some hub between root port and device */
>pos += snprintf(fw_path + pos, fw_len - pos, "hub@%lx/",
> nr);
> > 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;
> > }
> > }
>
> I'd prefer to keep the line break where it is:
>
> snprintf(fw_path + pos, fw_len - pos, "%s@%lx",
> qdev_fw_name(qdev), nr);
>
> The patch is safe, so
> Reviewed-by: Markus Armbruster
>
> The loss of symmetry betwen the two arms of the if is a bit sad. Up to Gerd.
If symmetry looks better. I should change it later.
Thanks,
Chen Qun
RE: [PATCH v2 07/10] vfio/platform: Remove dead assignment in vfio_intp_interrupt()
> > 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 > > Signed-off-by: Chen Qun > > Reviewed-by: Eric Auger > > --- > > Cc: Alex Williamson > > Cc: Eric Auger > > Cc: Stefan Hajnoczi > > --- > > 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(>pending_intp_queue, > > intp, pqnext); > > -ret = event_notifier_test_and_clear(intp->interrupt); > > Shouldn't we check the 'ret' like the other place in this function? Hi, Li Qiang, Eric、Alex、Stefan has already discussed this point in the V1 version. https://patchwork.kernel.org/patch/11711897/ Thanks.
RE: [PATCH 08/11] tcg/optimize: Remove redundant statement in tcg_optimize()
> > 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 09/11] usb/bus: Remove dead assignment in usb_get_fw_dev_path()
> 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: [PULL 04/20] crypto: Redundant type conversion for AES_KEY pointer
>-Original Message- >From: Daniel P. Berrangé [mailto:berra...@redhat.com] >Sent: Monday, May 4, 2020 8:58 PM >To: Chenqun (kuhn) >Cc: qemu-devel@nongnu.org; Michael Tokarev ; qemu- >triv...@nongnu.org; Laurent Vivier ; Euler Robot > >Subject: Re: [PULL 04/20] crypto: Redundant type conversion for AES_KEY >pointer > >Hi Chen, > >This patch triggered a build failure in QEMU about discarding the "const" >qualifier. > >IOW, the type conversion is not redundant after all - it is required in order >to >explicitly discard "const". > Yes, you are right! Thank you for pointing it out ! It is my carelessness, this patch is not complete. >I believe we can probably fix this by changing >qcrypto_cipher_aes_ecb_(en|de)crypt() methods so that they also have a "const" >qualifier on the AES_KEY parameter. > It's a good point. I will update the patch later. Thanks.
RE: [PATCH v3 06/12] gdbstub: fix compiler complaining
>-Original Message-
>From: Alex Bennée [mailto:alex.ben...@linaro.org]
>Sent: Saturday, April 4, 2020 3:12 AM
>To: qemu-devel@nongnu.org
>Cc: Denis Plotnikov ; Euler Robot
>; Chenqun (kuhn) ;
>Richard Henderson ; Alex Bennée
>; Philippe Mathieu-Daudé
>Subject: [PATCH v3 06/12] gdbstub: fix compiler complaining
>
>From: Denis Plotnikov
>
>./gdbstub.c: In function ‘handle_query_thread_extra’:
>/usr/include/glib-2.0/glib/glib-autocleanups.h:28:10:
>error: ‘cpu_name’ may be used uninitialized in this function
>[-Werror=maybe-uninitialized]
>g_free (*pp);
> ^
>./gdbstub.c:2063:26: note: ‘cpu_name’ was declared here
>g_autofree char *cpu_name;
> ^
>cc1: all warnings being treated as errors
>
>Signed-off-by: Denis Plotnikov
>Message-Id: <20200326151407.25046-1-dplotni...@virtuozzo.com>
>Reported-by: Euler Robot
>Reported-by: Chen Qun
>Reviewed-by: Richard Henderson
>Signed-off-by: Alex Bennée
>Reviewed-by: Philippe Mathieu-Daudé
>---
Add Miroslav Rezanina's "Reviewed-by" tag.
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg07651.html
Reviewed-by: Miroslav Rezanina
Thanks.
> gdbstub.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/gdbstub.c b/gdbstub.c
>index 013fb1ac0f1..171e1509509 100644
>--- a/gdbstub.c
>+++ b/gdbstub.c
>@@ -2060,8 +2060,8 @@ static void
>handle_query_thread_extra(GdbCmdContext *gdb_ctx, void *user_ctx)
> /* Print the CPU model and name in multiprocess mode */
> ObjectClass *oc = object_get_class(OBJECT(cpu));
> const char *cpu_model = object_class_get_name(oc);
>-g_autofree char *cpu_name;
>-cpu_name = object_get_canonical_path_component(OBJECT(cpu));
>+g_autofree char *cpu_name =
>+object_get_canonical_path_component(OBJECT(cpu));
> g_string_printf(rs, "%s %s [%s]", cpu_model, cpu_name,
> cpu->halted ? "halted " : "running");
> } else {
>--
>2.20.1
RE: [PATCH v5 0/3] redundant code: Fix warnings reported by Clang static code analyzer
>-Original Message- >From: Laurent Vivier [mailto:laur...@vivier.eu] >Sent: Friday, April 3, 2020 4:28 PM >To: Chenqun (kuhn) ; qemu-devel@nongnu.org; >qemu-triv...@nongnu.org >Cc: Zhanghailiang ; phi...@redhat.com >Subject: Re: [PATCH v5 0/3] redundant code: Fix warnings reported by Clang >static code analyzer > >Le 03/04/2020 à 10:22, Laurent Vivier a écrit : >> Le 03/04/2020 à 10:10, Chenqun (kuhn) a écrit : >>>> -Original Message- >>>> From: Laurent Vivier [mailto:laur...@vivier.eu] >>>> Sent: Friday, April 3, 2020 4:04 PM >>>> To: Chenqun (kuhn) ; qemu- >de...@nongnu.org; >>>> qemu-triv...@nongnu.org >>>> Cc: Zhanghailiang ; >>>> phi...@redhat.com >>>> Subject: Re: [PATCH v5 0/3] redundant code: Fix warnings reported by >>>> Clang static code analyzer >>>> >>>> Le 03/04/2020 à 09:51, Chenqun (kuhn) a écrit : >>>>> Ping! >>>>> >>>>> This series has been reviewed. Could someone please pick this up >>>>> (e.g. qemu- >>>> trivial?)? >>>> >>>> As we are in hard feature freeze now and this is not critical bug >>>> fixes I'm going to queue them for 5.1 except if you have good arguments to >have them in 5.0. >>>> >>> OK, I get it. >>> It is important to ensure a stable version! >> >> Queued to my linux-user-for-5.1 queue. > >I meant trivial-patches-for-5.1 > Thanks. Could you add another trivial patch to the queue by the way? https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg07534.html
RE: [PATCH v5 0/3] redundant code: Fix warnings reported by Clang static code analyzer
>-Original Message- >From: Laurent Vivier [mailto:laur...@vivier.eu] >Sent: Friday, April 3, 2020 4:04 PM >To: Chenqun (kuhn) ; qemu-devel@nongnu.org; >qemu-triv...@nongnu.org >Cc: Zhanghailiang ; phi...@redhat.com >Subject: Re: [PATCH v5 0/3] redundant code: Fix warnings reported by Clang >static code analyzer > >Le 03/04/2020 à 09:51, Chenqun (kuhn) a écrit : >> Ping! >> >> This series has been reviewed. Could someone please pick this up (e.g. qemu- >trivial?)? > >As we are in hard feature freeze now and this is not critical bug fixes I'm >going >to queue them for 5.1 except if you have good arguments to have them in 5.0. > OK, I get it. It is important to ensure a stable version! Thanks. >>> -Original Message- >>> From: Chenqun (kuhn) >>> Sent: Wednesday, March 25, 2020 10:59 AM >>> To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org >>> Cc: Zhanghailiang ; >>> laur...@vivier.eu; phi...@redhat.com; Chenqun (kuhn) >>> >>> Subject: [PATCH v5 0/3] redundant code: Fix warnings reported by >>> Clang static code analyzer >>> >>> v1->v2: >>> - Patch1: Add John Snow review comment. >>> - Patch9: Move the 'dst_type' declaration to while() statement. >>> - Patch12: Add Philippe Mathieu-Daud茅 review comment. >>> - Patch13: Move the 'set' declaration to the for() statement. >>> >>> v2->v3: >>> - Patch1: Add Kevin Wolf review comment. >>> - Patch2: Keep the 'flags' then use it(Base on Kevin's comments). >>> - Patch3: Add Kevin Wolf review comment. >>> - Patch9: Add Francisco Iglesias and Alistair Francis review comment. >>> - Patch10: Juan Quintela has added it to the queue and delete it. >>> - Patch12->Patch11: Add Philippe Mathieu-Daud茅 review comment. >>> - Patch13->Patch12: Add Philippe Mathieu-Daud茅 review comment. >>> >>> v3->v4: >>> - Deleted the patches that have been merged in the v3. >>> - Modify "scsi/esp-pci" Patch, use g_assert with variable size. >>> >>> v4->v5: >>> - Patch1: Add Laurent Vivier review comment and change the subject. >>> - Patch2: Use extract16() instead of delete bit operation statement. >>> - Patch3: Add Laurent Vivier review comment. >>> >>> Chen Qun (3): >>> scsi/esp-pci: add g_assert() for fix clang analyzer warning in >>>esp_pci_io_write() >>> display/blizzard: use extract16() for fix clang analyzer warning in >>>blizzard_draw_line16_32() >>> timer/exynos4210_mct: Remove redundant statement in >>>exynos4210_mct_write() >>> >>> hw/display/blizzard.c | 10 -- >>> hw/scsi/esp-pci.c | 1 + >>> hw/timer/exynos4210_mct.c | 4 >>> 3 files changed, 5 insertions(+), 10 deletions(-) >>> >>> -- >>> 2.23.0 >>> >>
RE: [PATCH v5 0/3] redundant code: Fix warnings reported by Clang static code analyzer
Ping! This series has been reviewed. Could someone please pick this up (e.g. qemu-trivial?)? >-Original Message- >From: Chenqun (kuhn) >Sent: Wednesday, March 25, 2020 10:59 AM >To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org >Cc: Zhanghailiang ; laur...@vivier.eu; >phi...@redhat.com; Chenqun (kuhn) >Subject: [PATCH v5 0/3] redundant code: Fix warnings reported by Clang static >code analyzer > >v1->v2: >- Patch1: Add John Snow review comment. >- Patch9: Move the 'dst_type' declaration to while() statement. >- Patch12: Add Philippe Mathieu-Daud茅 review comment. >- Patch13: Move the 'set' declaration to the for() statement. > >v2->v3: >- Patch1: Add Kevin Wolf review comment. >- Patch2: Keep the 'flags' then use it(Base on Kevin's comments). >- Patch3: Add Kevin Wolf review comment. >- Patch9: Add Francisco Iglesias and Alistair Francis review comment. >- Patch10: Juan Quintela has added it to the queue and delete it. >- Patch12->Patch11: Add Philippe Mathieu-Daud茅 review comment. >- Patch13->Patch12: Add Philippe Mathieu-Daud茅 review comment. > >v3->v4: >- Deleted the patches that have been merged in the v3. >- Modify "scsi/esp-pci" Patch, use g_assert with variable size. > >v4->v5: >- Patch1: Add Laurent Vivier review comment and change the subject. >- Patch2: Use extract16() instead of delete bit operation statement. >- Patch3: Add Laurent Vivier review comment. > >Chen Qun (3): > scsi/esp-pci: add g_assert() for fix clang analyzer warning in >esp_pci_io_write() > display/blizzard: use extract16() for fix clang analyzer warning in >blizzard_draw_line16_32() > timer/exynos4210_mct: Remove redundant statement in >exynos4210_mct_write() > > hw/display/blizzard.c | 10 -- > hw/scsi/esp-pci.c | 1 + > hw/timer/exynos4210_mct.c | 4 > 3 files changed, 5 insertions(+), 10 deletions(-) > >-- >2.23.0 >
RE: [PATCH 1/3] gdbstub: prevent uninitialized warning
>-Original Message-
>From: Alex Bennée [mailto:alex.ben...@linaro.org]
>Sent: Friday, March 27, 2020 5:13 PM
>To: Chenqun (kuhn)
>Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org; Zhanghailiang
>; laur...@vivier.eu; Euler Robot
>; Philippe Mathieu-Daudé
>Subject: Re: [PATCH 1/3] gdbstub: prevent uninitialized warning
>
>
>Chen Qun writes:
>
>> According to the glib function requirements, we need initialise
>> the variable. Otherwise there will be compilation warnings:
>>
>> qemu/gdbstub.c: In function ‘handle_query_thread_extra’:
>> /usr/include/glib-2.0/glib/glib-autocleanups.h:28:3: warning:
>> ‘cpu_name’ may be used uninitialized in this function
>> [-Wmaybe-uninitialized]
>>g_free (*pp);
>>^~~~
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Chen Qun
>
>Thanks,
>
>I've pulled in a duplicate fix from:
>
> Message-Id: <20200326151407.25046-1-dplotni...@virtuozzo.com>
>
>and added your Reported-by's
OK, If possible, bring Miroslav Rezanina's "Reviewed-by" tag.
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg07651.html
Thanks.
>
>> ---
>> Cc: "Alex Bennée"
>> Cc: "Philippe Mathieu-Daudé"
>> ---
>> gdbstub.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/gdbstub.c b/gdbstub.c
>> index 013fb1ac0f..171e150950 100644
>> --- a/gdbstub.c
>> +++ b/gdbstub.c
>> @@ -2060,8 +2060,8 @@ static void
>handle_query_thread_extra(GdbCmdContext *gdb_ctx, void *user_ctx)
>> /* Print the CPU model and name in multiprocess mode */
>> ObjectClass *oc = object_get_class(OBJECT(cpu));
>> const char *cpu_model = object_class_get_name(oc);
>> -g_autofree char *cpu_name;
>> -cpu_name = object_get_canonical_path_component(OBJECT(cpu));
>> +g_autofree char *cpu_name =
>> +object_get_canonical_path_component(OBJECT(cpu));
>> g_string_printf(rs, "%s %s [%s]", cpu_model, cpu_name,
>> cpu->halted ? "halted " : "running");
>> } else {
>
>
>--
>Alex Bennée
RE: [PATCH 3/3] crypto: Redundant type conversion for AES_KEY pointer
>-Original Message-
>From: Laurent Vivier [mailto:laur...@vivier.eu]
>Sent: Wednesday, March 25, 2020 5:45 PM
>To: Chenqun (kuhn) ; qemu-devel@nongnu.org;
>qemu-triv...@nongnu.org
>Cc: Zhanghailiang ; Euler Robot
>; Daniel P. Berrangé
>Subject: Re: [PATCH 3/3] crypto: Redundant type conversion for AES_KEY pointer
>
>Le 25/03/2020 à 10:21, Chen Qun a écrit :
>> Fix: eaec903c5b8
>>
>
>Did you run the coccinelle script scripts/coccinelle/typecast.cocci ?
>
Yes, I run it and plan to integrate it into EulerRobot so that similar issues
can be discovered sooner.
Thanks.
>
>> Reported-by: Euler Robot
>> Signed-off-by: Chen Qun
>> ---
>> Cc: "Daniel P. Berrangé"
>> ---
>> crypto/cipher-builtin.c | 6 ++
>> 1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/crypto/cipher-builtin.c b/crypto/cipher-builtin.c index
>> bf8413e71a..99d6280a16 100644
>> --- a/crypto/cipher-builtin.c
>> +++ b/crypto/cipher-builtin.c
>> @@ -133,8 +133,7 @@ static void qcrypto_cipher_aes_xts_encrypt(const
>> void *ctx, {
>> const QCryptoCipherBuiltinAESContext *aesctx = ctx;
>>
>> -qcrypto_cipher_aes_ecb_encrypt((AES_KEY *)>enc,
>> - src, dst, length);
>> +qcrypto_cipher_aes_ecb_encrypt(>enc, src, dst, length);
>> }
>>
>>
>> @@ -145,8 +144,7 @@ static void qcrypto_cipher_aes_xts_decrypt(const
>> void *ctx, {
>> const QCryptoCipherBuiltinAESContext *aesctx = ctx;
>>
>> -qcrypto_cipher_aes_ecb_decrypt((AES_KEY *)>dec,
>> - src, dst, length);
>> +qcrypto_cipher_aes_ecb_decrypt(>dec, src, dst, length);
>> }
>>
>>
>>
RE: [PATCH v4 3/3] timer/exynos4210_mct: Remove redundant statement in exynos4210_mct_write()
>-Original Message-
>From: Laurent Vivier [mailto:laur...@vivier.eu]
>Sent: Tuesday, March 24, 2020 6:59 PM
>To: Chenqun (kuhn) ; qemu-devel@nongnu.org;
>qemu-triv...@nongnu.org
>Cc: Zhanghailiang ; phi...@redhat.com;
>Euler Robot ; Igor Mitsyanko
>; Peter Maydell
>Subject: Re: [PATCH v4 3/3] timer/exynos4210_mct: Remove redundant
>statement in exynos4210_mct_write()
>
>Le 24/03/2020 à 09:22, Chen Qun a écrit :
>> Clang static code analyzer show warning:
>> hw/timer/exynos4210_mct.c:1370:9: warning: Value stored to 'index' is never
>read
>> index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
>> ^ ~
>> hw/timer/exynos4210_mct.c:1399:9: warning: Value stored to 'index' is never
>read
>> index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
>> ^ ~
>> hw/timer/exynos4210_mct.c:1441:9: warning: Value stored to 'index' is never
>read
>> index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
>> ^ ~
>
>It would be interesting to understand why we need the index for these registers
>in exynos4210_mct_read() and not in exynos4210_mct_write().
>
I think the index can also be used in exynos4210_mct_write(), but the original
author used a more obvious reg name instead of it.
The obvious reg name:
case L0_TCNTB: case L1_TCNTB:
reg name is : L_REG_CNT_TCNTB
case L0_ICNTB: case L1_ICNTB:
reg name is : L_REG_CNT_ICNTB
case L0_FRCNTB: case L1_FRCNTB:
reg name is : L_REG_CNT_FRCCNTB
Thanks.
>
>> Reported-by: Euler Robot
>> Signed-off-by: Chen Qun
>> ---
>> Cc: Igor Mitsyanko
>> Cc: Peter Maydell
>> ---
>> hw/timer/exynos4210_mct.c | 4
>> 1 file changed, 4 deletions(-)
>>
>> diff --git a/hw/timer/exynos4210_mct.c b/hw/timer/exynos4210_mct.c
>> index 944120aea5..570cf7075b 100644
>> --- a/hw/timer/exynos4210_mct.c
>> +++ b/hw/timer/exynos4210_mct.c
>> @@ -1367,7 +1367,6 @@ static void exynos4210_mct_write(void *opaque,
>> hwaddr offset,
>>
>> case L0_TCNTB: case L1_TCNTB:
>> lt_i = GET_L_TIMER_IDX(offset);
>> -index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
>>
>> /*
>> * TCNTB is updated to internal register only after CNT expired.
>> @@ -1396,7 +1395,6 @@ static void exynos4210_mct_write(void *opaque,
>> hwaddr offset,
>>
>> case L0_ICNTB: case L1_ICNTB:
>> lt_i = GET_L_TIMER_IDX(offset);
>> -index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
>>
>> s->l_timer[lt_i].reg.wstat |= L_WSTAT_ICNTB_WRITE;
>> s->l_timer[lt_i].reg.cnt[L_REG_CNT_ICNTB] = value & @@
>> -1438,8 +1436,6 @@ static void exynos4210_mct_write(void *opaque,
>> hwaddr offset,
>>
>> case L0_FRCNTB: case L1_FRCNTB:
>> lt_i = GET_L_TIMER_IDX(offset);
>> -index = GET_L_TIMER_CNT_REG_IDX(offset, lt_i);
>> -
>> DPRINTF("local timer[%d] FRCNTB write %llx\n", lt_i, value);
>>
>> s->l_timer[lt_i].reg.wstat |= L_WSTAT_FRCCNTB_WRITE;
>>
RE: [PATCH v4 2/3] display/blizzard: Remove redundant statement in blizzard_draw_line16_32()
>-Original Message- >From: Laurent Vivier [mailto:laur...@vivier.eu] >Sent: Tuesday, March 24, 2020 4:40 PM >To: Chenqun (kuhn) ; qemu-devel@nongnu.org; >qemu-triv...@nongnu.org >Cc: Zhanghailiang ; phi...@redhat.com; >Euler Robot ; Andrzej Zaborowski >; Peter Maydell >Subject: Re: [PATCH v4 2/3] display/blizzard: Remove redundant statement in >blizzard_draw_line16_32() > >Le 24/03/2020 à 09:38, Laurent Vivier a écrit : >> Le 24/03/2020 à 09:22, Chen Qun a écrit : >>> Clang static code analyzer show warning: >>> hw/display/blizzard.c:940:9: warning: Value stored to 'data' is never read >>> data >>= 5; >>> ^~ >>> Reported-by: Euler Robot >>> Signed-off-by: Chen Qun >>> --- >>> Cc: Andrzej Zaborowski >>> Cc: Peter Maydell >>> --- >>> hw/display/blizzard.c | 1 - >>> 1 file changed, 1 deletion(-) >>> >>> diff --git a/hw/display/blizzard.c b/hw/display/blizzard.c index >>> 359e399c2a..62517bdf75 100644 >>> --- a/hw/display/blizzard.c >>> +++ b/hw/display/blizzard.c >>> @@ -937,7 +937,6 @@ static void blizzard_draw_line16_32(uint32_t *dest, >>> g = (data & 0x3f) << 2; >>> data >>= 6; >>> r = (data & 0x1f) << 3; >>> -data >>= 5; >>> *dest++ = rgb_to_pixel32(r, g, b); >>> } >>> } >>> >> >> Perhaps it would be clearer to use extract32() to compute r, g and b? > >in fact extract16() as data is uint16_t... Good Point, I will update it next verison. Thanks.
RE: [PATCH-for-5.0 v2 00/11] misc: Trivial static code analyzer fixes
>-Original Message- >From: Qemu-devel [mailto:qemu-devel- >bounces+kuhn.chenqun=huawei@nongnu.org] On Behalf Of Laurent Vivier >Sent: Monday, March 23, 2020 10:56 PM >To: Philippe Mathieu-Daudé ; qemu-devel@nongnu.org >Cc: Fam Zheng ; Peter Maydell ; >Michael S. Tsirkin ; Mark Cave-Ayland ayl...@ilande.co.uk>; qemu-bl...@nongnu.org; qemu-triv...@nongnu.org; >Markus Armbruster ; Hervé Poussineau >; Joel Stanley ; Michael Tokarev >; Alistair Francis ; qemu- >a...@nongnu.org; Cédric Le Goater ; John Snow >; David Gibson ; Kevin Wolf >; Andrew Jeffery ; Max Reitz >; Igor Mitsyanko ; qemu- >p...@nongnu.org; Paolo Bonzini >Subject: Re: [PATCH-for-5.0 v2 00/11] misc: Trivial static code analyzer fixes > >Le 23/03/2020 à 15:45, Philippe Mathieu-Daudé a écrit : >> On 3/23/20 3:32 PM, Laurent Vivier wrote: >>> Le 21/03/2020 à 15:40, Philippe Mathieu-Daudé a écrit : Fix trivial warnings reported by the Clang static code analyzer. Since v1: - Addressed Markus/Zoltan/Aleksandar review comments Philippe Mathieu-Daudé (11): block: Avoid dead assignment blockdev: Remove dead assignment hw/i2c/pm_smbus: Remove dead assignment hw/input/adb-kbd: Remove dead assignment hw/ide/sii3112: Remove dead assignment hw/isa/i82378: Remove dead assignment hw/gpio/aspeed_gpio: Remove dead assignment hw/timer/exynos4210_mct: Remove dead assignments Same as this: https://patchwork.kernel.org/patch/11415527/ hw/timer/stm32f2xx_timer: Remove dead assignment hw/timer/pxa2xx_timer: Add assertion to silent static analyzer warning hw/scsi/esp-pci: Remove dead assignment Same as this: https://patchwork.kernel.org/patch/11415529/ block.c | 2 +- blockdev.c | 2 +- hw/gpio/aspeed_gpio.c | 2 +- hw/i2c/pm_smbus.c | 1 - hw/ide/sii3112.c | 5 +++-- hw/input/adb-kbd.c | 6 +- hw/isa/i82378.c | 8 hw/scsi/esp-pci.c | 1 - hw/timer/exynos4210_mct.c | 3 --- hw/timer/pxa2xx_timer.c | 1 + hw/timer/stm32f2xx_timer.c | 1 - 11 files changed, 12 insertions(+), 20 deletions(-) >>> >>> I think your series covers cases already covered by: >>> >>> [PATCH v3 00/12] redundant code: Fix warnings reported by Clang >>> static code analyzer >>> https://patchew.org/QEMU/20200302130715.29440-1-kuhn.ch >> >> Unfortunately [for me...] I don't have v3 in my INBOX... *sigh* This >> was 3 weeks ago. *sigh*. >> >> I can see the series in the archives: >> https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg00219.html >> But I can't find the outcome, was it queued in the trivial tree? >> Any idea when this will be merged in the master tree? > >Some patches are already merged via trivial (1, 2 (should go by SCSI >queue) 3, 5, 6, 7, 9, 11 (by USB queue), 12). > >But others needed R-b tags or new version. I didn't check which of your patches >are already covered by this series. > Hi, Laurent and Philippe Yes, there are currently three patches( 4、8 、10) that are not merged. And, only patch4 and patch10 have the same points as this series. I should update it again earlier. Thanks.
RE: [PATCH v3] block/iscsi:use the flags in iscsi_open() prevent Clang warning
Gentle ping.
Any other suggestions about this?
Thanks.
>-Original Message-
>From: Chenqun (kuhn)
>Sent: Wednesday, March 11, 2020 11:29 AM
>To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org
>Cc: Zhanghailiang ; Chenqun (kuhn)
>; Euler Robot ;
>Kevin Wolf ; Ronnie Sahlberg
>; Paolo Bonzini ; Peter
>Lieven ; Max Reitz ; Laurent Vivier
>
>Subject: [PATCH v3] block/iscsi:use the flags in iscsi_open() prevent Clang
>warning
>
>Clang static code analyzer show warning:
> block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read
>flags &= ~BDRV_O_RDWR;
>^
>
>In iscsi_allocmap_init() only checks BDRV_O_NOCACHE, which is the same in
>both of flags and bs->open_flags.
>We can use the flags instead bs->open_flags to prevent Clang warning.
>
>Reported-by: Euler Robot
>Signed-off-by: Chen Qun
>Reviewed-by: Kevin Wolf
>---
>Cc: Ronnie Sahlberg
>Cc: Paolo Bonzini
>Cc: Peter Lieven
>Cc: Kevin Wolf
>Cc: Max Reitz
>Cc: Laurent Vivier
>
>v1->v2:
> Keep the 'flags' then use it(Base on Kevin's comments).
>
>v2->v3:
> Modify subject and commit messages(Base on Kevin's and Laurent's
>comments).
>---
> block/iscsi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/block/iscsi.c b/block/iscsi.c index 682abd8e09..50bae51700 100644
>--- a/block/iscsi.c
>+++ b/block/iscsi.c
>@@ -2002,7 +2002,7 @@ static int iscsi_open(BlockDriverState *bs, QDict
>*options, int flags,
> iscsilun->cluster_size = iscsilun->bl.opt_unmap_gran *
> iscsilun->block_size;
> if (iscsilun->lbprz) {
>-ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
>+ret = iscsi_allocmap_init(iscsilun, flags);
> }
> }
>
>--
>2.23.0
>
RE: [PATCH] qom-qmp-cmds: Fix another memory leak in qmp_object_add()
>-Original Message-
>From: Qemu-devel [mailto:qemu-devel-
>bounces+kuhn.chenqun=huawei@nongnu.org] On Behalf Of Markus
>Armbruster
>Sent: Tuesday, March 17, 2020 5:23 PM
>To: qemu-devel@nongnu.org
>Cc: Kevin Wolf ; pbonz...@redhat.com;
>berra...@redhat.com; ehabk...@redhat.com
>Subject: [PATCH] qom-qmp-cmds: Fix another memory leak in
>qmp_object_add()
>
>When user_creatable_add_type() fails, qmp_object_add() returns both its
>error and the usual empty QDict success value. The QMP core handles the
>error, and ignores the success value, leaking it. Exposed by qmp-cmd-test
>case /x86_64/qmp/object-add-without-props, and duly reported both by
>ASan and valgrind.
>
>To plug the leak, set the success value only on success.
>
>Fixes: 5f07c4d60d091320186e7b0edaf9ed2cc16b2d1e
>Cc: Kevin Wolf
>Signed-off-by: Markus Armbruster
>---
Hi, Markus
Looks like the same patch that has been reported already here:
https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg03928.html
Maybe we should initialize ret_data in xen-block to avoid a possible
uninitialized error ？
Thanks.
> qom/qom-qmp-cmds.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/qom/qom-qmp-cmds.c b/qom/qom-qmp-cmds.c index
>435193b036..6bd137ccbf 100644
>--- a/qom/qom-qmp-cmds.c
>+++ b/qom/qom-qmp-cmds.c
>@@ -287,8 +287,8 @@ void qmp_object_add(QDict *qdict, QObject
>**ret_data, Error **errp)
> visit_free(v);
> if (obj) {
> object_unref(obj);
>+*ret_data = QOBJECT(qdict_new());
> }
>-*ret_data = QOBJECT(qdict_new());
> }
>
> void qmp_object_del(const char *id, Error **errp)
>--
>2.21.1
>
RE: [PATCH v4] hw/net/imx_fec: write TGSR and TCSR3 in imx_enet_write()
>-Original Message- >From: Peter Maydell [mailto:peter.mayd...@linaro.org] >Sent: Friday, March 13, 2020 10:29 PM >To: Chenqun (kuhn) >Cc: QEMU Developers ; QEMU Trivial triv...@nongnu.org>; Zhanghailiang ; >Euler Robot ; Jason Wang >; Peter Chubb >Subject: Re: [PATCH v4] hw/net/imx_fec: write TGSR and TCSR3 in >imx_enet_write() > >On Fri, 13 Mar 2020 at 12:33, Chen Qun wrote: >> >> The current code causes clang static code analyzer generate warning: >> hw/net/imx_fec.c:858:9: warning: Value stored to 'value' is never read >> value = value & 0x000f; >> ^ ~~ >> hw/net/imx_fec.c:864:9: warning: Value stored to 'value' is never read >> value = value & 0x00fd; >> ^ ~~ >> >> According to the definition of the function, the two “value” >> assignments should be written to registers. >> >> Reported-by: Euler Robot >> Signed-off-by: Chen Qun > > >Applied to target-arm.next; thanks for working through the code review >process. > Thank you for your detailed review and effective suggestions, too.
RE: [PATCH v3] hw/net/imx_fec: write TGSR and TCSR3 in imx_enet_write()
>-Original Message- >From: Peter Maydell [mailto:peter.mayd...@linaro.org] >Sent: Friday, March 13, 2020 7:31 PM >To: Chenqun (kuhn) >Cc: QEMU Developers ; QEMU Trivial triv...@nongnu.org>; Zhanghailiang ; >Euler Robot ; Jason Wang >; Peter Chubb >Subject: Re: [PATCH v3] hw/net/imx_fec: write TGSR and TCSR3 in >imx_enet_write() > >On Fri, 13 Mar 2020 at 03:23, Chen Qun wrote: >> >> The current code causes clang static code analyzer generate warning: >> hw/net/imx_fec.c:858:9: warning: Value stored to 'value' is never read >> value = value & 0x000f; >> ^ ~~ >> hw/net/imx_fec.c:864:9: warning: Value stored to 'value' is never read >> value = value & 0x00fd; >> ^ ~~ >> >> According to the definition of the function, the two “value” >> assignments should be written to registers. >> >> Reported-by: Euler Robot >> Signed-off-by: Chen Qun >> --- >> Cc: Peter Maydell >> Cc: Jason Wang >> Cc: Peter Chubb >> >> v1->v2: >> The register 'ENET_TGSR' write-1-to-clear timer flag. >> The register 'ENET_TCSRn' 7bit(TF) write-1-to-clear timer flag. >> >> v2->v3: >> Optimize code style, based on discussions with Peter. >> --- >> hw/net/imx_fec.c | 9 ++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c index >> 6a124a154a..3547975710 100644 >> --- a/hw/net/imx_fec.c >> +++ b/hw/net/imx_fec.c >> @@ -854,14 +854,17 @@ static void imx_enet_write(IMXFECState *s, >uint32_t index, uint32_t value) >> s->regs[index] = value & 0x7f7f; >> break; >> case ENET_TGSR: >> -/* implement clear timer flag */ >> -value = value & 0x000f; >> +/* implement clear timer flag, 0-3 bits W1C, reserved bits write >> zero */ >> +s->regs[index] &= ~(value & 0x000f) & 0x000f; > >I think I must have misunderstood what you were suggesting in your previous >question. >The final & with 0x000f here is unnecessary, because those bits are >always 0 in s->regs[index] anyway. > >> break; >> case ENET_TCSR0: >> case ENET_TCSR1: >> case ENET_TCSR2: >> case ENET_TCSR3: >> -value = value & 0x00fd; >> +/* 7 bits W1C, reserved bits write zero */ >> +s->regs[index] &= ~(value & 0x0080) & 0x00ff; > >Similarly here. > >> +s->regs[index] &= ~0x007d; /* writable fields */ >> +s->regs[index] |= (value & 0x007d); >> break; >> case ENET_TCCR0: >> case ENET_TCCR1: > >Short answer: my recommendation is to use the expressions I recommended >that you use... All right, I change it later. Thanks.
RE: [PATCH v2] hw/net/imx_fec: write TGSR and TCSR3 in imx_enet_write()
>-Original Message- >From: Peter Maydell [mailto:peter.mayd...@linaro.org] >Sent: Friday, March 13, 2020 1:01 AM >To: Chenqun (kuhn) >Cc: QEMU Developers ; QEMU Trivial triv...@nongnu.org>; Zhanghailiang ; >Jason Wang ; Peter Chubb >; qemu-arm ; Euler >Robot >Subject: Re: [PATCH v2] hw/net/imx_fec: write TGSR and TCSR3 in >imx_enet_write() > >On Tue, 10 Mar 2020 at 08:08, Chenqun (kuhn) >wrote: >> >> >-Original Message- >> >From: Peter Maydell [mailto:peter.mayd...@linaro.org] >> >> >> >> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c index >> >> 6a124a154a..322cbdcc17 100644 >> >> --- a/hw/net/imx_fec.c >> >> +++ b/hw/net/imx_fec.c >> >> @@ -855,13 +855,15 @@ static void imx_enet_write(IMXFECState *s, >> >uint32_t index, uint32_t value) >> >> break; >> >> case ENET_TGSR: >> >> /* implement clear timer flag */ >> >> -value = value & 0x000f; >> >> +s->regs[index] ^= s->regs[index] & value; >> >> +s->regs[index] &= 0x000f; >> >> break; >> >> case ENET_TCSR0: >> >> case ENET_TCSR1: >> >> case ENET_TCSR2: >> >> case ENET_TCSR3: >> >> -value = value & 0x00fd; >> >> +s->regs[index] = (value & 0x0080) ? (0x007d & value) : >> >> + (value & 0x00fd); >> >> break; >> >> case ENET_TCCR0: >> >> case ENET_TCCR1: >> > >> >This isn't the usual way to write W1C behaviour. >> >If all the relevant bits are W1C, as for TGSR: >> > >> > s->regs[index] &= ~(value & 0xf); /* all bits W1C */ >> > >> Yes, it looks better. >> But do we need clear the reserved bit (31 - 4 bits) explicitly ? > >Not necessarily, but it seems to be how the other registers in the device have >generally been coded, and it's clearly what the intent was here given that the >original (buggy) code was masking out reserved bits. So I think it makes sense >to continue in that style. > OK, let's keep original code style, and clear reserved bit. I will provide v3 version for it. Thanks.
RE: [PATCH] hw/scsi/megasas:Clean up some redundant code fix Clang warnings
>-Original Message-
>From: Laurent Vivier [mailto:laur...@vivier.eu]
>Sent: Tuesday, March 10, 2020 11:01 PM
>To: Chenqun (kuhn) ; qemu-
>de...@nongnu.org; qemu-triv...@nongnu.org
>Cc: Fam Zheng ; Hannes Reinecke ;
>Zhanghailiang ; qemu-bl...@nongnu.org;
>Euler Robot ; Paolo Bonzini
>
>Subject: Re: [PATCH] hw/scsi/megasas:Clean up some redundant code fix
>Clang warnings
>
>Le 10/03/2020 à 14:08, Chen Qun a écrit :
>> Here are some redundant statements, we can clean them up.
>> Clang static code analyzer show warning:
>> hw/scsi/megasas.c:1175:32: warning: Value stored to 'max_ld_disks' during
>its initialization is never read
>> uint32_t num_ld_disks = 0, max_ld_disks = s->fw_luns;
>>^~~~ ~~
>> hw/scsi/megasas.c:1183:9: warning: Value stored to 'max_ld_disks' is never
>read
>> max_ld_disks = 0;
>> ^ ~
>
>This has been introduced by:
>
>d97ae3684863 ("megasas: fixup MFI_DCMD_LD_LIST_QUERY")
>
>And modified by:
>
>commit 3f2cd4dd47719497540fb0e0aa0635e127f2838f
>
Yes, this modification makes the first piece of code(the if statement in
megasas.c:1183) look meaningless and resulting in warning.
Maybe we can make this piece code better, although my modification is wrong.
Thanks.
>Author: Hannes Reinecke
>Date: Wed Oct 29 13:00:07 2014 +0100
>
>megasas: fixup device mapping
>
>Logical drives can only be addressed with the 'target_id' number;
>LUN numbers cannot be selected.
>Physical drives can be selected with both, target and LUN id.
>
>So we should disallow LUN numbers not equal to 0 when in
>RAID mode.
>
>Signed-off-by: Hannes Reinecke
>Signed-off-by: Paolo Bonzini ...
>@@ -1143,10 +1152,13 @@ static int
>megasas_dcmd_ld_list_query(MegasasState *s, MegasasCmd *cmd)
> return MFI_STAT_INVALID_PARAMETER;
> }
> dcmd_size = sizeof(uint32_t) * 2 + 3;
>-
>+max_ld_disks = cmd->iov_size - dcmd_size;
> if (megasas_is_jbod(s)) {
> max_ld_disks = 0;
> }
>+if (max_ld_disks > MFI_MAX_LD) {
>+max_ld_disks = MFI_MAX_LD;
>+}
> QTAILQ_FOREACH(kid, >bus.qbus.children, sibling) {
> SCSIDevice *sdev = DO_UPCAST(SCSIDevice, qdev, kid->child); ...
>
>
>Thanks,
>Laurent
RE: [PATCH v3 02/12] block/iscsi:Remove redundant statement in iscsi_open()
>-Original Message-
>From: Laurent Vivier [mailto:laur...@vivier.eu]
>Sent: Tuesday, March 10, 2020 11:02 PM
>To: Kevin Wolf ; Chenqun (kuhn)
>
>Cc: peter.mayd...@linaro.org; Zhanghailiang
>; qemu-triv...@nongnu.org; Peter Lieven
>; qemu-devel@nongnu.org; Max Reitz ;
>Ronnie Sahlberg ; Euler Robot
>; Paolo Bonzini
>Subject: Re: [PATCH v3 02/12] block/iscsi:Remove redundant statement in
>iscsi_open()
>
>Le 10/03/2020 à 15:26, Kevin Wolf a écrit :
>> Am 02.03.2020 um 14:07 hat Chen Qun geschrieben:
>>> Clang static code analyzer show warning:
>>> block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read
>>> flags &= ~BDRV_O_RDWR;
>>> ^
>>>
>>> Reported-by: Euler Robot
>>> Signed-off-by: Chen Qun
>>> ---
>>> Cc: Ronnie Sahlberg
>>> Cc: Paolo Bonzini
>>> Cc: Peter Lieven
>>> Cc: Kevin Wolf
>>> Cc: Max Reitz
>>>
>>> v1->v2:
>>> Keep the 'flags' then use it(Base on Kevin's comments).
>>
>> I think this patch wants a different subject line now.
>
Yes, it needs a more appropriate subject.
>It needs also a better explanation in the commit message and should not go
>through trivial as the change is not obvious.
>
OK , I will update the commit message base on existing comments.
Thanks.
>Thanks,
>Laurent
>
>>
>>> diff --git a/block/iscsi.c b/block/iscsi.c index
>>> 682abd8e09..50bae51700 100644
>>> --- a/block/iscsi.c
>>> +++ b/block/iscsi.c
>>> @@ -2002,7 +2002,7 @@ static int iscsi_open(BlockDriverState *bs, QDict
>*options, int flags,
>>> iscsilun->cluster_size = iscsilun->bl.opt_unmap_gran *
>>> iscsilun->block_size;
>>> if (iscsilun->lbprz) {
>>> -ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
>>> +ret = iscsi_allocmap_init(iscsilun, flags);
>>> }
>>> }
>>
>> The code looks good.
>>
>> Reviewed-by: Kevin Wolf
>>
>>
RE: [PATCH] hw/scsi/megasas:Clean up some redundant code fix Clang warnings
>-Original Message-
>From: Peter Maydell [mailto:peter.mayd...@linaro.org]
>Sent: Tuesday, March 10, 2020 9:47 PM
>To: Chenqun (kuhn)
>Cc: QEMU Developers ; QEMU Trivial triv...@nongnu.org>; Fam Zheng ; Hannes Reinecke
>; Zhanghailiang ;
>Qemu-block ; Euler Robot
>; Paolo Bonzini
>Subject: Re: [PATCH] hw/scsi/megasas:Clean up some redundant code fix
>Clang warnings
>
>On Tue, 10 Mar 2020 at 13:10, Chen Qun
>wrote:
>>
>> Here are some redundant statements, we can clean them up.
>> Clang static code analyzer show warning:
>> hw/scsi/megasas.c:1175:32: warning: Value stored to 'max_ld_disks' during
>its initialization is never read
>> uint32_t num_ld_disks = 0, max_ld_disks = s->fw_luns;
>>^~~~ ~~
>> hw/scsi/megasas.c:1183:9: warning: Value stored to 'max_ld_disks' is never
>read
>> max_ld_disks = 0;
>> ^ ~
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Chen Qun
>> ---
>> Cc: Paolo Bonzini
>> Cc: Fam Zheng
>> Cc: Hannes Reinecke
>> Cc: qemu-bl...@nongnu.org
>> ---
>> hw/scsi/megasas.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c index
>> af18c88b65..3f982e1d3b 100644
>> --- a/hw/scsi/megasas.c
>> +++ b/hw/scsi/megasas.c
>> @@ -1172,7 +1172,7 @@ static int
>megasas_dcmd_ld_list_query(MegasasState *s, MegasasCmd *cmd)
>> uint16_t flags;
>> struct mfi_ld_targetid_list info;
>> size_t dcmd_size = sizeof(info), resid;
>> -uint32_t num_ld_disks = 0, max_ld_disks = s->fw_luns;
>> +uint32_t num_ld_disks = 0, max_ld_disks;
>> BusChild *kid;
>>
>> /* mbox0 contains flags */
>> @@ -1180,7 +1180,6 @@ static int
>megasas_dcmd_ld_list_query(MegasasState *s, MegasasCmd *cmd)
>> trace_megasas_dcmd_ld_list_query(cmd->index, flags);
>> if (flags != MR_LD_QUERY_TYPE_ALL &&
>> flags != MR_LD_QUERY_TYPE_EXPOSED_TO_HOST) {
>> -max_ld_disks = 0;
>> }
>
>This doesn't look right -- your change removes the only statement in the body
>of this "if". I think you need to examine what the function is trying to do
>with
>the test it is doing on these flags in order to identify what the right change
>is...
>
Ah, sorry for trouble, it is not a mistake that should happen. I should double
check it next time.
>Probably this means going back to the h/w spec to identify the correct
>behaviour overall.
>
Yes, I should go back the hw spec in order to understand the behaviour overall.
Thanks.
Chen Qun
RE: [PATCH v3 04/12] scsi/esp-pci: Remove redundant statement in esp_pci_io_write()
>-Original Message- >From: Laurent Vivier [mailto:laur...@vivier.eu] >Sent: Monday, March 9, 2020 8:22 PM >To: Chenqun (kuhn) ; qemu- >de...@nongnu.org; qemu-triv...@nongnu.org >Cc: peter.mayd...@linaro.org; Euler Robot ; >Zhanghailiang ; Paolo Bonzini > >Subject: Re: [PATCH v3 04/12] scsi/esp-pci: Remove redundant statement in >esp_pci_io_write() > >Le 02/03/2020 à 14:07, Chen Qun a écrit : >> Clang static code analyzer show warning: >> hw/scsi/esp-pci.c:198:9: warning: Value stored to 'size' is never read >> size = 4; >> ^ ~ >> >> Reported-by: Euler Robot >> Signed-off-by: Chen Qun >> --- >> Cc: Paolo Bonzini Cc:Fam Zheng > >> --- >> hw/scsi/esp-pci.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/hw/scsi/esp-pci.c b/hw/scsi/esp-pci.c index >> d5a1f9e017..2e6cc07d4e 100644 >> --- a/hw/scsi/esp-pci.c >> +++ b/hw/scsi/esp-pci.c >> @@ -195,7 +195,6 @@ static void esp_pci_io_write(void *opaque, hwaddr >addr, >> val <<= shift; >> val |= current & ~(mask << shift); >> addr &= ~3; >> -size = 4; >> } > >perhaps a "g_assert(size >= 4)" instead would be cleaner to mute the warning? > Yes, add 'g_assert(size >= 4)' can mute the warning. > >I think it's a good point to update the size if in the future the code below is >modified to use size. > Hmm, maybe it is true. So, let's keep ' size = 4' and add 'g_assert(size >= 4)' after if() statement , shall we? Thanks, Chen Qun > >Thanks, >Laurent >
RE: [PATCH v2] hw/net/imx_fec: write TGSR and TCSR3 in imx_enet_write()
>-Original Message- >From: Peter Maydell [mailto:peter.mayd...@linaro.org] >Sent: Monday, March 9, 2020 7:36 PM >To: Chenqun (kuhn) >Cc: QEMU Developers ; QEMU Trivial triv...@nongnu.org>; Zhanghailiang ; >Jason Wang ; Peter Chubb >; qemu-arm ; Euler >Robot >Subject: Re: [PATCH v2] hw/net/imx_fec: write TGSR and TCSR3 in >imx_enet_write() > >On Thu, 5 Mar 2020 at 10:53, Chen Qun wrote: >> >> The current code causes clang static code analyzer generate warning: >> hw/net/imx_fec.c:858:9: warning: Value stored to 'value' is never read >> value = value & 0x000f; >> ^ ~~ >> hw/net/imx_fec.c:864:9: warning: Value stored to 'value' is never read >> value = value & 0x00fd; >> ^ ~~ >> >> According to the definition of the function, the two “value” >> assignments should be written to registers. >> >> Reported-by: Euler Robot >> Signed-off-by: Chen Qun >> --- >> v1->v2: >> The register 'ENET_TGSR' write-1-to-clear timer flag. >> The register 'ENET_TCSRn' 7bit(TF) write-1-to-clear timer flag. >> --- >> hw/net/imx_fec.c | 6 -- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c index >> 6a124a154a..322cbdcc17 100644 >> --- a/hw/net/imx_fec.c >> +++ b/hw/net/imx_fec.c >> @@ -855,13 +855,15 @@ static void imx_enet_write(IMXFECState *s, >uint32_t index, uint32_t value) >> break; >> case ENET_TGSR: >> /* implement clear timer flag */ >> -value = value & 0x000f; >> +s->regs[index] ^= s->regs[index] & value; >> +s->regs[index] &= 0x000f; >> break; >> case ENET_TCSR0: >> case ENET_TCSR1: >> case ENET_TCSR2: >> case ENET_TCSR3: >> -value = value & 0x00fd; >> +s->regs[index] = (value & 0x0080) ? (0x007d & value) : >> + (value & 0x00fd); >> break; >> case ENET_TCCR0: >> case ENET_TCCR1: > >This isn't the usual way to write W1C behaviour. >If all the relevant bits are W1C, as for TGSR: > > s->regs[index] &= ~(value & 0xf); /* all bits W1C */ > Yes, it looks better. But do we need clear the reserved bit (31 - 4 bits) explicitly ? We see that other registers have explicitly cleared the reserved bit, which also meets the I.MX datasheet requirements. s->regs[index] &= ~(value & 0xf) & 0xf;/* 0-3 bits W1C, 4-31 reserved bits write zero */ >If some but not all bits are W1C, as for TCSR*: > Yes, this patch is just only W1C for 7bit, not all bits for TCSRn. But do we need clear the reserved bit (31 - 8 bits) explicitly ? > s->regs[index] &= ~(value & 0x80); /* W1C bits */ s->regs[index] &= ~(value & 0x80) & 0xff ; /* 7 bits W1C, 8-31 reserved bits write zero */ > s->regs[index] &= ~0x7d; /* writable fields */ > s->regs[index] |= (value & 0x7d); > >thanks >-- PMM
RE: [PATCH v3 11/12] usb/hcd-ehci: Remove redundant statements
>-Original Message-
>From: Laurent Vivier [mailto:laur...@vivier.eu]
>Sent: Monday, March 9, 2020 8:43 PM
>To: Chenqun (kuhn) ; qemu-
>de...@nongnu.org; qemu-triv...@nongnu.org
>Cc: peter.mayd...@linaro.org; Zhanghailiang
>; Gerd Hoffmann ;
>Euler Robot ; Philippe Mathieu-Daudé
>
>Subject: Re: [PATCH v3 11/12] usb/hcd-ehci: Remove redundant statements
>
>Le 02/03/2020 à 14:07, Chen Qun a écrit :
>> The "again" assignment is meaningless before g_assert_not_reached.
>> In addition, the break statements no longer needs to be after
>> g_assert_not_reached.
>>
>> Clang static code analyzer show warning:
>> hw/usb/hcd-ehci.c:2108:13: warning: Value stored to 'again' is never read
>> again = -1;
>> ^ ~~
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Chen Qun
>> Reviewed-by: Philippe Mathieu-Daudé
>> ---
>> Cc: Gerd Hoffmann
>> Cc: Philippe Mathieu-Daudé
>> ---
>> hw/usb/hcd-ehci.c | 3 ---
>> 1 file changed, 3 deletions(-)
>>
>> diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index
>> 56ab2f457f..29d49c2d7e 100644
>> --- a/hw/usb/hcd-ehci.c
>> +++ b/hw/usb/hcd-ehci.c
>> @@ -1301,7 +1301,6 @@ static void ehci_execute_complete(EHCIQueue *q)
>> /* should not be triggerable */
>> fprintf(stderr, "USB invalid response %d\n", p->packet.status);
>> g_assert_not_reached();
>> -break;
>> }
>>
>> /* TODO check 4.12 for splits */
>> @@ -2105,9 +2104,7 @@ static void ehci_advance_state(EHCIState *ehci,
>> int async)
>>
>> default:
>> fprintf(stderr, "Bad state!\n");
>> -again = -1;
>> g_assert_not_reached();
>> -break;
>> }
>>
>> if (again < 0 || itd_count > 16) {
>>
>
>Applied to my trivial-patches branch.
>
Hi, Laurent
Gerd has added a patch to the USB queue today.
https://patchwork.kernel.org/patch/11405615/
>Thanks,
>Laurent
RE: [PATCH] contrib/elf2dmp: prevent uninitialized warning
>-Original Message-
>From: Viktor Prutyanov [mailto:viktor.prutya...@phystech.edu]
>Sent: Friday, March 6, 2020 7:48 PM
>To: Chenqun (kuhn)
>Cc: qemu-triv...@nongnu.org; pbonz...@redhat.com; qemu-
>de...@nongnu.org; Zhanghailiang
>Subject: Re: [PATCH] contrib/elf2dmp: prevent uninitialized warning
>
>On Fri, 6 Mar 2020 02:18:07 +
>"Chenqun (kuhn)" wrote:
>
>> >-Original Message-
>> >From: Viktor Prutyanov [mailto:viktor.prutya...@phystech.edu]
>> >Sent: Friday, March 6, 2020 2:59 AM
>> >To: Chenqun (kuhn)
>> >Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; Zhanghailiang
>> >; qemu-triv...@nongnu.org
>> >Subject: Re: [PATCH] contrib/elf2dmp: prevent uninitialized warning
>> >
>> >On Fri, 7 Feb 2020 12:16:01 +0800
>> > wrote:
>> >
>> >> From: Chen Qun
>> >>
>> >> Fix compilation warnings:
>> >> contrib/elf2dmp/main.c:66:17: warning: ‘KdpDataBlockEncoded’ may be
>> >> used uninitialized in this function [-Wmaybe-uninitialized]
>> >> block = __builtin_bswap64(block ^ kdbe) ^ kwa;
>> >> ^~~
>> >> contrib/elf2dmp/main.c:78:24: note: ‘KdpDataBlockEncoded’ was
>> >> declared here uint64_t kwn, kwa, KdpDataBlockEncoded;
>> >> ^~~
>> >>
>> >> Reported-by: Euler Robot
>> >> Signed-off-by: Chen Qun
>> >> ---
>> >> contrib/elf2dmp/main.c | 25 -
>> >> 1 file changed, 12 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c index
>> >> 9a2dbc2902..203b9e6d04 100644
>> >> --- a/contrib/elf2dmp/main.c
>> >> +++ b/contrib/elf2dmp/main.c
>> >> @@ -76,6 +76,7 @@ static KDDEBUGGER_DATA64 *get_kdbg(uint64_t
>> >> KernBase, struct pdb_reader *pdb, DBGKD_DEBUG_DATA_HEADER64
>> >kdbg_hdr;
>> >> bool decode = false;
>> >> uint64_t kwn, kwa, KdpDataBlockEncoded;
>> >> +uint64_t KiWaitNever, KiWaitAlways;
>> >>
>> >> if (va_space_rw(vs,
>> >> KdDebuggerDataBlock + offsetof(KDDEBUGGER_DATA64,
>> >> Header), @@ -84,21 +85,19 @@ static KDDEBUGGER_DATA64
>> >> *get_kdbg(uint64_t KernBase, struct pdb_reader *pdb, return NULL;
>> >> }
>> >>
>> >> -if (memcmp(_hdr.OwnerTag, OwnerTag, sizeof(OwnerTag))) {
>> >> -uint64_t KiWaitNever, KiWaitAlways;
>> >> -
>> >> -decode = true;
>> >> +if (!SYM_RESOLVE(KernBase, pdb, KiWaitNever) ||
>> >> +!SYM_RESOLVE(KernBase, pdb, KiWaitAlways) ||
>> >> +!SYM_RESOLVE(KernBase, pdb, KdpDataBlockEncoded)) {
>> >> +return NULL;
>> >> +}
>> >>
>> >> -if (!SYM_RESOLVE(KernBase, pdb, KiWaitNever) ||
>> >> -!SYM_RESOLVE(KernBase, pdb, KiWaitAlways) ||
>> >> -!SYM_RESOLVE(KernBase, pdb, KdpDataBlockEncoded))
>> >> {
>> >> -return NULL;
>> >> -}
>> >> +if (va_space_rw(vs, KiWaitNever, , sizeof(kwn), 0) ||
>> >> +va_space_rw(vs, KiWaitAlways, , sizeof(kwa), 0)) {
>> >> +return NULL;
>> >> +}
>> >>
>> >> -if (va_space_rw(vs, KiWaitNever, , sizeof(kwn), 0) ||
>> >> -va_space_rw(vs, KiWaitAlways, , sizeof(kwa),
>> >> 0)) {
>> >> -return NULL;
>> >> -}
>> >> +if (memcmp(_hdr.OwnerTag, OwnerTag, sizeof(OwnerTag))) {
>> >> +decode = true;
>> >>
>> >> printf("[KiWaitNever] = 0x%016"PRIx64"\n", kwn);
>> >> printf("[KiWaitAlways] = 0x%016"PRIx64"\n", kwa);
>> >
>> >Hi!
>> >
>> >I suppose the problem is in your compiler, because kdbg_decode() is
>> >only used when KdpDataBlockEncoded is already initialized by
>> >SYM_RESOLVE().
>> >
>> Hi Viktor,
>>
>>I know it's actually initialized when 'decode = true;',
>> otherwise ' return kdbg;' no need to initialize.
>> But usually the compiler cannot understand it, because it seems
>> that the initialization is only in the if() statement.
>
>As for me, my GCC 9.2.1 doesn't show any warning while building elf2dmp.
>
Maybe you are right, my GCC version lower( 7.3.0).
>
>> If we put the initialization outside the if() statement, it might
>> looks better without affecting the functionality ?
>
>For now, your original patch affects the functionality. The utility tries to
>resolve symbols as little as possible during conversion, because we don't
>know exactly how Windows kernel works. This is the reason why KDBG
>header should be checked before resolving 3 symbols.
>
OK , let's drop this patch.
Thanks.
>>
>> Thanks.
>> >--
>> >Viktor Prutyanov
>
>
>
>--
>Viktor Prutyanov
RE: [PATCH] contrib/elf2dmp: prevent uninitialized warning
>-Original Message-
>From: Viktor Prutyanov [mailto:viktor.prutya...@phystech.edu]
>Sent: Friday, March 6, 2020 2:59 AM
>To: Chenqun (kuhn)
>Cc: qemu-devel@nongnu.org; pbonz...@redhat.com; Zhanghailiang
>; qemu-triv...@nongnu.org
>Subject: Re: [PATCH] contrib/elf2dmp: prevent uninitialized warning
>
>On Fri, 7 Feb 2020 12:16:01 +0800
> wrote:
>
>> From: Chen Qun
>>
>> Fix compilation warnings:
>> contrib/elf2dmp/main.c:66:17: warning: ‘KdpDataBlockEncoded’ may be
>> used uninitialized in this function [-Wmaybe-uninitialized]
>> block = __builtin_bswap64(block ^ kdbe) ^ kwa;
>> ^~~
>> contrib/elf2dmp/main.c:78:24: note: ‘KdpDataBlockEncoded’ was declared
>> here uint64_t kwn, kwa, KdpDataBlockEncoded;
>> ^~~
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Chen Qun
>> ---
>> contrib/elf2dmp/main.c | 25 -
>> 1 file changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/contrib/elf2dmp/main.c b/contrib/elf2dmp/main.c index
>> 9a2dbc2902..203b9e6d04 100644
>> --- a/contrib/elf2dmp/main.c
>> +++ b/contrib/elf2dmp/main.c
>> @@ -76,6 +76,7 @@ static KDDEBUGGER_DATA64 *get_kdbg(uint64_t
>> KernBase, struct pdb_reader *pdb, DBGKD_DEBUG_DATA_HEADER64
>kdbg_hdr;
>> bool decode = false;
>> uint64_t kwn, kwa, KdpDataBlockEncoded;
>> +uint64_t KiWaitNever, KiWaitAlways;
>>
>> if (va_space_rw(vs,
>> KdDebuggerDataBlock + offsetof(KDDEBUGGER_DATA64,
>> Header), @@ -84,21 +85,19 @@ static KDDEBUGGER_DATA64
>> *get_kdbg(uint64_t KernBase, struct pdb_reader *pdb, return NULL;
>> }
>>
>> -if (memcmp(_hdr.OwnerTag, OwnerTag, sizeof(OwnerTag))) {
>> -uint64_t KiWaitNever, KiWaitAlways;
>> -
>> -decode = true;
>> +if (!SYM_RESOLVE(KernBase, pdb, KiWaitNever) ||
>> +!SYM_RESOLVE(KernBase, pdb, KiWaitAlways) ||
>> +!SYM_RESOLVE(KernBase, pdb, KdpDataBlockEncoded)) {
>> +return NULL;
>> +}
>>
>> -if (!SYM_RESOLVE(KernBase, pdb, KiWaitNever) ||
>> -!SYM_RESOLVE(KernBase, pdb, KiWaitAlways) ||
>> -!SYM_RESOLVE(KernBase, pdb, KdpDataBlockEncoded)) {
>> -return NULL;
>> -}
>> +if (va_space_rw(vs, KiWaitNever, , sizeof(kwn), 0) ||
>> +va_space_rw(vs, KiWaitAlways, , sizeof(kwa), 0)) {
>> +return NULL;
>> +}
>>
>> -if (va_space_rw(vs, KiWaitNever, , sizeof(kwn), 0) ||
>> -va_space_rw(vs, KiWaitAlways, , sizeof(kwa), 0))
>> {
>> -return NULL;
>> -}
>> +if (memcmp(_hdr.OwnerTag, OwnerTag, sizeof(OwnerTag))) {
>> +decode = true;
>>
>> printf("[KiWaitNever] = 0x%016"PRIx64"\n", kwn);
>> printf("[KiWaitAlways] = 0x%016"PRIx64"\n", kwa);
>
>Hi!
>
>I suppose the problem is in your compiler, because kdbg_decode() is only
>used when KdpDataBlockEncoded is already initialized by SYM_RESOLVE().
>
Hi Viktor,
I know it's actually initialized when 'decode = true;', otherwise '
return kdbg;' no need to initialize.
But usually the compiler cannot understand it, because it seems that the
initialization is only in the if() statement.
If we put the initialization outside the if() statement, it might looks better
without affecting the functionality ?
Thanks.
>--
>Viktor Prutyanov
RE: [PATCH v2] hw/net/imx_fec: write TGSR and TCSR3 in imx_enet_write()
>-Original Message- >From: Chenqun (kuhn) >Sent: Thursday, March 5, 2020 6:53 PM >To: qemu-devel@nongnu.org; qemu-triv...@nongnu.org >Cc: peter.mayd...@linaro.org; Zhanghailiang >; jasow...@redhat.com; >peter.ch...@nicta.com.au; qemu-...@nongnu.org; Chenqun (kuhn) >; Euler Robot >Subject: [PATCH v2] hw/net/imx_fec: write TGSR and TCSR3 in >imx_enet_write() > >The current code causes clang static code analyzer generate warning: >hw/net/imx_fec.c:858:9: warning: Value stored to 'value' is never read >value = value & 0x000f; >^ ~~ >hw/net/imx_fec.c:864:9: warning: Value stored to 'value' is never read >value = value & 0x00fd; >^ ~~ > >According to the definition of the function, the two “value” assignments >should be written to registers. > >Reported-by: Euler Robot >Signed-off-by: Chen Qun >--- >v1->v2: > The register 'ENET_TGSR' write-1-to-clear timer flag. > The register 'ENET_TCSRn' 7bit(TF) write-1-to-clear timer flag. >--- > hw/net/imx_fec.c | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) > >diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c index >6a124a154a..322cbdcc17 100644 >--- a/hw/net/imx_fec.c >+++ b/hw/net/imx_fec.c >@@ -855,13 +855,15 @@ static void imx_enet_write(IMXFECState *s, >uint32_t index, uint32_t value) > break; > case ENET_TGSR: > /* implement clear timer flag */ >-value = value & 0x000f; >+s->regs[index] ^= s->regs[index] & value; >+s->regs[index] &= 0x000f; > In "i.MX 6Dual/6Quad Applications Processor Reference Manual" documentation, the register 'ENET_TGSR' all fields TFn is write-1-to-clear. It is described in detail as follows： --- Field Description --- 31–4 This field is reserved. --- 3 Copy Of Timer Flag For Channel 3 TF3 0 Timer Flag for Channel 3 is clear 1 Timer Flag for Channel 3 is set --- 2 Copy Of Timer Flag For Channel 2 TF2 0 Timer Flag for Channel 2 is clear 1 Timer Flag for Channel 2 is set --- 1 Copy Of Timer Flag For Channel 1 TF1 0 Timer Flag for Channel 1 is clear 1 Timer Flag for Channel 1 is set --- 0 Copy Of Timer Flag For Channel 0 TF0 0 Timer Flag for Channel 0 is clear 1 Timer Flag for Channel 0 is set -- > break; > case ENET_TCSR0: > case ENET_TCSR1: > case ENET_TCSR2: > case ENET_TCSR3: >-value = value & 0x00fd; >+s->regs[index] = (value & 0x0080) ? (0x007d & value) : >+ (value & 0x00fd); > The ENET_TCSRn field descriptions: --- Field Description --- 31–8 This field is reserved. -- 7 Timer Flag TFSets when input capture or output compare occurs. This flag is double buffered between the module clock and 1588 clock domains. When this field is 1, it can be cleared to 0 by writing 1to it. 0 Input Capture or Output Compare has not occurred 1 Input Capture or Output Compare has occurred 6 Timer Interrupt Enable TIE 0 Interrupt is disabled 1 Interrupt is enabled 2-5 Timer Mode .. 1 This field is reserved. 0Timer DMA Request Enable TDRE0 DMA request is disabled 1 DMA request is enabled > break; > case ENET_TCCR0: > case ENET_TCCR1: >-- >2.23.0 >
RE: [PATCH v2 02/13] block/iscsi:Remove redundant statement in iscsi_open()
>-Original Message- >From: Kevin Wolf [mailto:kw...@redhat.com] >Sent: Friday, February 28, 2020 6:55 PM >To: Chenqun (kuhn) >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org; >peter.mayd...@linaro.org; Zhanghailiang ; >Euler Robot ; Ronnie Sahlberg >; Paolo Bonzini ; Peter >Lieven ; Max Reitz >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement in >iscsi_open() > >Am 28.02.2020 um 08:30 hat Chenqun (kuhn) geschrieben: >> >-Original Message- >> >From: Kevin Wolf [mailto:kw...@redhat.com] >> >Sent: Thursday, February 27, 2020 6:31 PM >> >To: Chenqun (kuhn) >> >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org; >> >peter.mayd...@linaro.org; Zhanghailiang >> >; Euler Robot >> >; Ronnie Sahlberg >; >> >Paolo Bonzini ; Peter Lieven ; Max >> >Reitz >> >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement >> >in >> >iscsi_open() >> > >> >Am 27.02.2020 um 02:49 hat Chenqun (kuhn) geschrieben: >> >> >-Original Message- >> >> >From: Kevin Wolf [mailto:kw...@redhat.com] >> >> >Sent: Wednesday, February 26, 2020 5:55 PM >> >> >To: Chenqun (kuhn) >> >> >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org; >> >> >peter.mayd...@linaro.org; Zhanghailiang >> >> >; Euler Robot >> >> >; Ronnie Sahlberg >> >; >> >> >Paolo Bonzini ; Peter Lieven ; >> >> >Max Reitz >> >> >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant >> >> >statement in >> >> >iscsi_open() >> >> > >> >> >Am 26.02.2020 um 09:46 hat kuhn.chen...@huawei.com geschrieben: >> >> >> From: Chen Qun >> >> >> >> >> >> Clang static code analyzer show warning: >> >> >> block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read >> >> >> flags &= ~BDRV_O_RDWR; >> >> >> ^ >> >> >> >> >> >> Reported-by: Euler Robot >> >> >> Signed-off-by: Chen Qun >> >> > >> >> >Hmm, I'm not so sure about this one because if we remove the line, >> >> >flags will be inconsistent with bs->open_flags. It feels like >> >> >setting a trap for anyone who wants to add code using flags in the >future. >> >> Hi Kevin, >> >> I find it exists since 8f3bf50d34037266. : ) >> > >> >Yes, it has existed from the start with auto-read-only. >> > >> >> It's not a big deal, just upset clang static code analyzer. >> >> As you said, it could be a trap for the future. >> > >> >What's interesting is that we do have one user of the flags later in >> >the function, but it uses bs->open_flags instead: >> > >> >ret = iscsi_allocmap_init(iscsilun, bs->open_flags); >> > >> >Maybe this should be using flags? (The value of the bits we're >> >interested in is the same, but when flags is passed as a parameter, I >> >would expect it to be >> >used.) >> > >> Hi Kevin, >> I have a question: are 'flags' exactly the same as 'bs-> open_flags'? >> In the function bdrv_open_common() at block.c file, the existence of >statement( open_flags = bdrv_open_flags(bs, bs->open_flags); ) makes them >a little different. >> Will this place affect them inconsistently ? > >Not exactly the same, that's why I said "value of the bits we're interested in >is >the same". bdrv_open_flags() basically just filters out things that are handled >by the generic block layer and none of the block driver's business. > >To be precise, iscsi_allocmap_init() only checks BDRV_O_NOCACHE, which is >the same in both. > >> Is it safer if we assign bs-> open_flags to flags? > >This would add back the flags that we consciously filtered out before, so I >would say no. > Well, I see, thank you very much for your detailed explanation! Thanks.
RE: [PATCH v2 02/13] block/iscsi:Remove redundant statement in iscsi_open()
>-Original Message-
>From: Kevin Wolf [mailto:kw...@redhat.com]
>Sent: Thursday, February 27, 2020 6:31 PM
>To: Chenqun (kuhn)
>Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org;
>peter.mayd...@linaro.org; Zhanghailiang ;
>Euler Robot ; Ronnie Sahlberg
>; Paolo Bonzini ; Peter
>Lieven ; Max Reitz
>Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement in
>iscsi_open()
>
>Am 27.02.2020 um 02:49 hat Chenqun (kuhn) geschrieben:
>> >-Original Message-
>> >From: Kevin Wolf [mailto:kw...@redhat.com]
>> >Sent: Wednesday, February 26, 2020 5:55 PM
>> >To: Chenqun (kuhn)
>> >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org;
>> >peter.mayd...@linaro.org; Zhanghailiang
>> >; Euler Robot
>> >; Ronnie Sahlberg
>;
>> >Paolo Bonzini ; Peter Lieven ; Max
>> >Reitz
>> >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement
>> >in
>> >iscsi_open()
>> >
>> >Am 26.02.2020 um 09:46 hat kuhn.chen...@huawei.com geschrieben:
>> >> From: Chen Qun
>> >>
>> >> Clang static code analyzer show warning:
>> >> block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read
>> >> flags &= ~BDRV_O_RDWR;
>> >> ^
>> >>
>> >> Reported-by: Euler Robot
>> >> Signed-off-by: Chen Qun
>> >
>> >Hmm, I'm not so sure about this one because if we remove the line,
>> >flags will be inconsistent with bs->open_flags. It feels like setting
>> >a trap for anyone who wants to add code using flags in the future.
>> Hi Kevin,
>> I find it exists since 8f3bf50d34037266. : )
>
>Yes, it has existed from the start with auto-read-only.
>
>> It's not a big deal, just upset clang static code analyzer.
>> As you said, it could be a trap for the future.
>
>What's interesting is that we do have one user of the flags later in the
>function,
>but it uses bs->open_flags instead:
>
>ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
>
>Maybe this should be using flags? (The value of the bits we're interested in is
>the same, but when flags is passed as a parameter, I would expect it to be
>used.)
>
Hi Kevin,
I have a question: are 'flags' exactly the same as 'bs-> open_flags'?
In the function bdrv_open_common() at block.c file, the existence of
statement( open_flags = bdrv_open_flags(bs, bs->open_flags); ) makes them a
little different.
Will this place affect them inconsistently ?
Is it safer if we assign bs-> open_flags to flags?
Modify just like：
@@ -1917,7 +1917,7 @@ static int iscsi_open(BlockDriverState *bs, QDict
*options, int flags,
if (ret < 0) {
goto out;
}
-flags &= ~BDRV_O_RDWR;
+flags = bs->open_flags;
}
iscsi_readcapacity_sync(iscsilun, _err);
@@ -2002,7 +2002,7 @@ static int iscsi_open(BlockDriverState *bs, QDict
*options, int flags,
iscsilun->cluster_size = iscsilun->bl.opt_unmap_gran *
iscsilun->block_size;
if (iscsilun->lbprz) {
-ret = iscsi_allocmap_init(iscsilun, bs->open_flags);
+ret = iscsi_allocmap_init(iscsilun, flags);
}
}
Thanks.
RE: [PATCH v2 02/13] block/iscsi:Remove redundant statement in iscsi_open()
>-Original Message- >From: Kevin Wolf [mailto:kw...@redhat.com] >Sent: Thursday, February 27, 2020 6:31 PM >To: Chenqun (kuhn) >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org; >peter.mayd...@linaro.org; Zhanghailiang ; >Euler Robot ; Ronnie Sahlberg >; Paolo Bonzini ; Peter >Lieven ; Max Reitz >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement in >iscsi_open() > >Am 27.02.2020 um 02:49 hat Chenqun (kuhn) geschrieben: >> >-Original Message- >> >From: Kevin Wolf [mailto:kw...@redhat.com] >> >Sent: Wednesday, February 26, 2020 5:55 PM >> >To: Chenqun (kuhn) >> >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org; >> >peter.mayd...@linaro.org; Zhanghailiang >> >; Euler Robot >> >; Ronnie Sahlberg >; >> >Paolo Bonzini ; Peter Lieven ; Max >> >Reitz >> >Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement >> >in >> >iscsi_open() >> > >> >Am 26.02.2020 um 09:46 hat kuhn.chen...@huawei.com geschrieben: >> >> From: Chen Qun >> >> >> >> Clang static code analyzer show warning: >> >> block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read >> >> flags &= ~BDRV_O_RDWR; >> >> ^ >> >> >> >> Reported-by: Euler Robot >> >> Signed-off-by: Chen Qun >> > >> >Hmm, I'm not so sure about this one because if we remove the line, >> >flags will be inconsistent with bs->open_flags. It feels like setting >> >a trap for anyone who wants to add code using flags in the future. >> Hi Kevin, >> I find it exists since 8f3bf50d34037266. : ) > >Yes, it has existed from the start with auto-read-only. > >> It's not a big deal, just upset clang static code analyzer. >> As you said, it could be a trap for the future. > >What's interesting is that we do have one user of the flags later in the >function, >but it uses bs->open_flags instead: > >ret = iscsi_allocmap_init(iscsilun, bs->open_flags); > Good point, I think this is exactly where the 'flags' are needed now. It should be right to keep it for the future, too. Later version, I will modify it. Thanks. > >Maybe this should be using flags? (The value of the bits we're interested in is >the same, but when flags is passed as a parameter, I would expect it to be >used.)
RE: [PATCH v3 12/19] tests/plugin: prevent uninitialized warning
>-Original Message-
>From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com]
>Sent: Wednesday, February 26, 2020 10:00 PM
>To: Alex Bennée ; qemu-devel@nongnu.org
>Cc: f...@euphon.net; Thomas Huth ;
>berra...@redhat.com; robert.fo...@linaro.org; pbonz...@redhat.com;
>stef...@linux.vnet.ibm.com; Euler Robot ;
>richard.hender...@linaro.org; f4...@amsat.org; robhe...@microsoft.com;
>marcandre.lur...@redhat.com; aa...@os.amperecomputing.com;
>c...@braap.org; stefa...@redhat.com; Chenqun (kuhn)
>; peter.pu...@linaro.org;
>aurel...@aurel32.net
>Subject: Re: [PATCH v3 12/19] tests/plugin: prevent uninitialized warning
>
>On 2/25/20 1:47 PM, Alex Bennée wrote:
>> From: Chen Qun
>>
>> According to the glibc function requirements, we need initialise
>
>GLib?
Yes, Glib function requirements.
https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html#g-autofree
Thanks.
>
>> the variable. Otherwise there will be compilation warnings:
>>
>> glib-autocleanups.h:28:3: warning: ‘out’ may be used uninitialized in
>> this function [-Wmaybe-uninitialized]
>> g_free (*pp);
>> ^~~~
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Chen Qun
>> Reviewed-by: Thomas Huth
>> Message-Id: <20200206093238.203984-1-kuhn.chen...@huawei.com>
>> [AJB: uses Thomas's single line allocation]
>> Signed-off-by: Alex Bennée
>> Reviewed-by: Richard Henderson
>> ---
>> tests/plugin/bb.c | 6 +++---
>> tests/plugin/insn.c | 3 +--
>> 2 files changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c index
>> f30bea08dcc..df19fd359df 100644
>> --- a/tests/plugin/bb.c
>> +++ b/tests/plugin/bb.c
>> @@ -22,9 +22,9 @@ static bool do_inline;
>>
>> static void plugin_exit(qemu_plugin_id_t id, void *p)
>> {
>> -g_autofree gchar *out;
>> -out = g_strdup_printf("bb's: %" PRIu64", insns: %" PRIu64 "\n",
>> - bb_count, insn_count);
>> +g_autofree gchar *out = g_strdup_printf(
>> +"bb's: %" PRIu64", insns: %" PRIu64 "\n",
>> +bb_count, insn_count);
>> qemu_plugin_outs(out);
>> }
>>
>> diff --git a/tests/plugin/insn.c b/tests/plugin/insn.c index
>> 0a8f5ae..a9a6e412373 100644
>> --- a/tests/plugin/insn.c
>> +++ b/tests/plugin/insn.c
>> @@ -44,8 +44,7 @@ static void vcpu_tb_trans(qemu_plugin_id_t id,
>> struct qemu_plugin_tb *tb)
>>
>> static void plugin_exit(qemu_plugin_id_t id, void *p)
>> {
>> -g_autofree gchar *out;
>> -out = g_strdup_printf("insns: %" PRIu64 "\n", insn_count);
>> +g_autofree gchar *out = g_strdup_printf("insns: %" PRIu64 "\n",
>> + insn_count);
>> qemu_plugin_outs(out);
>> }
>>
>>
RE: [PATCH v2 02/13] block/iscsi:Remove redundant statement in iscsi_open()
>-Original Message-
>From: Kevin Wolf [mailto:kw...@redhat.com]
>Sent: Wednesday, February 26, 2020 5:55 PM
>To: Chenqun (kuhn)
>Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org;
>peter.mayd...@linaro.org; Zhanghailiang ;
>Euler Robot ; Ronnie Sahlberg
>; Paolo Bonzini ; Peter
>Lieven ; Max Reitz
>Subject: Re: [PATCH v2 02/13] block/iscsi:Remove redundant statement in
>iscsi_open()
>
>Am 26.02.2020 um 09:46 hat kuhn.chen...@huawei.com geschrieben:
>> From: Chen Qun
>>
>> Clang static code analyzer show warning:
>> block/iscsi.c:1920:9: warning: Value stored to 'flags' is never read
>> flags &= ~BDRV_O_RDWR;
>> ^
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Chen Qun
>
>Hmm, I'm not so sure about this one because if we remove the line, flags will
>be inconsistent with bs->open_flags. It feels like setting a trap for anyone
>who wants to add code using flags in the future.
Hi Kevin,
I find it exists since 8f3bf50d34037266. : )
It's not a big deal, just upset clang static code analyzer.
As you said, it could be a trap for the future.
It ’s okay, whether it exists or not.
Thanks.
>
>Kevin
>
>> Cc: Ronnie Sahlberg
>> Cc: Paolo Bonzini
>> Cc: Peter Lieven
>> Cc: Kevin Wolf
>> Cc: Max Reitz
>> ---
>> block/iscsi.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/block/iscsi.c b/block/iscsi.c index
>> 682abd8e09..ed88479ede 100644
>> --- a/block/iscsi.c
>> +++ b/block/iscsi.c
>> @@ -1917,7 +1917,6 @@ static int iscsi_open(BlockDriverState *bs, QDict
>*options, int flags,
>> if (ret < 0) {
>> goto out;
>> }
>> -flags &= ~BDRV_O_RDWR;
>> }
>>
>> iscsi_readcapacity_sync(iscsilun, _err);
>> --
>> 2.23.0
>>
>>
RE: [PATCH v2 01/13] block/stream: Remove redundant statement in stream_run()
>-Original Message-
>From: Kevin Wolf [mailto:kw...@redhat.com]
>Sent: Wednesday, February 26, 2020 6:36 PM
>To: Chenqun (kuhn)
>Cc: John Snow ; qemu-devel@nongnu.org; qemu-
>triv...@nongnu.org; peter.mayd...@linaro.org; Zhanghailiang
>; Euler Robot
>; Max Reitz
>Subject: Re: [PATCH v2 01/13] block/stream: Remove redundant statement in
>stream_run()
>
>Am 26.02.2020 um 11:21 hat Chenqun (kuhn) geschrieben:
>>
>>
>> >-Original Message-
>> >From: Kevin Wolf [mailto:kw...@redhat.com]
>> >Sent: Wednesday, February 26, 2020 5:51 PM
>> >To: Chenqun (kuhn)
>> >Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org;
>> >peter.mayd...@linaro.org; Zhanghailiang
>> >; Euler Robot
>> >; John Snow ; Max Reitz
>> >
>> >Subject: Re: [PATCH v2 01/13] block/stream: Remove redundant
>> >statement in
>> >stream_run()
>> >
>> >Am 26.02.2020 um 09:46 hat kuhn.chen...@huawei.com geschrieben:
>> >> From: Chen Qun
>> >>
>> >> Clang static code analyzer show warning:
>> >> block/stream.c:186:9: warning: Value stored to 'ret' is never read
>> >> ret = 0;
>> >> ^ ~
>> >> Reported-by: Euler Robot
>> >> Signed-off-by: Chen Qun
>> >> Reviewed-by: John Snow
>> >
>> >Let's mention that this is unnecessary since commit 1d809098aa9.
>> >
>> >Since the same commit, the initialisation 'int ret = 0;' is
>> >unnecessary because we never read ret before overwriting the initial
>> >value. We could clean this up in the same patch.
>>
>> Yes, we can clean it and move 'ret' declaration to the for() statement.
>>
>> Modify just Like this：
>> [...]
>
>Godd point, makes sense to me. Please keep my R-b if you make this change.
OK, no problem！
Thanks.
>
>Kevin
>
>> @@ -114,7 +114,6 @@ static int coroutine_fn stream_run(Job *job, Error
>**errp)
>> int64_t offset = 0;
>> uint64_t delay_ns = 0;
>> int error = 0;
>> -int ret = 0;
>> int64_t n = 0; /* bytes */
>>
>> if (bs == s->bottom) {
>> @@ -139,6 +138,7 @@ static int coroutine_fn stream_run(Job *job, Error
>> **errp)
>>
>> for ( ; offset < len; offset += n) {
>> bool copy;
>> +int ret;
>>
>> /* Note that even when no rate limit is applied we need to yield
>> * with no pending I/O here so that bdrv_drain_all() returns.
>> @@ -183,7 +183,6 @@ static int coroutine_fn stream_run(Job *job, Error
>**errp)
>> break;
>> }
>> }
>> -ret = 0;
>>
>> /* Publish progress */
>> job_progress_update(>common.job, n);
>>
>> >
>> >With or without the changes:
>> >
>> >Reviewed-by: Kevin Wolf
>>
RE: [PATCH v2 01/13] block/stream: Remove redundant statement in stream_run()
>-Original Message-
>From: Kevin Wolf [mailto:kw...@redhat.com]
>Sent: Wednesday, February 26, 2020 5:51 PM
>To: Chenqun (kuhn)
>Cc: qemu-devel@nongnu.org; qemu-triv...@nongnu.org;
>peter.mayd...@linaro.org; Zhanghailiang ;
>Euler Robot ; John Snow ;
>Max Reitz
>Subject: Re: [PATCH v2 01/13] block/stream: Remove redundant statement in
>stream_run()
>
>Am 26.02.2020 um 09:46 hat kuhn.chen...@huawei.com geschrieben:
>> From: Chen Qun
>>
>> Clang static code analyzer show warning:
>> block/stream.c:186:9: warning: Value stored to 'ret' is never read
>> ret = 0;
>> ^ ~
>> Reported-by: Euler Robot
>> Signed-off-by: Chen Qun
>> Reviewed-by: John Snow
>
>Let's mention that this is unnecessary since commit 1d809098aa9.
>
>Since the same commit, the initialisation 'int ret = 0;' is unnecessary because
>we never read ret before overwriting the initial value. We could clean this up
>in the same patch.
Yes, we can clean it and move 'ret' declaration to the for() statement.
Modify just Like this：
@@ -114,7 +114,6 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
int64_t offset = 0;
uint64_t delay_ns = 0;
int error = 0;
-int ret = 0;
int64_t n = 0; /* bytes */
if (bs == s->bottom) {
@@ -139,6 +138,7 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
for ( ; offset < len; offset += n) {
bool copy;
+int ret;
/* Note that even when no rate limit is applied we need to yield
* with no pending I/O here so that bdrv_drain_all() returns.
@@ -183,7 +183,6 @@ static int coroutine_fn stream_run(Job *job, Error **errp)
break;
}
}
-ret = 0;
/* Publish progress */
job_progress_update(>common.job, n);
>
>With or without the changes:
>
>Reviewed-by: Kevin Wolf
RE: [PATCH] hw/net/imx_fec: write TGSR and TCSR3 in imx_enet_write()
>-Original Message- >From: Jason Wang [mailto:jasow...@redhat.com] >Sent: Wednesday, February 26, 2020 11:03 AM >To: Peter Maydell >Cc: Chenqun (kuhn) ; QEMU Developers >; QEMU Trivial ; >Zhanghailiang ; Peter Chubb >; qemu-arm >Subject: Re: [PATCH] hw/net/imx_fec: write TGSR and TCSR3 in >imx_enet_write() > > >On 2020/2/25 下午6:18, Peter Maydell wrote: >> On Tue, 25 Feb 2020 at 05:41, Jason Wang wrote: >>> >>> On 2020/2/25 上午10:59, Chen Qun wrote: >>>> The current code causes clang static code analyzer generate warning: >>>> hw/net/imx_fec.c:858:9: warning: Value stored to 'value' is never read >>>> value = value & 0x000f; >>>> ^ ~~ >>>> hw/net/imx_fec.c:864:9: warning: Value stored to 'value' is never read >>>> value = value & 0x00fd; >>>> ^ ~~ >>>> >>>> According to the definition of the function, the two “value” assignments >>>>should be written to registers. >>>> >>>> Reported-by: Euler Robot >>>> Signed-off-by: Chen Qun >>>> --- >>>> I'm not sure if this modification is correct, just from the function >>>>definition, it is correct. >>>> --- >>>>hw/net/imx_fec.c | 4 ++-- >>>>1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/net/imx_fec.c b/hw/net/imx_fec.c index >>>> 6a124a154a..92f6215712 100644 >>>> --- a/hw/net/imx_fec.c >>>> +++ b/hw/net/imx_fec.c >>>> @@ -855,13 +855,13 @@ static void imx_enet_write(IMXFECState *s, >uint32_t index, uint32_t value) >>>>break; >>>>case ENET_TGSR: >>>>/* implement clear timer flag */ >>>> -value = value & 0x000f; >>>> +s->regs[index] = value & 0x000f; >>>>break; >> Hi; the datasheet for this SoC says that these bits of the register >> are write-1-to-clear, so while this is definitely a bug I don't think >> this is the right fix. >> >>>>case ENET_TCSR0: >>>>case ENET_TCSR1: >>>>case ENET_TCSR2: >>>>case ENET_TCSR3: >>>> -value = value & 0x00fd; >>>> +s->regs[index] = value & 0x00fd; >>>>break; >> Here bit 7 is write-1-to-clear, though bits 0 and >> 2..5 are simple write-the-value. >> >>>>case ENET_TCCR0: >>>>case ENET_TCCR1: >>> >>> Applied. >> Could you drop this from your queue, please? >> >> thanks >> -- PMM > > >Sure, Chen please send V2 to address Peter's comment. OK, but I didn't find the datasheet that contains these two registers description. Could someone provide me with a connection for the datasheet ?
RE: [PATCH 09/13] dma/xlnx-zdma: Remove redundant statement in zdma_write_dst()
>-Original Message-
>From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com]
>Sent: Tuesday, February 25, 2020 6:12 PM
>To: Chenqun (kuhn) ; qemu-
>de...@nongnu.org; qemu-triv...@nongnu.org
>Cc: peter.mayd...@linaro.org; Zhanghailiang
>; Alistair Francis ;
>qemu-...@nongnu.org
>Subject: Re: [PATCH 09/13] dma/xlnx-zdma: Remove redundant statement in
>zdma_write_dst()
>
>On 2/25/20 11:01 AM, Chenqun (kuhn) wrote:
>>> -Original Message-
>>> From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com]
>>> Sent: Tuesday, February 25, 2020 5:36 PM
>>> To: Chenqun (kuhn) ; qemu-
>de...@nongnu.org;
>>> qemu-triv...@nongnu.org
>>> Cc: peter.mayd...@linaro.org; Zhanghailiang
>>> ; Alistair Francis
>>> ; qemu-...@nongnu.org
>>> Subject: Re: [PATCH 09/13] dma/xlnx-zdma: Remove redundant statement
>>> in
>>> zdma_write_dst()
>>>
>>> On 2/25/20 3:09 AM, kuhn.chen...@huawei.com wrote:
>>>> From: Chen Qun
>>>>
>>>> Clang static code analyzer show warning:
>>>> hw/dma/xlnx-zdma.c:399:13: warning: Value stored to 'dst_type' is
>>>> never
>>> read
>>>> dst_type = FIELD_EX32(s->dsc_dst.words[3],
>>> ZDMA_CH_DST_DSCR_WORD3,
>>>> ^
>>> ~~~
>>>>
>>>> Reported-by: Euler Robot
>>>> Signed-off-by: Chen Qun
>>>> ---
>>>> Cc: Alistair Francis
>>>> Cc: "Edgar E. Iglesias"
>>>> Cc: Peter Maydell
>>>> Cc: qemu-...@nongnu.org
>>>> ---
>>>>hw/dma/xlnx-zdma.c | 2 --
>>>>1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c index
>>>> 8fb83f5b07..45355c5d59 100644
>>>> --- a/hw/dma/xlnx-zdma.c
>>>> +++ b/hw/dma/xlnx-zdma.c
>>>> @@ -396,8 +396,6 @@ static void zdma_write_dst(XlnxZDMA *s, uint8_t
>>> *buf, uint32_t len)
>>>>zdma_load_descriptor(s, next, >dsc_dst);
>>>>dst_size = FIELD_EX32(s->dsc_dst.words[2],
>>> ZDMA_CH_DST_DSCR_WORD2,
>>>> SIZE);
>>>> -dst_type = FIELD_EX32(s->dsc_dst.words[3],
>>> ZDMA_CH_DST_DSCR_WORD3,
>>>> - TYPE);
>>>
>>> Maybe move dst_type to this if() statement now?
>>>
>> Sorry, I don't follow you. I didn't find where I could move dst_type.
>> Do you mean to move the first dst_type to the if().
>> Modify it like this:
>> while (len) {
>> dst_size = FIELD_EX32(s->dsc_dst.words[2],
>ZDMA_CH_DST_DSCR_WORD2,
>>SIZE);
>> if (dst_size == 0 && ptype == PT_MEM) {
>> uint64_t next;
>> dst_type = FIELD_EX32(s->dsc_dst.words[3],
>ZDMA_CH_DST_DSCR_WORD3,
>>TYPE);
>> next = zdma_update_descr_addr(s, dst_type,
>>R_ZDMA_CH_DST_CUR_DSCR_LSB);
>> zdma_load_descriptor(s, next, >dsc_dst);
>> dst_size = FIELD_EX32(s->dsc_dst.words[2],
>ZDMA_CH_DST_DSCR_WORD2,
>>SIZE);
>> }
>> ...
>> }
>
>No, like this:
>
>-- >8 --
>@@ -373,7 +373,7 @@ static uint64_t zdma_update_descr_addr(XlnxZDMA
>*s, bool type,
> static void zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
> {
> uint32_t dst_size, dlen;
>-bool dst_intr, dst_type;
>+bool dst_intr;
> unsigned int ptype = ARRAY_FIELD_EX32(s->regs, ZDMA_CH_CTRL0,
>POINT_TYPE);
> unsigned int rw_mode = ARRAY_FIELD_EX32(s->regs, ZDMA_CH_CTRL0,
>MODE);
> unsigned int burst_type = ARRAY_FIELD_EX32(s->regs,
>ZDMA_CH_DATA_ATTR, @@ -387,17 +387,17 @@ static void
>zdma_write_dst(XlnxZDMA *s, uint8_t *buf, uint32_t len)
> while (len) {
> dst_size = FIELD_EX32(s->dsc_dst.words[2],
>ZDMA_CH_DST_DSCR_WORD2,
>SIZE);
>-dst_type = FIELD_EX32(s->dsc_dst.words[3],
>ZDMA_CH_DST_DSCR_WORD3,
>- TYPE);
> if (dst_size == 0 && ptype == PT_MEM) {
> uint64_t next;
>+bool dst_type;
>+
>+dst_type = FIELD_EX32(s->dsc_dst.words[3],
>ZDMA_CH_DST_DSCR_WORD3,
>+ TYPE);
> next = zdma_update_descr_addr(s, dst_type,
>R_ZDMA_CH_DST_CUR_DSCR_LSB);
> zdma_load_descriptor(s, next, >dsc_dst);
> dst_size = FIELD_EX32(s->dsc_dst.words[2],
>ZDMA_CH_DST_DSCR_WORD2,
>SIZE);
>-dst_type = FIELD_EX32(s->dsc_dst.words[3],
>ZDMA_CH_DST_DSCR_WORD3,
>- TYPE);
> }
Hmm, this is better.
I will modify it later in V2.
Thanks.
>---
RE: [PATCH 13/13] monitor/hmp-cmds: Remove redundant statement in hmp_rocker_of_dpa_groups()
>-Original Message-
>From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com]
>Sent: Tuesday, February 25, 2020 5:45 PM
>To: Chenqun (kuhn) ; qemu-
>de...@nongnu.org; qemu-triv...@nongnu.org
>Cc: peter.mayd...@linaro.org; Zhanghailiang
>; Dr. David Alan Gilbert
>
>Subject: Re: [PATCH 13/13] monitor/hmp-cmds: Remove redundant
>statement in hmp_rocker_of_dpa_groups()
>
>On 2/25/20 3:09 AM, kuhn.chen...@huawei.com wrote:
>> From: Chen Qun
>>
>> Clang static code analyzer show warning:
>> monitor/hmp-cmds.c:2867:17: warning: Value stored to 'set' is never read
>> set = true;
>> ^
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Chen Qun
>> ---
>> Cc: "Dr. David Alan Gilbert"
>> ---
>> monitor/hmp-cmds.c | 1 -
>> 1 file changed, 1 deletion(-)
>>
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c index
>> 53bc3f76c4..84f94647cd 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -2864,7 +2864,6 @@ void hmp_rocker_of_dpa_groups(Monitor *mon,
>> const QDict *qdict)
>>
>> if (group->has_set_eth_dst) {
>> if (!set) {
>> -set = true;
>> monitor_printf(mon, " set");
>> }
>> monitor_printf(mon, " dst %s", group->set_eth_dst);
>>
>
>Can you move the 'set' declaration to the for() statement and also remove the
>last "set = false;"?
Yes, you are right! It will be better!I will modify it later in V2.
Thanks.
RE: [PATCH 09/13] dma/xlnx-zdma: Remove redundant statement in zdma_write_dst()
>-Original Message-
>From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com]
>Sent: Tuesday, February 25, 2020 5:36 PM
>To: Chenqun (kuhn) ; qemu-
>de...@nongnu.org; qemu-triv...@nongnu.org
>Cc: peter.mayd...@linaro.org; Zhanghailiang
>; Alistair Francis ;
>qemu-...@nongnu.org
>Subject: Re: [PATCH 09/13] dma/xlnx-zdma: Remove redundant statement in
>zdma_write_dst()
>
>On 2/25/20 3:09 AM, kuhn.chen...@huawei.com wrote:
>> From: Chen Qun
>>
>> Clang static code analyzer show warning:
>> hw/dma/xlnx-zdma.c:399:13: warning: Value stored to 'dst_type' is never
>read
>> dst_type = FIELD_EX32(s->dsc_dst.words[3],
>ZDMA_CH_DST_DSCR_WORD3,
>> ^
>~~~
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Chen Qun
>> ---
>> Cc: Alistair Francis
>> Cc: "Edgar E. Iglesias"
>> Cc: Peter Maydell
>> Cc: qemu-...@nongnu.org
>> ---
>> hw/dma/xlnx-zdma.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/hw/dma/xlnx-zdma.c b/hw/dma/xlnx-zdma.c index
>> 8fb83f5b07..45355c5d59 100644
>> --- a/hw/dma/xlnx-zdma.c
>> +++ b/hw/dma/xlnx-zdma.c
>> @@ -396,8 +396,6 @@ static void zdma_write_dst(XlnxZDMA *s, uint8_t
>*buf, uint32_t len)
>> zdma_load_descriptor(s, next, >dsc_dst);
>> dst_size = FIELD_EX32(s->dsc_dst.words[2],
>ZDMA_CH_DST_DSCR_WORD2,
>> SIZE);
>> -dst_type = FIELD_EX32(s->dsc_dst.words[3],
>ZDMA_CH_DST_DSCR_WORD3,
>> - TYPE);
>
>Maybe move dst_type to this if() statement now?
>
Sorry, I don't follow you. I didn't find where I could move dst_type.
Do you mean to move the first dst_type to the if().
Modify it like this:
while (len) {
dst_size = FIELD_EX32(s->dsc_dst.words[2], ZDMA_CH_DST_DSCR_WORD2,
SIZE);
if (dst_size == 0 && ptype == PT_MEM) {
uint64_t next;
dst_type = FIELD_EX32(s->dsc_dst.words[3], ZDMA_CH_DST_DSCR_WORD3,
TYPE);
next = zdma_update_descr_addr(s, dst_type,
R_ZDMA_CH_DST_CUR_DSCR_LSB);
zdma_load_descriptor(s, next, >dsc_dst);
dst_size = FIELD_EX32(s->dsc_dst.words[2], ZDMA_CH_DST_DSCR_WORD2,
SIZE);
}
...
}
Thanks.
>> }
>>
>> /* Match what hardware does by ignoring the dst_size and
>> only using
>>
RE: [PATCH 0/3]hw: Fixs memleak of fdevice tree blob
>-Original Message- >From: Chenqun (kuhn) >Sent: Tuesday, February 18, 2020 5:12 PM >To: qemu-devel@nongnu.org; qemu-...@nongnu.org; jcmvb...@gmail.com; >crwu...@gmail.com; ma...@denx.de; edgar.igles...@gmail.com; >da...@gibson.dropbear.id.au >Cc: Zhanghailiang ; qemu- >triv...@nongnu.org; pbonz...@redhat.com; Pannengyuan >; Chenqun (kuhn) > >Subject: [PATCH 0/3]hw: Fixs memleak of fdevice tree blob > >From: Chen Qun > Hi all, after reviewing various patches from Pan Nengyuan fix ppc-e500 fdt memleaks, I search for related content and found Paolo Bonzini's similarly patchs. (sorry, this description is missing here because of this patch format.) >The device tree blob returned by load_device_tree is malloced. >We should free it after cpu_physical_memory_write().Otherwise, >if we repeatedly call 'system_reset',it will repeatedly load fdt, so there are >many memleaks. > >Paolo Bonzini : >https://lists.gnu.org/archive/html/qemu-devel/2019-10/msg00129.html > >Pan Nengyuan: >https://lists.gnu.org/archive/html/qemu-devel/2020-02/msg03594.html > >After searching the device code, I found three similar issues. >This series fixes the last three. > >Chen Qun (3): > hw/nios2:fix leak of fdevice tree blob > hw/ppc/virtex_ml507:fix leak of fdevice tree blob > hw/xtensa/xtfpga:fix leak of fdevice tree blob > > hw/nios2/boot.c | 1 + > hw/ppc/virtex_ml507.c | 1 + > hw/xtensa/xtfpga.c| 1 + > 3 files changed, 3 insertions(+) > >-- >2.23.0 >
RE: [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init
>-Original Message-
>From: Eduardo Habkost [mailto:ehabk...@redhat.com]
>Sent: Thursday, February 13, 2020 12:20 AM
>To: Philippe Mathieu-Daudé
>Cc: Chenqun (kuhn) ; qemu-
>de...@nongnu.org; i.mitsya...@gmail.com; peter.mayd...@linaro.org;
>qemu-triv...@nongnu.org; Zhanghailiang
>; Markus Armbruster
>
>Subject: Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in
>exynos4210_uart_init
>
>On Wed, Feb 12, 2020 at 08:39:55AM +0100, Philippe Mathieu-Daudé wrote:
>> Cc'ing Eduardo & Markus.
>>
>> On 2/12/20 7:44 AM, Chenqun (kuhn) wrote:
>> > > -Original Message-
>> > > From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com]
>> > > Sent: Wednesday, February 12, 2020 2:16 PM
>> > > To: Chenqun (kuhn) ; qemu-
>> > > de...@nongnu.org; i.mitsya...@gmail.com; peter.mayd...@linaro.org
>> > > Cc: qemu-triv...@nongnu.org; Zhanghailiang
>> > >
>> > > Subject: Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in
>> > > exynos4210_uart_init
>> > >
>> > > On 2/12/20 4:36 AM, kuhn.chen...@huawei.com wrote:
>> > > > From: Chen Qun
>> > > >
>> > > > It's easy to reproduce as follow:
>> > > > virsh qemu-monitor-command vm1 --pretty '{"execute":
>> > > > "device-list-properties",
>"arguments":{"typename":"exynos4210.uart"}}'
>> > > >
>> > > > ASAN shows memory leak stack:
>> > > > #1 0xfffd896d71cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
>> > > > #2 0xaaad270beee3 in timer_new_full
>/qemu/include/qemu/timer.h:530
>> > > > #3 0xaaad270beee3 in timer_new /qemu/include/qemu/timer.h:551
>> > > > #4 0xaaad270beee3 in timer_new_ns
>/qemu/include/qemu/timer.h:569
>> > > > #5 0xaaad270beee3 in exynos4210_uart_init
>> > > /qemu/hw/char/exynos4210_uart.c:677
>> > > > #6 0xaaad275c8f4f in object_initialize_with_type
>/qemu/qom/object.c:516
>> > > > #7 0xaaad275c91bb in object_new_with_type
>/qemu/qom/object.c:684
>> > > > #8 0xaaad2755df2f in qmp_device_list_properties
>> > > > /qemu/qom/qom-qmp-cmds.c:152
>> > > >
>> > > > Reported-by: Euler Robot
>> > > > Signed-off-by: Chen Qun
>> > > > ---
>> > > >hw/char/exynos4210_uart.c | 8
>> > > >1 file changed, 4 insertions(+), 4 deletions(-)
>> > > >
>> > > > diff --git a/hw/char/exynos4210_uart.c
>> > > > b/hw/char/exynos4210_uart.c index 25d6588e41..5048db5410 100644
>> > > > --- a/hw/char/exynos4210_uart.c
>> > > > +++ b/hw/char/exynos4210_uart.c
>> > > > @@ -674,10 +674,6 @@ static void exynos4210_uart_init(Object *obj)
>> > > >SysBusDevice *dev = SYS_BUS_DEVICE(obj);
>> > > >Exynos4210UartState *s = EXYNOS4210_UART(dev);
>> > > >
>> > > > -s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> > > > - exynos4210_uart_timeout_int,
>> > > > s);
>> > > > -s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
>> > >
>> > > Why are you moving s->wordtime from init() to realize()?
>> > >
>> > Hi Philippe, thanks for your reply!
>> >
>> > Because I found the variable wordtime is usually used with
>fifo_timeout_timer.
>> > Eg, they are used together in the exynos4210_uart_rx_timeout_set
>function.
>> >
>> > I didn't find anything wrong with wordtime in the realize().
>> > Does it have any other effects?
>>
>> IIUC when we use both init() and realize(), realize() should only
>> contains on code that consumes the object properties... But maybe the
>> design is not clear. Then why not move all the init() code to realize()?
>
>Normally I would recommend the opposite: delay as much as possible to
>realize(), to avoid unwanted side effects when (e.g.) running qom-list-
>properties.
>
>But as s->wordtime is a simple struct field (that we could even decide to
>expose to the outside as a read-only QOM property), it doesn't really matter.
>Personally, I would keep it where it is just to avoid churn.
>
OK, Let's keep s->wordtime in init().
I will change it in next version.
Thanks.
>--
>Eduardo
RE: [PATCH] hw/char/exynos4210_uart: Fix memleaks in exynos4210_uart_init
>-Original Message-
>From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com]
>Sent: Wednesday, February 12, 2020 2:16 PM
>To: Chenqun (kuhn) ; qemu-
>de...@nongnu.org; i.mitsya...@gmail.com; peter.mayd...@linaro.org
>Cc: qemu-triv...@nongnu.org; Zhanghailiang
>
>Subject: Re: [PATCH] hw/char/exynos4210_uart: Fix memleaks in
>exynos4210_uart_init
>
>On 2/12/20 4:36 AM, kuhn.chen...@huawei.com wrote:
>> From: Chen Qun
>>
>> It's easy to reproduce as follow:
>> virsh qemu-monitor-command vm1 --pretty '{"execute":
>> "device-list-properties", "arguments":{"typename":"exynos4210.uart"}}'
>>
>> ASAN shows memory leak stack:
>>#1 0xfffd896d71cb in g_malloc0 (/lib64/libglib-2.0.so.0+0x571cb)
>>#2 0xaaad270beee3 in timer_new_full /qemu/include/qemu/timer.h:530
>>#3 0xaaad270beee3 in timer_new /qemu/include/qemu/timer.h:551
>>#4 0xaaad270beee3 in timer_new_ns /qemu/include/qemu/timer.h:569
>>#5 0xaaad270beee3 in exynos4210_uart_init
>/qemu/hw/char/exynos4210_uart.c:677
>>#6 0xaaad275c8f4f in object_initialize_with_type /qemu/qom/object.c:516
>>#7 0xaaad275c91bb in object_new_with_type /qemu/qom/object.c:684
>>#8 0xaaad2755df2f in qmp_device_list_properties
>> /qemu/qom/qom-qmp-cmds.c:152
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Chen Qun
>> ---
>> hw/char/exynos4210_uart.c | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
>> index 25d6588e41..5048db5410 100644
>> --- a/hw/char/exynos4210_uart.c
>> +++ b/hw/char/exynos4210_uart.c
>> @@ -674,10 +674,6 @@ static void exynos4210_uart_init(Object *obj)
>> SysBusDevice *dev = SYS_BUS_DEVICE(obj);
>> Exynos4210UartState *s = EXYNOS4210_UART(dev);
>>
>> -s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> - exynos4210_uart_timeout_int, s);
>> -s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
>
>Why are you moving s->wordtime from init() to realize()?
>
Hi Philippe, thanks for your reply!
Because I found the variable wordtime is usually used with fifo_timeout_timer.
Eg, they are used together in the exynos4210_uart_rx_timeout_set function.
I didn't find anything wrong with wordtime in the realize().
Does it have any other effects?
Thanks.
>> -
>> /* memory mapping */
>> memory_region_init_io(>iomem, obj, _uart_ops, s,
>> "exynos4210.uart",
>> EXYNOS4210_UART_REGS_MEM_SIZE); @@ -691,6 +687,10 @@ static void
>exynos4210_uart_realize(DeviceState *dev, Error **errp)
>> {
>> Exynos4210UartState *s = EXYNOS4210_UART(dev);
>>
>> +s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>> + exynos4210_uart_timeout_int, s);
>> +s->wordtime = NANOSECONDS_PER_SECOND * 10 / 9600;
>> +
>> qemu_chr_fe_set_handlers(>chr, exynos4210_uart_can_receive,
>>exynos4210_uart_receive,
>> exynos4210_uart_event,
>>NULL, s, NULL, true);
>>
RE: [PATCH] tests/plugin: prevent uninitialized warning
>-Original Message-
>From: Alex Bennée [mailto:alex.ben...@linaro.org]
>Sent: Thursday, February 6, 2020 8:46 PM
>To: Chenqun (kuhn)
>Cc: qemu-devel@nongnu.org; Zhanghailiang
>; qemu-triv...@nongnu.org;
>richard.hender...@linaro.org
>Subject: Re: [PATCH] tests/plugin: prevent uninitialized warning
>
>
>kuhn.chen...@huawei.com writes:
>
>> From: Chen Qun
>>
>> According to the glibc function requirements, we need initialise the
>> variable. Otherwise there will be compilation warnings:
>>
>> glib-autocleanups.h:28:3: warning: ‘out’ may be used uninitialized in
>> this function [-Wmaybe-uninitialized]
>>g_free (*pp);
>>^~~~
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Chen Qun
>
>Queued to plugins/next with Thomas' single line suggestion, thanks.
Thank you!
By the way, what is plugins/next connection address?
>
>> ---
>> tests/plugin/bb.c | 2 +-
>> tests/plugin/insn.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/plugin/bb.c b/tests/plugin/bb.c index
>> f30bea08dc..8b9da23a04 100644
>> --- a/tests/plugin/bb.c
>> +++ b/tests/plugin/bb.c
>> @@ -22,7 +22,7 @@ static bool do_inline;
>>
>> static void plugin_exit(qemu_plugin_id_t id, void *p) {
>> -g_autofree gchar *out;
>> +g_autofree gchar *out = NULL;
>> out = g_strdup_printf("bb's: %" PRIu64", insns: %" PRIu64 "\n",
>>bb_count, insn_count);
>> qemu_plugin_outs(out);
>> diff --git a/tests/plugin/insn.c b/tests/plugin/insn.c index
>> 0a8f5a..c83b1c0157 100644
>> --- a/tests/plugin/insn.c
>> +++ b/tests/plugin/insn.c
>> @@ -44,7 +44,7 @@ static void vcpu_tb_trans(qemu_plugin_id_t id,
>> struct qemu_plugin_tb *tb)
>>
>> static void plugin_exit(qemu_plugin_id_t id, void *p) {
>> -g_autofree gchar *out;
>> +g_autofree gchar *out = NULL;
>> out = g_strdup_printf("insns: %" PRIu64 "\n", insn_count);
>> qemu_plugin_outs(out);
>> }
>
>
>--
>Alex Bennée
RE: [PATCH] xhci: Fix memory leak in xhci_kick_epctx when poweroff GuestOS
>-Original Message-
>From: Gerd Hoffmann [mailto:kra...@redhat.com]
>Sent: Monday, January 13, 2020 3:48 PM
>To: Philippe Mathieu-Daudé
>Cc: Chenqun (kuhn) ; qemu-
>de...@nongnu.org; qemu-triv...@nongnu.org; Pannengyuan
>; Zhanghailiang
>
>Subject: Re: [PATCH] xhci: Fix memory leak in xhci_kick_epctx when poweroff
>GuestOS
>
>> > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index
>> > 80988bb305..0d3d96d05a 100644
>> > --- a/hw/usb/hcd-xhci.c
>> > +++ b/hw/usb/hcd-xhci.c
>> > @@ -2000,6 +2000,7 @@ static void xhci_kick_epctx(XHCIEPContext
>*epctx, unsigned int streamid)
>> > if (xfer != NULL && xfer->running_retry) {
>> > DPRINTF("xhci: xfer nacked, stopping schedule\n");
>> > epctx->retry = xfer;
>> > +xhci_xfer_unmap(xfer);
>>
>> Shouldn't we use xhci_ep_free_xfer() instead?
>
>No, xhci will try to run the transfer again later.
>
>xhci will re-create the sgl then, so freeing the sgl here is correct. Patch
>added
>to usb queue.
Hi Gerd,
I test every keyboard input, it will leak once.
I tested qemu-4.0.0 also had this leak .
Maybe we should cc to qemu-stable ?
Thanks.
>
>thanks,
> Gerd
RE: [PATCH] xhci: Fix memory leak in xhci_kick_epctx when poweroff GuestOS
>-Original Message-
>From: Philippe Mathieu-Daudé [mailto:phi...@redhat.com]
>Sent: Friday, January 10, 2020 9:14 PM
>To: Chenqun (kuhn) ; qemu-
>de...@nongnu.org; kra...@redhat.com
>Cc: qemu-triv...@nongnu.org; Pannengyuan ;
>Zhanghailiang
>Subject: Re: [PATCH] xhci: Fix memory leak in xhci_kick_epctx when poweroff
>GuestOS
>
>On 1/10/20 11:58 AM, kuhn.chen...@huawei.com wrote:
>> From: Chen Qun
>>
>> start vm with libvirt, when GuestOS running, enter poweroff command
>> using the xhci keyboard, then ASAN shows memory leak stack：
>>
>> Direct leak of 80 byte(s) in 5 object(s) allocated from:
>> #0 0xfffd1e6431cb in __interceptor_malloc (/lib64/libasan.so.4+0xd31cb)
>> #1 0xfffd1e107163 in g_malloc (/lib64/libglib-2.0.so.0+0x57163)
>> #2 0xaaad39051367 in qemu_sglist_init /qemu/dma-helpers.c:43
>> #3 0xaaad3947c407 in pci_dma_sglist_init
>/qemu/include/hw/pci/pci.h:842
>> #4 0xaaad3947c407 in xhci_xfer_create_sgl /qemu/hw/usb/hcd-
>xhci.c:1446
>> #5 0xaaad3947c407 in xhci_setup_packet /qemu/hw/usb/hcd-xhci.c:1618
>> #6 0xaaad3948625f in xhci_submit /qemu/hw/usb/hcd-xhci.c:1827
>> #7 0xaaad3948625f in xhci_fire_transfer /qemu/hw/usb/hcd-xhci.c:1839
>> #8 0xaaad3948625f in xhci_kick_epctx /qemu/hw/usb/hcd-xhci.c:1991
>> #9 0xaaad3948f537 in xhci_doorbell_write /qemu/hw/usb/hcd-
>xhci.c:3158
>> #10 0xaaad38bcbfc7 in memory_region_write_accessor
>/qemu/memory.c:483
>> #11 0xaaad38bc654f in access_with_adjusted_size /qemu/memory.c:544
>> #12 0xaaad38bd1877 in memory_region_dispatch_write
>/qemu/memory.c:1482
>> #13 0xaaad38b1c77f in flatview_write_continue /qemu/exec.c:3167
>> #14 0xaaad38b1ca83 in flatview_write /qemu/exec.c:3207
>> #15 0xaaad38b268db in address_space_write /qemu/exec.c:3297
>> #16 0xaaad38bf909b in kvm_cpu_exec /qemu/accel/kvm/kvm-all.c:2383
>> #17 0xaaad38bb063f in qemu_kvm_cpu_thread_fn /qemu/cpus.c:1246
>> #18 0xaaad39821c93 in qemu_thread_start /qemu/util/qemu-thread-
>posix.c:519
>> #19 0xfffd1c8378bb (/lib64/libpthread.so.0+0x78bb)
>> #20 0xfffd1c77616b (/lib64/libc.so.6+0xd616b)
>>
>> Reported-by: Euler Robot
>> Signed-off-by: Chen Qun
>> ---
>> hw/usb/hcd-xhci.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c index
>> 80988bb305..0d3d96d05a 100644
>> --- a/hw/usb/hcd-xhci.c
>> +++ b/hw/usb/hcd-xhci.c
>> @@ -2000,6 +2000,7 @@ static void xhci_kick_epctx(XHCIEPContext *epctx,
>unsigned int streamid)
>> if (xfer != NULL && xfer->running_retry) {
>> DPRINTF("xhci: xfer nacked, stopping schedule\n");
>> epctx->retry = xfer;
>> +xhci_xfer_unmap(xfer);
>
>Shouldn't we use xhci_ep_free_xfer() instead?
>Also, it would be cleaner if you set it to NULL.
>
Hi Philippe,
Thanks for your reply!
But, It is just a sglist leak. xhci_xfer_unmap() can free and set it to
NULL.
The xhci_ep_free_xfer() did't free a sglist.
By the way, xfer should be use for epctx->retry, we can't free it.
Thanks.
>> break;
>> }
>> if (count++ > TRANSFER_LIMIT) {
>>
