[RFC,v3,0/7] qemu_thread_create: propagate errors to callers to check

Message

Fei Li Sept. 19, 2018, 1:35 p.m. UTC

Hi,

This idea comes from BiteSizedTasks, and this patch series implement
the error checking of qemu_thread_create: make qemu_thread_create
return a flag to indicate if it succeeded rather than failing with an
error; make all callers check it.

The first three patches apply to those call traces whose further
callers already have the *errp to pass, thus add a new Error paramater
in the call trace to propagate the error and let the further caller
check it. The last patch modifies the qemu_thread_create() and makes
it return a bool to all direct callers to indicate if it succeeds.
The middle three fix some segmentation fault when using GDB to debug.

###### Here I have a concern about using multifd migration. ######
It is about "[PATCH RFC v3 5/7] migration: fix the multifd code".
In our current code, when multifd is used during migration, if there
is an error before the destination receives all new channels (I mean
multifd_recv_new_channel(ioc)), the destination does not exit but
keeps waiting (Hang in recvmsg() in qio_channel_socket_readv) until
the source exits. I know hang is not right, but so sorry that I did
not come up with a good idea on handling this. Not so familiar with
that part of code.. Could someone kindly give a hand? Thanks a lot!


v3:
- Add two migration related patches to fix the segmentaion fault
- Extract the segmentation fault fix from v2's last patch to be a 
  separate patch
- Add cleaning up code for touched functions
- Update some error messages

v2:
- Pass errp straightly instead of using a local_err & error_propagate
- Return a bool: false/true to indicate if one function succeeds
- Merge v1's last two patches into one to avoid the compile error
- Fix one omitted error in patch1 and update some error messages

Fei Li (7):
  Fix segmentation fault when qemu_signal_init fails
  ui/vnc.c: polish vnc_init_func
  qemu_init_vcpu: add a new Error parameter to propagate
  migration: fix the compression code
  migration: fix the multifd code
  qemu_thread_join: fix segmentation fault
  qemu_thread_create: propagate the error to callers to handle

 accel/tcg/user-exec-stub.c      |  2 +-
 cpus.c                          | 79 ++++++++++++++++++++++++++---------------
 dump.c                          |  6 ++--
 hw/misc/edu.c                   |  6 ++--
 hw/ppc/spapr_hcall.c            |  9 +++--
 hw/rdma/rdma_backend.c          |  3 +-
 hw/usb/ccid-card-emulated.c     | 13 ++++---
 include/qemu/osdep.h            |  2 +-
 include/qemu/thread.h           |  4 +--
 include/qom/cpu.h               |  2 +-
 include/ui/console.h            |  2 +-
 io/task.c                       |  3 +-
 iothread.c                      | 16 ++++++---
 migration/migration.c           | 49 +++++++++++++++++--------
 migration/postcopy-ram.c        | 14 ++++++--
 migration/ram.c                 | 69 ++++++++++++++++++++++++++---------
 migration/savevm.c              | 11 ++++--
 target/alpha/cpu.c              |  4 ++-
 target/arm/cpu.c                |  4 ++-
 target/cris/cpu.c               |  4 ++-
 target/hppa/cpu.c               |  4 ++-
 target/i386/cpu.c               |  4 ++-
 target/lm32/cpu.c               |  4 ++-
 target/m68k/cpu.c               |  4 ++-
 target/microblaze/cpu.c         |  4 ++-
 target/mips/cpu.c               |  4 ++-
 target/moxie/cpu.c              |  4 ++-
 target/nios2/cpu.c              |  4 ++-
 target/openrisc/cpu.c           |  4 ++-
 target/ppc/translate_init.inc.c |  4 ++-
 target/riscv/cpu.c              |  4 ++-
 target/s390x/cpu.c              |  4 ++-
 target/sh4/cpu.c                |  4 ++-
 target/sparc/cpu.c              |  4 ++-
 target/tilegx/cpu.c             |  4 ++-
 target/tricore/cpu.c            |  4 ++-
 target/unicore32/cpu.c          |  4 ++-
 target/xtensa/cpu.c             |  4 ++-
 tests/atomic_add-bench.c        |  3 +-
 tests/iothread.c                |  2 +-
 tests/qht-bench.c               |  3 +-
 tests/rcutorture.c              |  3 +-
 tests/test-aio.c                |  2 +-
 tests/test-rcu-list.c           |  3 +-
 ui/vnc-jobs.c                   | 17 ++++++---
 ui/vnc-jobs.h                   |  2 +-
 ui/vnc.c                        | 12 +++++--
 util/compatfd.c                 | 18 +++++++---
 util/main-loop.c                | 11 +++---
 util/oslib-posix.c              | 17 ++++++---
 util/qemu-thread-posix.c        | 21 +++++++----
 util/qemu-thread-win32.c        | 15 +++++---
 util/rcu.c                      |  3 +-
 util/thread-pool.c              |  4 ++-
 54 files changed, 359 insertions(+), 151 deletions(-)

Comments

Fam Zheng Sept. 19, 2018, 3:52 p.m. UTC | #1

On Wed, 09/19 21:35, Fei Li wrote:
> Hi,
> 
> This idea comes from BiteSizedTasks, and this patch series implement
> the error checking of qemu_thread_create: make qemu_thread_create
> return a flag to indicate if it succeeded rather than failing with an
> error; make all callers check it.

Looks good in general! Though I've skipped the migration fixes hoping Peter can
chime in.

Fam

Peter Xu Sept. 20, 2018, 3:35 a.m. UTC | #2

On Wed, Sep 19, 2018 at 09:35:16PM +0800, Fei Li wrote:
> Hi,
> 
> This idea comes from BiteSizedTasks, and this patch series implement
> the error checking of qemu_thread_create: make qemu_thread_create
> return a flag to indicate if it succeeded rather than failing with an
> error; make all callers check it.
> 
> The first three patches apply to those call traces whose further
> callers already have the *errp to pass, thus add a new Error paramater
> in the call trace to propagate the error and let the further caller
> check it. The last patch modifies the qemu_thread_create() and makes
> it return a bool to all direct callers to indicate if it succeeds.
> The middle three fix some segmentation fault when using GDB to debug.
> 
> ###### Here I have a concern about using multifd migration. ######
> It is about "[PATCH RFC v3 5/7] migration: fix the multifd code".
> In our current code, when multifd is used during migration, if there
> is an error before the destination receives all new channels (I mean
> multifd_recv_new_channel(ioc)), the destination does not exit but
> keeps waiting (Hang in recvmsg() in qio_channel_socket_readv) until
> the source exits. I know hang is not right, but so sorry that I did
> not come up with a good idea on handling this. Not so familiar with
> that part of code.. Could someone kindly give a hand? Thanks a lot!

CCing Juan for the multifd concerns.

Regards,