* [Qemu-devel] [PATCH] fix: avoid infinite loop when blockjob encountering failure @ 2017-06-14 6:22 sochin.jiang 2017-06-14 6:33 ` no-reply 2017-06-14 13:12 ` Max Reitz 0 siblings, 2 replies; 8+ messages in thread From: sochin.jiang @ 2017-06-14 6:22 UTC (permalink / raw) To: kwolf, mreitz Cc: qemu-block, qemu-devel, sochin.jiang, eric.fangyi, subo7, xieyingtai, lina.lulina, zhangshuai13, lizhengui From: "sochin.jiang" <sochin.jiang@huawei.com> img_commit could fall into infinite loop if it's blockjob fail encountering any I/O error. Try to fix it. Signed-off-by: sochin.jiang <sochin.jiang@huawei.com> --- qemu-img.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/qemu-img.c b/qemu-img.c index 0ad698d..6ba565d 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -895,8 +895,11 @@ static void run_block_job(BlockJob *job, Error **errp) aio_poll(aio_context, true); qemu_progress_print(job->len ? ((float)job->offset / job->len * 100.f) : 0.0f, 0); - } while (!job->ready); + } while (!job->ready && !job->ret); + if (job->ret) { + return; + } block_job_complete_sync(job, errp); aio_context_release(aio_context); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] fix: avoid infinite loop when blockjob encountering failure 2017-06-14 6:22 [Qemu-devel] [PATCH] fix: avoid infinite loop when blockjob encountering failure sochin.jiang @ 2017-06-14 6:33 ` no-reply 2017-06-14 6:46 ` Fam Zheng 2017-06-14 13:12 ` Max Reitz 1 sibling, 1 reply; 8+ messages in thread From: no-reply @ 2017-06-14 6:33 UTC (permalink / raw) To: sochin.jiang Cc: famz, kwolf, mreitz, xieyingtai, lina.lulina, lizhengui, qemu-block, subo7, eric.fangyi, zhangshuai13, qemu-devel Hi, This series failed automatic build test. Please find the testing commands and their output below. If you have docker installed, you can probably reproduce it locally. Subject: [Qemu-devel] [PATCH] fix: avoid infinite loop when blockjob encountering failure Type: series Message-id: 1497421359-32660-1-git-send-email-sochin.jiang@huawei.com === TEST SCRIPT BEGIN === #!/bin/bash set -e git submodule update --init dtc # Let docker tests dump environment info export SHOW_ENV=1 export J=8 time make docker-test-quick@centos6 time make docker-test-mingw@fedora time make docker-test-build@min-glib === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/1497421359-32660-1-git-send-email-sochin.jiang@huawei.com -> patchew/1497421359-32660-1-git-send-email-sochin.jiang@huawei.com Switched to a new branch 'test' ce8c9c4 fix: avoid infinite loop when blockjob encountering failure === OUTPUT BEGIN === Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-uxcyyu1e/src/dtc'... Submodule path 'dtc': checked out '558cd81bdd432769b59bff01240c44f82cfb1a9d' BUILD centos6 make[1]: Entering directory '/var/tmp/patchew-tester-tmp-uxcyyu1e/src' ARCHIVE qemu.tgz ARCHIVE dtc.tgz COPY RUNNER RUN test-quick in qemu:centos6 Packages installed: SDL-devel-1.2.14-7.el6_7.1.x86_64 ccache-3.1.6-2.el6.x86_64 epel-release-6-8.noarch gcc-4.4.7-17.el6.x86_64 git-1.7.1-4.el6_7.1.x86_64 glib2-devel-2.28.8-5.el6.x86_64 libfdt-devel-1.4.0-1.el6.x86_64 make-3.81-23.el6.x86_64 package g++ is not installed pixman-devel-0.32.8-1.el6.x86_64 tar-1.23-15.el6_8.x86_64 zlib-devel-1.2.3-29.el6.x86_64 Environment variables: PACKAGES=libfdt-devel ccache tar git make gcc g++ zlib-devel glib2-devel SDL-devel pixman-devel epel-release HOSTNAME=2eef1afcc7f8 TERM=xterm MAKEFLAGS= -j8 HISTSIZE=1000 J=8 USER=root CCACHE_DIR=/var/tmp/ccache EXTRA_CONFIGURE_OPTS= V= SHOW_ENV=1 MAIL=/var/spool/mail/root PATH=/usr/lib/ccache:/usr/lib64/ccache:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin PWD=/ LANG=en_US.UTF-8 TARGET_LIST= HISTCONTROL=ignoredups SHLVL=1 HOME=/root TEST_DIR=/tmp/qemu-test LOGNAME=root LESSOPEN=||/usr/bin/lesspipe.sh %s FEATURES= dtc DEBUG= G_BROKEN_FILENAMES=1 CCACHE_HASHDIR= _=/usr/bin/env Configure options: --enable-werror --target-list=x86_64-softmmu,aarch64-softmmu --prefix=/var/tmp/qemu-build/install /tmp/qemu-test/src/configure: line 4683: c++: command not found No C++ compiler available; disabling C++ specific optional code Install prefix /var/tmp/qemu-build/install BIOS directory /var/tmp/qemu-build/install/share/qemu binary directory /var/tmp/qemu-build/install/bin library directory /var/tmp/qemu-build/install/lib module directory /var/tmp/qemu-build/install/lib/qemu libexec directory /var/tmp/qemu-build/install/libexec include directory /var/tmp/qemu-build/install/include config directory /var/tmp/qemu-build/install/etc local state directory /var/tmp/qemu-build/install/var Manual directory /var/tmp/qemu-build/install/share/man ELF interp prefix /usr/gnemul/qemu-%M Source path /tmp/qemu-test/src C compiler cc Host C compiler cc C++ compiler Objective-C compiler cc ARFLAGS rv CFLAGS -O2 -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -g QEMU_CFLAGS -I/usr/include/pixman-1 -I$(SRC_PATH)/dtc/libfdt -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -fPIE -DPIE -m64 -mcx16 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing -fno-common -fwrapv -Wendif-labels -Wno-missing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-declaration -Wold-style-definition -Wtype-limits -fstack-protector-all LDFLAGS -Wl,--warn-common -Wl,-z,relro -Wl,-z,now -pie -m64 -g make make install install python python -B smbd /usr/sbin/smbd module support no host CPU x86_64 host big endian no target list x86_64-softmmu aarch64-softmmu tcg debug enabled no gprof enabled no sparse enabled no strip binaries yes profiler no static build no pixman system SDL support yes (1.2.14) GTK support no GTK GL support no VTE support no TLS priority NORMAL GNUTLS support no GNUTLS rnd no libgcrypt no libgcrypt kdf no nettle no nettle kdf no libtasn1 no curses support no virgl support no curl support no mingw32 support no Audio drivers oss Block whitelist (rw) Block whitelist (ro) VirtFS support no VNC support yes VNC SASL support no VNC JPEG support no VNC PNG support no xen support no brlapi support no bluez support no Documentation no PIE yes vde support no netmap support no Linux AIO support no ATTR/XATTR support yes Install blobs yes KVM support yes HAX support no RDMA support no TCG interpreter no fdt support yes preadv support yes fdatasync yes madvise yes posix_madvise yes libcap-ng support no vhost-net support yes vhost-scsi support yes vhost-vsock support yes Trace backends log spice support no rbd support no xfsctl support no smartcard support no libusb no usb net redir no OpenGL support no OpenGL dmabufs no libiscsi support no libnfs support no build guest agent yes QGA VSS support no QGA w32 disk info no QGA MSI support no seccomp support no coroutine backend ucontext coroutine pool yes debug stack usage no GlusterFS support no gcov gcov gcov enabled no TPM support yes libssh2 support no TPM passthrough yes QOM debugging yes Live block migration yes lzo support no snappy support no bzip2 support no NUMA host support no tcmalloc support no jemalloc support no avx2 optimization no replication support yes VxHS block device no GEN config-host.h GEN x86_64-softmmu/config-devices.mak.tmp mkdir -p dtc/libfdt GEN aarch64-softmmu/config-devices.mak.tmp mkdir -p dtc/tests GEN qemu-options.def GEN qapi-types.h GEN qmp-commands.h GEN qapi-visit.h GEN qapi-event.h GEN x86_64-softmmu/config-devices.mak GEN qmp-marshal.c GEN aarch64-softmmu/config-devices.mak GEN qapi-types.c GEN qapi-visit.c GEN qapi-event.c GEN qmp-introspect.h GEN qmp-introspect.c GEN trace/generated-tcg-tracers.h GEN trace/generated-helpers-wrappers.h GEN trace/generated-helpers.h GEN trace/generated-helpers.c GEN module_block.h GEN tests/test-qapi-types.h GEN tests/test-qapi-visit.h GEN tests/test-qmp-commands.h GEN tests/test-qapi-event.h GEN tests/test-qmp-introspect.h GEN trace-root.h GEN util/trace.h GEN crypto/trace.h GEN io/trace.h GEN migration/trace.h GEN block/trace.h GEN backends/trace.h GEN chardev/trace.h GEN hw/block/trace.h GEN hw/block/dataplane/trace.h GEN hw/char/trace.h GEN hw/intc/trace.h GEN hw/net/trace.h GEN hw/virtio/trace.h GEN hw/audio/trace.h GEN hw/misc/trace.h GEN hw/usb/trace.h GEN hw/scsi/trace.h GEN hw/nvram/trace.h GEN hw/display/trace.h GEN hw/input/trace.h GEN hw/timer/trace.h GEN hw/dma/trace.h GEN hw/sparc/trace.h GEN hw/sd/trace.h GEN hw/isa/trace.h GEN hw/mem/trace.h GEN hw/i386/trace.h GEN hw/i386/xen/trace.h GEN hw/9pfs/trace.h GEN hw/ppc/trace.h GEN hw/pci/trace.h GEN hw/s390x/trace.h GEN hw/vfio/trace.h GEN hw/acpi/trace.h GEN hw/arm/trace.h GEN hw/alpha/trace.h GEN hw/xen/trace.h GEN ui/trace.h GEN audio/trace.h GEN net/trace.h GEN target/arm/trace.h GEN target/i386/trace.h GEN target/mips/trace.h GEN target/sparc/trace.h GEN target/s390x/trace.h GEN target/ppc/trace.h GEN qom/trace.h GEN linux-user/trace.h GEN qapi/trace.h GEN trace-root.c GEN util/trace.c GEN crypto/trace.c GEN io/trace.c GEN migration/trace.c GEN block/trace.c GEN backends/trace.c GEN chardev/trace.c GEN hw/block/trace.c GEN hw/block/dataplane/trace.c GEN hw/char/trace.c GEN hw/intc/trace.c GEN hw/net/trace.c GEN hw/virtio/trace.c GEN hw/audio/trace.c GEN hw/misc/trace.c GEN hw/usb/trace.c GEN hw/scsi/trace.c GEN hw/nvram/trace.c GEN hw/display/trace.c GEN hw/input/trace.c GEN hw/timer/trace.c GEN hw/dma/trace.c GEN hw/sparc/trace.c GEN hw/sd/trace.c GEN hw/isa/trace.c GEN hw/mem/trace.c GEN hw/i386/trace.c GEN hw/i386/xen/trace.c GEN hw/9pfs/trace.c GEN hw/ppc/trace.c GEN hw/pci/trace.c GEN hw/s390x/trace.c GEN hw/vfio/trace.c GEN hw/acpi/trace.c GEN hw/arm/trace.c GEN hw/alpha/trace.c GEN hw/xen/trace.c GEN ui/trace.c GEN audio/trace.c GEN net/trace.c GEN target/arm/trace.c GEN target/i386/trace.c GEN target/mips/trace.c GEN target/sparc/trace.c GEN target/s390x/trace.c GEN target/ppc/trace.c GEN qom/trace.c GEN linux-user/trace.c GEN qapi/trace.c GEN config-all-devices.mak DEP /tmp/qemu-test/src/dtc/tests/dumptrees.c DEP /tmp/qemu-test/src/dtc/tests/trees.S DEP /tmp/qemu-test/src/dtc/tests/testutils.c DEP /tmp/qemu-test/src/dtc/tests/value-labels.c DEP /tmp/qemu-test/src/dtc/tests/asm_tree_dump.c DEP /tmp/qemu-test/src/dtc/tests/truncated_property.c DEP /tmp/qemu-test/src/dtc/tests/check_path.c DEP /tmp/qemu-test/src/dtc/tests/overlay_bad_fixup.c DEP /tmp/qemu-test/src/dtc/tests/overlay.c DEP /tmp/qemu-test/src/dtc/tests/subnode_iterate.c DEP /tmp/qemu-test/src/dtc/tests/property_iterate.c DEP /tmp/qemu-test/src/dtc/tests/utilfdt_test.c DEP /tmp/qemu-test/src/dtc/tests/integer-expressions.c DEP /tmp/qemu-test/src/dtc/tests/path_offset_aliases.c DEP /tmp/qemu-test/src/dtc/tests/add_subnode_with_nops.c DEP /tmp/qemu-test/src/dtc/tests/dtbs_equal_unordered.c DEP /tmp/qemu-test/src/dtc/tests/dtb_reverse.c DEP /tmp/qemu-test/src/dtc/tests/dtbs_equal_ordered.c DEP /tmp/qemu-test/src/dtc/tests/extra-terminating-null.c DEP /tmp/qemu-test/src/dtc/tests/boot-cpuid.c DEP /tmp/qemu-test/src/dtc/tests/incbin.c DEP /tmp/qemu-test/src/dtc/tests/phandle_format.c DEP /tmp/qemu-test/src/dtc/tests/path-references.c DEP /tmp/qemu-test/src/dtc/tests/references.c DEP /tmp/qemu-test/src/dtc/tests/string_escapes.c DEP /tmp/qemu-test/src/dtc/tests/propname_escapes.c DEP /tmp/qemu-test/src/dtc/tests/appendprop2.c DEP /tmp/qemu-test/src/dtc/tests/appendprop1.c DEP /tmp/qemu-test/src/dtc/tests/del_node.c DEP /tmp/qemu-test/src/dtc/tests/del_property.c DEP /tmp/qemu-test/src/dtc/tests/setprop.c DEP /tmp/qemu-test/src/dtc/tests/set_name.c DEP /tmp/qemu-test/src/dtc/tests/rw_tree1.c DEP /tmp/qemu-test/src/dtc/tests/open_pack.c DEP /tmp/qemu-test/src/dtc/tests/nopulate.c DEP /tmp/qemu-test/src/dtc/tests/mangle-layout.c DEP /tmp/qemu-test/src/dtc/tests/move_and_save.c DEP /tmp/qemu-test/src/dtc/tests/sw_tree1.c DEP /tmp/qemu-test/src/dtc/tests/nop_node.c DEP /tmp/qemu-test/src/dtc/tests/nop_property.c DEP /tmp/qemu-test/src/dtc/tests/setprop_inplace.c DEP /tmp/qemu-test/src/dtc/tests/stringlist.c DEP /tmp/qemu-test/src/dtc/tests/addr_size_cells.c DEP /tmp/qemu-test/src/dtc/tests/notfound.c DEP /tmp/qemu-test/src/dtc/tests/sized_cells.c DEP /tmp/qemu-test/src/dtc/tests/char_literal.c DEP /tmp/qemu-test/src/dtc/tests/get_alias.c DEP /tmp/qemu-test/src/dtc/tests/node_offset_by_compatible.c DEP /tmp/qemu-test/src/dtc/tests/node_check_compatible.c DEP /tmp/qemu-test/src/dtc/tests/node_offset_by_phandle.c DEP /tmp/qemu-test/src/dtc/tests/node_offset_by_prop_value.c DEP /tmp/qemu-test/src/dtc/tests/parent_offset.c DEP /tmp/qemu-test/src/dtc/tests/supernode_atdepth_offset.c DEP /tmp/qemu-test/src/dtc/tests/get_path.c DEP /tmp/qemu-test/src/dtc/tests/get_phandle.c DEP /tmp/qemu-test/src/dtc/tests/getprop.c DEP /tmp/qemu-test/src/dtc/tests/get_name.c DEP /tmp/qemu-test/src/dtc/tests/path_offset.c DEP /tmp/qemu-test/src/dtc/tests/subnode_offset.c DEP /tmp/qemu-test/src/dtc/tests/find_property.c DEP /tmp/qemu-test/src/dtc/tests/root_node.c DEP /tmp/qemu-test/src/dtc/tests/get_mem_rsv.c DEP /tmp/qemu-test/src/dtc/libfdt/fdt_overlay.c DEP /tmp/qemu-test/src/dtc/libfdt/fdt_addresses.c DEP /tmp/qemu-test/src/dtc/libfdt/fdt_empty_tree.c DEP /tmp/qemu-test/src/dtc/libfdt/fdt_strerror.c DEP /tmp/qemu-test/src/dtc/libfdt/fdt_rw.c DEP /tmp/qemu-test/src/dtc/libfdt/fdt_sw.c DEP /tmp/qemu-test/src/dtc/libfdt/fdt_wip.c DEP /tmp/qemu-test/src/dtc/libfdt/fdt_ro.c DEP /tmp/qemu-test/src/dtc/libfdt/fdt.c DEP /tmp/qemu-test/src/dtc/util.c DEP /tmp/qemu-test/src/dtc/fdtput.c DEP /tmp/qemu-test/src/dtc/fdtget.c DEP /tmp/qemu-test/src/dtc/fdtdump.c LEX convert-dtsv0-lexer.lex.c make[1]: flex: Command not found DEP /tmp/qemu-test/src/dtc/srcpos.c BISON dtc-parser.tab.c make[1]: bison: Command not found LEX dtc-lexer.lex.c make[1]: flex: Command not found DEP /tmp/qemu-test/src/dtc/treesource.c DEP /tmp/qemu-test/src/dtc/livetree.c DEP /tmp/qemu-test/src/dtc/fstree.c DEP /tmp/qemu-test/src/dtc/flattree.c DEP /tmp/qemu-test/src/dtc/dtc.c DEP /tmp/qemu-test/src/dtc/data.c DEP /tmp/qemu-test/src/dtc/checks.c CHK version_gen.h LEX convert-dtsv0-lexer.lex.c BISON dtc-parser.tab.c make[1]: flex: Command not found make[1]: bison: Command not found UPD version_gen.h LEX dtc-lexer.lex.c make[1]: flex: Command not found DEP /tmp/qemu-test/src/dtc/util.c LEX convert-dtsv0-lexer.lex.c BISON dtc-parser.tab.c make[1]: flex: Command not found make[1]: bison: Command not found LEX dtc-lexer.lex.c make[1]: flex: Command not found CC libfdt/fdt.o CC libfdt/fdt_ro.o CC libfdt/fdt_wip.o CC libfdt/fdt_sw.o CC libfdt/fdt_rw.o CC libfdt/fdt_strerror.o CC libfdt/fdt_addresses.o CC libfdt/fdt_empty_tree.o CC libfdt/fdt_overlay.o AR libfdt/libfdt.a ar: creating libfdt/libfdt.a a - libfdt/fdt.o a - libfdt/fdt_ro.o a - libfdt/fdt_wip.o a - libfdt/fdt_sw.o a - libfdt/fdt_rw.o a - libfdt/fdt_strerror.o a - libfdt/fdt_empty_tree.o a - libfdt/fdt_addresses.o a - libfdt/fdt_overlay.o BISON dtc-parser.tab.c make[1]: bison: Command not found LEX convert-dtsv0-lexer.lex.c make[1]: flex: Command not found LEX dtc-lexer.lex.c make[1]: flex: Command not found GEN qga/qapi-generated/qga-qapi-types.h CC tests/qemu-iotests/socket_scm_helper.o GEN qga/qapi-generated/qga-qapi-types.c GEN qga/qapi-generated/qga-qapi-visit.h CC qmp-introspect.o GEN qga/qapi-generated/qga-qmp-marshal.c GEN qga/qapi-generated/qga-qmp-commands.h GEN qga/qapi-generated/qga-qapi-visit.c CC qapi-types.o CC qapi-visit.o CC qapi-event.o CC qapi/qapi-visit-core.o CC qapi/qapi-dealloc-visitor.o CC qapi/qobject-input-visitor.o CC qapi/qobject-output-visitor.o CC qapi/qmp-registry.o CC qapi/qmp-dispatch.o CC qapi/string-input-visitor.o CC qapi/string-output-visitor.o CC qapi/opts-visitor.o CC qapi/qapi-clone-visitor.o CC qapi/qmp-event.o CC qapi/qapi-util.o CC qobject/qnull.o CC qobject/qstring.o CC qobject/qint.o CC qobject/qdict.o CC qobject/qlist.o CC qobject/qfloat.o CC qobject/qbool.o CC qobject/qjson.o CC qobject/qobject.o CC qobject/json-lexer.o CC qobject/json-streamer.o CC trace/control.o CC qobject/json-parser.o CC trace/qmp.o CC util/unicode.o CC util/osdep.o CC util/cutils.o CC util/qemu-timer-common.o CC util/lockcnt.o CC util/aiocb.o CC util/bufferiszero.o CC util/async.o CC util/thread-pool.o CC util/qemu-timer.o CC util/main-loop.o CC util/iohandler.o CC util/aio-posix.o CC util/compatfd.o CC util/event_notifier-posix.o CC util/mmap-alloc.o CC util/oslib-posix.o CC util/qemu-openpty.o CC util/qemu-thread-posix.o CC util/memfd.o CC util/path.o CC util/envlist.o CC util/module.o CC util/bitmap.o CC util/bitops.o CC util/host-utils.o CC util/hbitmap.o CC util/fifo8.o CC util/acl.o CC util/error.o CC util/id.o CC util/qemu-error.o CC util/iov.o CC util/qemu-config.o CC util/qemu-sockets.o CC util/uri.o CC util/notify.o CC util/qemu-option.o CC util/qemu-progress.o CC util/keyval.o CC util/hexdump.o CC util/uuid.o CC util/crc32c.o CC util/throttle.o CC util/getauxval.o CC util/readline.o CC util/rcu.o CC util/qemu-coroutine.o CC util/qemu-coroutine-io.o CC util/qemu-coroutine-sleep.o CC util/qemu-coroutine-lock.o CC util/coroutine-ucontext.o CC util/buffer.o CC util/base64.o CC util/timed-average.o CC util/log.o CC util/qdist.o CC util/qht.o CC util/range.o CC util/systemd.o CC trace-root.o CC util/trace.o CC crypto/trace.o CC io/trace.o CC migration/trace.o CC block/trace.o CC backends/trace.o CC chardev/trace.o CC hw/block/trace.o CC hw/block/dataplane/trace.o CC hw/char/trace.o CC hw/intc/trace.o CC hw/net/trace.o CC hw/virtio/trace.o CC hw/audio/trace.o CC hw/misc/trace.o CC hw/usb/trace.o CC hw/nvram/trace.o CC hw/scsi/trace.o CC hw/display/trace.o CC hw/input/trace.o CC hw/dma/trace.o CC hw/timer/trace.o CC hw/sparc/trace.o CC hw/sd/trace.o CC hw/mem/trace.o CC hw/isa/trace.o CC hw/i386/trace.o CC hw/i386/xen/trace.o CC hw/9pfs/trace.o CC hw/ppc/trace.o CC hw/pci/trace.o CC hw/s390x/trace.o CC hw/acpi/trace.o CC hw/vfio/trace.o CC hw/arm/trace.o CC hw/xen/trace.o CC ui/trace.o CC audio/trace.o CC net/trace.o CC hw/alpha/trace.o CC target/arm/trace.o CC target/mips/trace.o CC target/i386/trace.o CC target/sparc/trace.o CC target/s390x/trace.o CC target/ppc/trace.o CC qom/trace.o CC linux-user/trace.o CC qapi/trace.o CC crypto/pbkdf-stub.o CC stubs/arch-query-cpu-def.o CC stubs/arch-query-cpu-model-expansion.o CC stubs/arch-query-cpu-model-comparison.o CC stubs/arch-query-cpu-model-baseline.o CC stubs/blk-commit-all.o CC stubs/bdrv-next-monitor-owned.o CC stubs/blockdev-close-all-bdrv-states.o CC stubs/clock-warp.o CC stubs/cpu-get-clock.o CC stubs/cpu-get-icount.o CC stubs/dump.o CC stubs/error-printf.o CC stubs/fdset.o CC stubs/gdbstub.o CC stubs/iothread.o CC stubs/iothread-lock.o CC stubs/get-vm-name.o CC stubs/is-daemonized.o CC stubs/machine-init-done.o CC stubs/migr-blocker.o CC stubs/monitor.o CC stubs/notify-event.o CC stubs/qtest.o CC stubs/replay.o CC stubs/runstate-check.o CC stubs/set-fd-handler.o CC stubs/slirp.o CC stubs/sysbus.o CC stubs/uuid.o CC stubs/vm-stop.o CC stubs/vmstate.o CC stubs/trace-control.o CC stubs/qmp_pc_dimm_device_list.o CC stubs/target-monitor-defs.o CC stubs/target-get-monitor-def.o CC stubs/pc_madt_cpu_entry.o CC stubs/vmgenid.o CC stubs/xen-common.o CC stubs/xen-hvm.o CC contrib/ivshmem-client/ivshmem-client.o CC contrib/ivshmem-client/main.o CC contrib/ivshmem-server/ivshmem-server.o CC contrib/ivshmem-server/main.o CC qemu-nbd.o CC block.o CC qemu-io-cmds.o CC blockjob.o CC replication.o CC block/raw-format.o CC block/qcow.o CC block/vdi.o CC block/cloop.o CC block/vmdk.o CC block/bochs.o CC block/dmg.o CC block/vpc.o CC block/vvfat.o CC block/qcow2-refcount.o CC block/qcow2.o CC block/qcow2-cluster.o CC block/qcow2-snapshot.o CC block/qcow2-cache.o CC block/qed.o CC block/qed-gencb.o CC block/qed-l2-cache.o CC block/qed-table.o CC block/qed-cluster.o CC block/qed-check.o CC block/vhdx.o CC block/vhdx-endian.o CC block/vhdx-log.o CC block/quorum.o CC block/parallels.o CC block/blkdebug.o CC block/blkverify.o CC block/blkreplay.o CC block/snapshot.o CC block/block-backend.o CC block/qapi.o CC block/file-posix.o CC block/null.o CC block/mirror.o CC block/commit.o CC block/io.o CC block/throttle-groups.o CC block/nbd.o CC block/nbd-client.o CC block/sheepdog.o CC block/dirty-bitmap.o CC block/accounting.o CC block/write-threshold.o CC block/backup.o CC block/replication.o CC block/crypto.o CC nbd/server.o CC nbd/client.o CC nbd/common.o CC crypto/init.o CC crypto/hash.o CC crypto/hash-glib.o CC crypto/hmac.o CC crypto/hmac-glib.o CC crypto/aes.o CC crypto/desrfb.o CC crypto/cipher.o CC crypto/tlscreds.o CC crypto/tlscredsanon.o CC crypto/tlscredsx509.o CC crypto/tlssession.o CC crypto/secret.o CC crypto/random-platform.o CC crypto/ivgen.o CC crypto/pbkdf.o CC crypto/ivgen-essiv.o CC crypto/ivgen-plain.o CC crypto/ivgen-plain64.o CC crypto/afsplit.o CC crypto/xts.o CC crypto/block.o CC crypto/block-qcow.o CC crypto/block-luks.o CC io/channel.o CC io/channel-buffer.o CC io/channel-command.o CC io/channel-file.o CC io/channel-socket.o CC io/channel-watch.o CC io/channel-tls.o CC io/channel-websock.o CC io/channel-util.o CC io/dns-resolver.o CC io/task.o CC qom/container.o CC qom/qom-qobject.o GEN qemu-img-cmds.h CC qom/object.o CC qom/object_interfaces.o CC qemu-io.o CC qemu-bridge-helper.o CC blockdev.o CC iothread.o CC blockdev-nbd.o CC qdev-monitor.o CC device-hotplug.o CC os-posix.o CC accel.o CC bt-host.o CC bt-vhci.o CC dma-helpers.o CC vl.o CC tpm.o CC device_tree.o CC qmp-marshal.o CC qmp.o CC hmp.o CC cpus-common.o CC audio/audio.o CC audio/noaudio.o CC audio/wavaudio.o CC audio/mixeng.o CC audio/sdlaudio.o CC audio/ossaudio.o CC audio/wavcapture.o CC backends/rng.o CC backends/rng-egd.o CC backends/tpm.o CC backends/rng-random.o CC backends/hostmem.o CC backends/hostmem-ram.o CC backends/hostmem-file.o CC backends/cryptodev.o CC backends/cryptodev-builtin.o CC block/stream.o CC chardev/msmouse.o CC chardev/wctablet.o CC chardev/testdev.o CC disas/arm.o CC disas/i386.o CC fsdev/qemu-fsdev-dummy.o CC fsdev/qemu-fsdev-opts.o CC fsdev/qemu-fsdev-throttle.o CC hw/acpi/piix4.o CC hw/acpi/pcihp.o CC hw/acpi/core.o CC hw/acpi/ich9.o CC hw/acpi/tco.o CC hw/acpi/memory_hotplug.o CC hw/acpi/cpu_hotplug.o CC hw/acpi/cpu.o CC hw/acpi/vmgenid.o CC hw/acpi/nvdimm.o CC hw/acpi/acpi_interface.o CC hw/acpi/bios-linker-loader.o CC hw/acpi/ipmi.o CC hw/acpi/aml-build.o CC hw/acpi/acpi-stub.o CC hw/acpi/ipmi-stub.o CC hw/audio/sb16.o CC hw/audio/es1370.o CC hw/audio/ac97.o CC hw/audio/fmopl.o CC hw/audio/adlib.o CC hw/audio/gus.o CC hw/audio/gusemu_hal.o CC hw/audio/gusemu_mixer.o CC hw/audio/cs4231a.o CC hw/audio/intel-hda.o CC hw/audio/hda-codec.o CC hw/audio/pcspk.o CC hw/audio/pl041.o CC hw/audio/wm8750.o CC hw/audio/lm4549.o CC hw/audio/soundhw.o CC hw/audio/marvell_88w8618.o CC hw/block/block.o CC hw/block/cdrom.o CC hw/block/hd-geometry.o CC hw/block/nand.o CC hw/block/pflash_cfi01.o CC hw/block/pflash_cfi02.o CC hw/block/fdc.o CC hw/block/m25p80.o CC hw/block/ecc.o CC hw/block/onenand.o CC hw/block/nvme.o CC hw/bt/core.o CC hw/bt/l2cap.o CC hw/bt/hci.o CC hw/bt/sdp.o CC hw/bt/hci-csr.o CC hw/bt/hid.o CC hw/char/ipoctal232.o CC hw/char/pl011.o CC hw/char/serial.o CC hw/char/parallel.o CC hw/char/serial-pci.o CC hw/char/serial-isa.o CC hw/char/virtio-console.o CC hw/char/cadence_uart.o CC hw/char/debugcon.o CC hw/char/imx_serial.o CC hw/core/qdev.o CC hw/core/qdev-properties.o CC hw/core/bus.o CC hw/core/reset.o CC hw/core/fw-path-provider.o CC hw/core/irq.o CC hw/core/hotplug.o CC hw/core/nmi.o CC hw/core/ptimer.o CC hw/core/sysbus.o CC hw/core/loader.o CC hw/core/machine.o CC hw/core/qdev-properties-system.o CC hw/core/register.o CC hw/core/or-irq.o CC hw/core/platform-bus.o CC hw/cpu/core.o CC hw/display/pl110.o CC hw/display/ssd0303.o CC hw/display/ads7846.o CC hw/display/ssd0323.o CC hw/display/cirrus_vga.o CC hw/display/vga-pci.o CC hw/display/vga-isa.o CC hw/display/vmware_vga.o CC hw/display/blizzard.o CC hw/display/framebuffer.o CC hw/display/exynos4210_fimd.o CC hw/display/tc6393xb.o CC hw/dma/pl080.o CC hw/dma/pl330.o CC hw/dma/i8257.o CC hw/dma/xlnx-zynq-devcfg.o CC hw/gpio/pl061.o CC hw/gpio/max7310.o CC hw/gpio/zaurus.o CC hw/gpio/gpio_key.o CC hw/i2c/core.o CC hw/i2c/smbus.o CC hw/i2c/smbus_eeprom.o CC hw/i2c/i2c-ddc.o CC hw/i2c/versatile_i2c.o CC hw/i2c/smbus_ich9.o CC hw/i2c/pm_smbus.o CC hw/i2c/bitbang_i2c.o CC hw/i2c/exynos4210_i2c.o CC hw/i2c/imx_i2c.o CC hw/i2c/aspeed_i2c.o CC hw/ide/core.o CC hw/ide/atapi.o CC hw/ide/qdev.o CC hw/ide/isa.o CC hw/ide/pci.o CC hw/ide/piix.o CC hw/ide/microdrive.o CC hw/ide/ahci.o CC hw/ide/ich.o CC hw/input/hid.o CC hw/input/lm832x.o CC hw/input/pckbd.o CC hw/input/pl050.o CC hw/input/ps2.o CC hw/input/stellaris_input.o CC hw/input/tsc2005.o CC hw/input/vmmouse.o CC hw/input/virtio-input-hid.o CC hw/input/virtio-input.o CC hw/input/virtio-input-host.o CC hw/intc/i8259_common.o CC hw/intc/i8259.o CC hw/intc/pl190.o CC hw/intc/imx_avic.o CC hw/intc/ioapic_common.o CC hw/intc/realview_gic.o CC hw/intc/arm_gic_common.o CC hw/intc/arm_gic.o CC hw/intc/arm_gicv2m.o CC hw/intc/arm_gicv3_common.o CC hw/intc/arm_gicv3_dist.o CC hw/intc/arm_gicv3.o CC hw/intc/arm_gicv3_redist.o CC hw/intc/arm_gicv3_its_common.o CC hw/intc/intc.o CC hw/ipack/tpci200.o CC hw/ipack/ipack.o CC hw/ipmi/ipmi.o CC hw/ipmi/ipmi_bmc_sim.o CC hw/ipmi/ipmi_bmc_extern.o CC hw/ipmi/isa_ipmi_kcs.o CC hw/ipmi/isa_ipmi_bt.o CC hw/isa/apm.o CC hw/isa/isa-bus.o CC hw/mem/pc-dimm.o CC hw/mem/nvdimm.o CC hw/misc/applesmc.o CC hw/misc/max111x.o CC hw/misc/tmp105.o CC hw/misc/tmp421.o CC hw/misc/debugexit.o CC hw/misc/sga.o CC hw/misc/pc-testdev.o CC hw/misc/unimp.o CC hw/misc/pci-testdev.o CC hw/misc/arm_l2x0.o CC hw/misc/arm_integrator_debug.o CC hw/misc/a9scu.o CC hw/misc/arm11scu.o CC hw/net/ne2000.o CC hw/net/pcnet-pci.o CC hw/net/eepro100.o CC hw/net/pcnet.o CC hw/net/e1000.o CC hw/net/e1000x_common.o CC hw/net/net_tx_pkt.o CC hw/net/net_rx_pkt.o CC hw/net/e1000e.o CC hw/net/rtl8139.o CC hw/net/vmxnet3.o CC hw/net/e1000e_core.o CC hw/net/smc91c111.o CC hw/net/lan9118.o CC hw/net/ne2000-isa.o CC hw/net/imx_fec.o CC hw/net/xgmac.o CC hw/net/allwinner_emac.o CC hw/net/cadence_gem.o CC hw/net/stellaris_enet.o CC hw/net/ftgmac100.o CC hw/net/rocker/rocker.o CC hw/net/rocker/rocker_fp.o CC hw/net/rocker/rocker_desc.o CC hw/net/rocker/rocker_world.o CC hw/nvram/eeprom93xx.o CC hw/nvram/fw_cfg.o CC hw/nvram/chrp_nvram.o CC hw/pci-bridge/pci_bridge_dev.o CC hw/pci-bridge/pcie_root_port.o CC hw/net/rocker/rocker_of_dpa.o CC hw/pci-bridge/gen_pcie_root_port.o CC hw/pci-bridge/pci_expander_bridge.o CC hw/pci-bridge/xio3130_upstream.o CC hw/pci-bridge/xio3130_downstream.o CC hw/pci-bridge/ioh3420.o CC hw/pci-host/pam.o CC hw/pci-bridge/i82801b11.o CC hw/pci-host/versatile.o CC hw/pci-host/piix.o CC hw/pci-host/q35.o CC hw/pci-host/gpex.o CC hw/pci/pci.o CC hw/pci/pci_bridge.o In file included from /tmp/qemu-test/src/hw/net/vmxnet3.c:30: /tmp/qemu-test/src/include/migration/register.h:18: error: redefinition of typedef ‘LoadStateHandler’ /tmp/qemu-test/src/include/migration/vmstate.h:32: note: previous declaration of ‘LoadStateHandler’ was here CC hw/pci/msix.o CC hw/pci/msi.o make: *** [hw/net/vmxnet3.o] Error 1 make: *** Waiting for unfinished jobs.... CC hw/pci/slotid_cap.o CC hw/pci/shpc.o tests/docker/Makefile.include:118: recipe for target 'docker-run' failed make[1]: *** [docker-run] Error 2 make[1]: Leaving directory '/var/tmp/patchew-tester-tmp-uxcyyu1e/src' tests/docker/Makefile.include:149: recipe for target 'docker-run-test-quick@centos6' failed make: *** [docker-run-test-quick@centos6] Error 2 === OUTPUT END === Test command exited with code: 2 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] fix: avoid infinite loop when blockjob encountering failure 2017-06-14 6:33 ` no-reply @ 2017-06-14 6:46 ` Fam Zheng 2017-06-14 6:58 ` sochin.jiang 0 siblings, 1 reply; 8+ messages in thread From: Fam Zheng @ 2017-06-14 6:46 UTC (permalink / raw) To: qemu-devel Cc: sochin.jiang, kwolf, mreitz, xieyingtai, lina.lulina, lizhengui, qemu-block, subo7, eric.fangyi, zhangshuai13 On Tue, 06/13 23:33, no-reply@patchew.org wrote: > In file included from /tmp/qemu-test/src/hw/net/vmxnet3.c:30: > /tmp/qemu-test/src/include/migration/register.h:18: error: redefinition of typedef ‘LoadStateHandler’ > /tmp/qemu-test/src/include/migration/vmstate.h:32: note: previous declaration of ‘LoadStateHandler’ was here Please ignore this error for now, the bug is on master. Fam ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] fix: avoid infinite loop when blockjob encountering failure 2017-06-14 6:46 ` Fam Zheng @ 2017-06-14 6:58 ` sochin.jiang 0 siblings, 0 replies; 8+ messages in thread From: sochin.jiang @ 2017-06-14 6:58 UTC (permalink / raw) To: Fam Zheng, qemu-devel Cc: kwolf, mreitz, xieyingtai, lina.lulina, lizhengui, qemu-block, subo7, eric.fangyi, zhangshuai13 All right, Thanks... On 2017/6/14 14:46, Fam Zheng wrote: > On Tue, 06/13 23:33, no-reply@patchew.org wrote: >> In file included from /tmp/qemu-test/src/hw/net/vmxnet3.c:30: >> /tmp/qemu-test/src/include/migration/register.h:18: error: redefinition of typedef ‘LoadStateHandler’ >> /tmp/qemu-test/src/include/migration/vmstate.h:32: note: previous declaration of ‘LoadStateHandler’ was here > Please ignore this error for now, the bug is on master. > > Fam > > . > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] fix: avoid infinite loop when blockjob encountering failure 2017-06-14 6:22 [Qemu-devel] [PATCH] fix: avoid infinite loop when blockjob encountering failure sochin.jiang 2017-06-14 6:33 ` no-reply @ 2017-06-14 13:12 ` Max Reitz 2017-06-15 2:38 ` sochin.jiang 1 sibling, 1 reply; 8+ messages in thread From: Max Reitz @ 2017-06-14 13:12 UTC (permalink / raw) To: sochin.jiang, kwolf Cc: qemu-block, qemu-devel, eric.fangyi, subo7, xieyingtai, lina.lulina, zhangshuai13, lizhengui [-- Attachment #1: Type: text/plain, Size: 3140 bytes --] Thanks for your patch! The issue can be reproduced as follows: $ qemu-img create -f qcow2 -b \ "json:{'driver':'raw','file':{ 'driver':'blkdebug','inject-error':[{'event':'write_aio'}], 'image':{'driver':'null-co'}}}" \ overlay.qcow2 $ qemu-io -c 'write 0 64k' overlay.qcow2 $ qemu-img commit overlay.qcow2 While your patch fixes that issue, I still have some comments: On 2017-06-14 08:22, sochin.jiang wrote: > From: "sochin.jiang" <sochin.jiang@huawei.com> > > img_commit could fall into infinite loop if it's blockjob This should be "into an infinite loop" and "its" instead if "it's". > This empty line should be omitted. > fail encountering any I/O error. Try to fix it. Should be "fails on any I/O error" or "fails on encountering any I/O error". Also, you're not trying to fix it but let's all hope you really are fixing it. :-) (So "Fix it." instead of "Try to fix it.") > > Signed-off-by: sochin.jiang <sochin.jiang@huawei.com> > --- > qemu-img.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/qemu-img.c b/qemu-img.c > index 0ad698d..6ba565d 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -895,8 +895,11 @@ static void run_block_job(BlockJob *job, Error **errp) > aio_poll(aio_context, true); > qemu_progress_print(job->len ? > ((float)job->offset / job->len * 100.f) : 0.0f, 0); > - } while (!job->ready); > + } while (!job->ready && !job->ret); I think it would be better to test job->completed instead of job->ret. > > + if (job->ret) { > + return; > + } We shouldn't just return here but still do all the deinitialization like call aio_context_release(). I guess the best would be to just skip the block_job_complete_sync() call if job->completed is true. > block_job_complete_sync(job, errp); > aio_context_release(aio_context); Then, there are three more issues I found while reviewing this patch: First, if the block job is completed before block_job_complete_sync() is called (i.e. if an error occurred), it is automatically freed. This is bad because this means we'll have some instances of use-after-free here. Therefore, we need to invoke block_job_ref() before run_block_job() and block_job_unref() afterwards. (And since these functions are currenctly static in blockjob.c, we'll have to make them global.) Secondly, run_block_job() doesn't evaluate job->ret. Therefore it will report success even if the commit failed (it is expecting block_job_complete_sync() to put an error into errp, but it will not do that). So we'll have to do that (manually check job->ret and if it's negative, put an error message into errp; also, assert that job->cancelled is false). Thirdly, we have segfault in bdrv_reopen_prepare() if the image has non-string options... I'll handle this one. I can also handle the other two issues, if you'd like me to. Finally, an iotest would be nice (see my reproducer above). But I can handle that as well, if you decide not to write one. Max [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 498 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] fix: avoid infinite loop when blockjob encountering failure 2017-06-14 13:12 ` Max Reitz @ 2017-06-15 2:38 ` sochin.jiang 2017-06-15 3:24 ` sochin.jiang 2017-06-16 13:06 ` Max Reitz 0 siblings, 2 replies; 8+ messages in thread From: sochin.jiang @ 2017-06-15 2:38 UTC (permalink / raw) To: Max Reitz, kwolf Cc: qemu-block, qemu-devel, eric.fangyi, subo7, xieyingtai, lina.lulina, zhangshuai13, lizhengui Thanks for your kindly reply. I do have made a mistake that ignoring the AIOContext lock. About the patch, firstly, if job->ret comes to be non-zero(also means job->completed to be true) , blockjob 'callback'(common_block_job_cb) will be called, blockjob error will be put into errp. It won't report success. Secondly, blockjob fails with 'ret < 0' and without calling block_job_complete_sync(), we won't have segfault because bdrv_reopen won't be called. Also, with the use-after-free problems. So, skip the block_job_complete_sync() call if job->completed(job->ret to be non-zero) is true can avoid all the problems, am I right ? Thank you again. Best Regard. Sochin On 2017/6/14 21:12, Max Reitz wrote: > Thanks for your patch! The issue can be reproduced as follows: > > $ qemu-img create -f qcow2 -b \ > "json:{'driver':'raw','file':{ > 'driver':'blkdebug','inject-error':[{'event':'write_aio'}], > 'image':{'driver':'null-co'}}}" \ > overlay.qcow2 > $ qemu-io -c 'write 0 64k' overlay.qcow2 > $ qemu-img commit overlay.qcow2 > > While your patch fixes that issue, I still have some comments: > > On 2017-06-14 08:22, sochin.jiang wrote: >> From: "sochin.jiang" <sochin.jiang@huawei.com> >> >> img_commit could fall into infinite loop if it's blockjob > This should be "into an infinite loop" and "its" instead if "it's". > > This empty line should be omitted. > >> fail encountering any I/O error. Try to fix it. > Should be "fails on any I/O error" or "fails on encountering any I/O > error". Also, you're not trying to fix it but let's all hope you really > are fixing it. :-) > > (So "Fix it." instead of "Try to fix it.") > >> Signed-off-by: sochin.jiang <sochin.jiang@huawei.com> >> --- >> qemu-img.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/qemu-img.c b/qemu-img.c >> index 0ad698d..6ba565d 100644 >> --- a/qemu-img.c >> +++ b/qemu-img.c >> @@ -895,8 +895,11 @@ static void run_block_job(BlockJob *job, Error **errp) >> aio_poll(aio_context, true); >> qemu_progress_print(job->len ? >> ((float)job->offset / job->len * 100.f) : 0.0f, 0); >> - } while (!job->ready); >> + } while (!job->ready && !job->ret); > I think it would be better to test job->completed instead of job->ret. > >> >> + if (job->ret) { >> + return; >> + } > We shouldn't just return here but still do all the deinitialization like > call aio_context_release(). I guess the best would be to just skip the > block_job_complete_sync() call if job->completed is true. > >> block_job_complete_sync(job, errp); >> aio_context_release(aio_context); > Then, there are three more issues I found while reviewing this patch: > > First, if the block job is completed before block_job_complete_sync() is > called (i.e. if an error occurred), it is automatically freed. This is > bad because this means we'll have some instances of use-after-free here. > Therefore, we need to invoke block_job_ref() before run_block_job() and > block_job_unref() afterwards. (And since these functions are currenctly > static in blockjob.c, we'll have to make them global.) > > Secondly, run_block_job() doesn't evaluate job->ret. Therefore it will > report success even if the commit failed (it is expecting > block_job_complete_sync() to put an error into errp, but it will not do > that). So we'll have to do that (manually check job->ret and if it's > negative, put an error message into errp; also, assert that > job->cancelled is false). > > Thirdly, we have segfault in bdrv_reopen_prepare() if the image has > non-string options... I'll handle this one. > > I can also handle the other two issues, if you'd like me to. > > > Finally, an iotest would be nice (see my reproducer above). But I can > handle that as well, if you decide not to write one. > > Max > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] fix: avoid infinite loop when blockjob encountering failure 2017-06-15 2:38 ` sochin.jiang @ 2017-06-15 3:24 ` sochin.jiang 2017-06-16 13:06 ` Max Reitz 1 sibling, 0 replies; 8+ messages in thread From: sochin.jiang @ 2017-06-15 3:24 UTC (permalink / raw) To: Max Reitz, kwolf Cc: qemu-block, qemu-devel, eric.fangyi, subo7, xieyingtai, lina.lulina, zhangshuai13, lizhengui I realized blockjob is freed after completed unless we call block_job_ref() before run_block_job is called. On 2017/6/15 10:38, sochin.jiang wrote: > Thanks for your kindly reply. > > I do have made a mistake that ignoring the AIOContext lock. > > About the patch, firstly, if job->ret comes to be non-zero(also means job->completed to be true) , blockjob 'callback'(common_block_job_cb) will be called, blockjob error will be put into errp. It won't report success. > > Secondly, blockjob fails with 'ret < 0' and without calling block_job_complete_sync(), we won't have segfault because bdrv_reopen won't be called. Also, with the use-after-free problems. > > So, skip the block_job_complete_sync() call if job->completed(job->ret to be non-zero) is true can avoid all the problems, am I right ? > > Thank you again. > > > Best Regard. > > Sochin > > > > > > > > On 2017/6/14 21:12, Max Reitz wrote: >> Thanks for your patch! The issue can be reproduced as follows: >> >> $ qemu-img create -f qcow2 -b \ >> "json:{'driver':'raw','file':{ >> 'driver':'blkdebug','inject-error':[{'event':'write_aio'}], >> 'image':{'driver':'null-co'}}}" \ >> overlay.qcow2 >> $ qemu-io -c 'write 0 64k' overlay.qcow2 >> $ qemu-img commit overlay.qcow2 >> >> While your patch fixes that issue, I still have some comments: >> >> On 2017-06-14 08:22, sochin.jiang wrote: >>> From: "sochin.jiang" <sochin.jiang@huawei.com> >>> >>> img_commit could fall into infinite loop if it's blockjob >> This should be "into an infinite loop" and "its" instead if "it's". >> >> This empty line should be omitted. >> >>> fail encountering any I/O error. Try to fix it. >> Should be "fails on any I/O error" or "fails on encountering any I/O >> error". Also, you're not trying to fix it but let's all hope you really >> are fixing it. :-) >> >> (So "Fix it." instead of "Try to fix it.") >> >>> Signed-off-by: sochin.jiang <sochin.jiang@huawei.com> >>> --- >>> qemu-img.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/qemu-img.c b/qemu-img.c >>> index 0ad698d..6ba565d 100644 >>> --- a/qemu-img.c >>> +++ b/qemu-img.c >>> @@ -895,8 +895,11 @@ static void run_block_job(BlockJob *job, Error **errp) >>> aio_poll(aio_context, true); >>> qemu_progress_print(job->len ? >>> ((float)job->offset / job->len * 100.f) : 0.0f, 0); >>> - } while (!job->ready); >>> + } while (!job->ready && !job->ret); >> I think it would be better to test job->completed instead of job->ret. >> >>> >>> + if (job->ret) { >>> + return; >>> + } >> We shouldn't just return here but still do all the deinitialization like >> call aio_context_release(). I guess the best would be to just skip the >> block_job_complete_sync() call if job->completed is true. >> >>> block_job_complete_sync(job, errp); >>> aio_context_release(aio_context); >> Then, there are three more issues I found while reviewing this patch: >> >> First, if the block job is completed before block_job_complete_sync() is >> called (i.e. if an error occurred), it is automatically freed. This is >> bad because this means we'll have some instances of use-after-free here. >> Therefore, we need to invoke block_job_ref() before run_block_job() and >> block_job_unref() afterwards. (And since these functions are currenctly >> static in blockjob.c, we'll have to make them global.) >> >> Secondly, run_block_job() doesn't evaluate job->ret. Therefore it will >> report success even if the commit failed (it is expecting >> block_job_complete_sync() to put an error into errp, but it will not do >> that). So we'll have to do that (manually check job->ret and if it's >> negative, put an error message into errp; also, assert that >> job->cancelled is false). >> >> Thirdly, we have segfault in bdrv_reopen_prepare() if the image has >> non-string options... I'll handle this one. >> >> I can also handle the other two issues, if you'd like me to. >> >> >> Finally, an iotest would be nice (see my reproducer above). But I can >> handle that as well, if you decide not to write one. >> >> Max >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] fix: avoid infinite loop when blockjob encountering failure 2017-06-15 2:38 ` sochin.jiang 2017-06-15 3:24 ` sochin.jiang @ 2017-06-16 13:06 ` Max Reitz 1 sibling, 0 replies; 8+ messages in thread From: Max Reitz @ 2017-06-16 13:06 UTC (permalink / raw) To: sochin.jiang, kwolf Cc: qemu-block, qemu-devel, eric.fangyi, subo7, xieyingtai, lina.lulina, zhangshuai13, lizhengui [-- Attachment #1: Type: text/plain, Size: 1381 bytes --] On 2017-06-15 04:38, sochin.jiang wrote: > Thanks for your kindly reply. > > I do have made a mistake that ignoring the AIOContext lock. > > About the patch, firstly, if job->ret comes to be non-zero(also means job->completed to be true) , blockjob 'callback'(common_block_job_cb) will be called, blockjob error will be put into errp. It won't report success. Oh, right, good. > Secondly, blockjob fails with 'ret < 0' and without calling block_job_complete_sync(), we won't have segfault because bdrv_reopen won't be called. commit_active_start() invokes bdrv_reopen(), and that segfaults for me if the backing file is a bit unusual. See: $ qemu-img create -f qcow2 \ -b "json:{'driver':'raw','file':{ 'driver':'blkdebug','inject-error':[{ 'event':'write_aio','once':true }], 'image':{'driver':'null-co'}}}" \ overlay.qcow2 $ qemu-img commit overlay.qcow2 [1] 11080 segmentation fault (core dumped) qemu-img commit overlay.qcow2 Same if you actually write something to the overlay to trigger the write error (which would fail the block job). Yes, not directly related to your patch, which is why I've fixed it myself: http://lists.nongnu.org/archive/html/qemu-block/2017-06/msg00424.html > Also, with the use-after-free problems. No, but I see you noticed that. :-) Max [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 498 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-06-16 13:06 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-06-14 6:22 [Qemu-devel] [PATCH] fix: avoid infinite loop when blockjob encountering failure sochin.jiang 2017-06-14 6:33 ` no-reply 2017-06-14 6:46 ` Fam Zheng 2017-06-14 6:58 ` sochin.jiang 2017-06-14 13:12 ` Max Reitz 2017-06-15 2:38 ` sochin.jiang 2017-06-15 3:24 ` sochin.jiang 2017-06-16 13:06 ` Max Reitz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).