From 01a02ec4f6b6a12df7acfb6ad820b384b48cbf70 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 8 Aug 2017 09:34:16 -0500 Subject: [PATCH 1/7] tests/multiboot: Fix whitespace failure Commit b43671f8 accidentally broke run_test.sh within tests/multiboot; due to a subtle change in whitespace. These two commands produce theh same output (at least, for sane $IFS of space-tab-newline): echo -e "...$@..." echo -e "...$*..." But that's only because echo inserts spaces between multiple arguments (the $@ case), while the $* form gives a single argument to echo with the spaces already present. But when converting to printf %b, there are no automatic spaces between multiple arguments, so we HAVE to use $*. It doesn't help that run_test.sh isn't part of 'make check'. Signed-off-by: Eric Blake Signed-off-by: Kevin Wolf --- tests/multiboot/run_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/multiboot/run_test.sh b/tests/multiboot/run_test.sh index c8f3da8f37..0278148b43 100755 --- a/tests/multiboot/run_test.sh +++ b/tests/multiboot/run_test.sh @@ -26,7 +26,7 @@ run_qemu() { local kernel=$1 shift - printf %b "\n\n=== Running test case: $kernel $@ ===\n\n" >> test.log + printf %b "\n\n=== Running test case: $kernel $* ===\n\n" >> test.log $QEMU \ -kernel $kernel \ From 81caa3cc3bdd9f5406130ee5bea94d5d7ee6e2e2 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 9 Aug 2017 15:38:04 -0500 Subject: [PATCH 2/7] vpc: Check failure of bdrv_getlength() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vpc_open() was checking for bdrv_getlength() failure in one, but not the other, location. Reported-by: Markus Armbruster Signed-off-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Jeff Cody Reviewed-by: John Snow Signed-off-by: Kevin Wolf --- block/vpc.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/block/vpc.c b/block/vpc.c index 574879ba7c..82911ebead 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -219,6 +219,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, uint64_t pagetable_size; int disk_type = VHD_DYNAMIC; int ret; + int64_t bs_size; bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false, errp); @@ -411,7 +412,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, } } - if (s->free_data_block_offset > bdrv_getlength(bs->file->bs)) { + bs_size = bdrv_getlength(bs->file->bs); + if (bs_size < 0) { + error_setg_errno(errp, -bs_size, "Unable to learn image size"); + ret = bs_size; + goto fail; + } + if (s->free_data_block_offset > bs_size) { error_setg(errp, "block-vpc: free_data_block_offset points after " "the end of file. The image has been truncated."); ret = -EINVAL; From c40fe9c06c35fa8076f9a0daeea5c8684d773645 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 9 Aug 2017 15:38:07 -0500 Subject: [PATCH 3/7] qcow2: Drop debugging dump_refcounts() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's been #if 0'd since its introduction in 2006, commit 585f8587. We can revive dead code if we need it, but in the meantime, it has bit-rotted (for example, not checking for failure in bdrv_getlength()). Signed-off-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Jeff Cody Reviewed-by: Kevin Wolf Reviewed-by: John Snow Signed-off-by: Kevin Wolf --- block/qcow2.c | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index d7c600b5a2..99407403ea 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3798,27 +3798,6 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs) return spec_info; } -#if 0 -static void dump_refcounts(BlockDriverState *bs) -{ - BDRVQcow2State *s = bs->opaque; - int64_t nb_clusters, k, k1, size; - int refcount; - - size = bdrv_getlength(bs->file->bs); - nb_clusters = size_to_clusters(s, size); - for(k = 0; k < nb_clusters;) { - k1 = k; - refcount = get_refcount(bs, k); - k++; - while (k < nb_clusters && get_refcount(bs, k) == refcount) - k++; - printf("%" PRId64 ": refcount=%d nb=%" PRId64 "\n", k, refcount, - k - k1); - } -} -#endif - static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos) { From d0d5d0e31a874d592741a088c2c5071bae164dbf Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Wed, 9 Aug 2017 15:38:08 -0500 Subject: [PATCH 4/7] qcow2: Check failure of bdrv_getlength() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit qcow2_co_pwritev_compressed() should not call bdrv_truncate() if determining the size failed. Reported-by: Markus Armbruster Signed-off-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Jeff Cody Reviewed-by: Kevin Wolf Reviewed-by: John Snow Signed-off-by: Kevin Wolf --- block/qcow2.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/block/qcow2.c b/block/qcow2.c index 99407403ea..40ba26c111 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3282,12 +3282,15 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, z_stream strm; int ret, out_len; uint8_t *buf, *out_buf; - uint64_t cluster_offset; + int64_t cluster_offset; if (bytes == 0) { /* align end of file to a sector boundary to ease reading with sector based I/Os */ cluster_offset = bdrv_getlength(bs->file->bs); + if (cluster_offset < 0) { + return cluster_offset; + } return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, NULL); } From ca749954b09b89e22cd69c4949fb7e689b057963 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 11 Aug 2017 19:44:46 +0800 Subject: [PATCH 5/7] osdep: Add runtime OFD lock detection Build time check of OFD lock is not sufficient and can cause image open errors when the runtime environment doesn't support it. Add a helper function to probe it at runtime, additionally. Also provide a qemu_has_ofd_lock() for callers to check the status. Signed-off-by: Fam Zheng Signed-off-by: Kevin Wolf --- include/qemu/osdep.h | 1 + util/osdep.c | 66 +++++++++++++++++++++++++++++++++++++------- 2 files changed, 57 insertions(+), 10 deletions(-) diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h index 3b74f6fcb2..6855b94bbf 100644 --- a/include/qemu/osdep.h +++ b/include/qemu/osdep.h @@ -357,6 +357,7 @@ int qemu_dup(int fd); int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive); int qemu_unlock_fd(int fd, int64_t start, int64_t len); int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive); +bool qemu_has_ofd_lock(void); #if defined(__HAIKU__) && defined(__i386__) #define FMT_pid "%ld" diff --git a/util/osdep.c b/util/osdep.c index a2863c8e53..a479fedc4a 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -38,14 +38,6 @@ extern int madvise(caddr_t, size_t, int); #include "qemu/error-report.h" #include "monitor/monitor.h" -#ifdef F_OFD_SETLK -#define QEMU_SETLK F_OFD_SETLK -#define QEMU_GETLK F_OFD_GETLK -#else -#define QEMU_SETLK F_SETLK -#define QEMU_GETLK F_GETLK -#endif - static bool fips_enabled = false; static const char *hw_version = QEMU_HW_VERSION; @@ -82,6 +74,10 @@ int qemu_madvise(void *addr, size_t len, int advice) } #ifndef _WIN32 + +static int fcntl_op_setlk = -1; +static int fcntl_op_getlk = -1; + /* * Dups an fd and sets the flags */ @@ -149,6 +145,54 @@ static int qemu_parse_fdset(const char *param) return qemu_parse_fd(param); } +static void qemu_probe_lock_ops(void) +{ + if (fcntl_op_setlk == -1) { +#ifdef F_OFD_SETLK + int fd; + int ret; + struct flock fl = { + .l_whence = SEEK_SET, + .l_start = 0, + .l_len = 0, + .l_type = F_WRLCK, + }; + + fd = open("/dev/null", O_RDWR); + if (fd < 0) { + fprintf(stderr, + "Failed to open /dev/null for OFD lock probing: %s\n", + strerror(errno)); + fcntl_op_setlk = F_SETLK; + fcntl_op_getlk = F_GETLK; + return; + } + ret = fcntl(fd, F_OFD_GETLK, &fl); + close(fd); + if (!ret) { + fcntl_op_setlk = F_OFD_SETLK; + fcntl_op_getlk = F_OFD_GETLK; + } else { + fcntl_op_setlk = F_SETLK; + fcntl_op_getlk = F_GETLK; + } +#else + fcntl_op_setlk = F_SETLK; + fcntl_op_getlk = F_GETLK; +#endif + } +} + +bool qemu_has_ofd_lock(void) +{ + qemu_probe_lock_ops(); +#ifdef F_OFD_SETLK + return fcntl_op_setlk == F_OFD_SETLK; +#else + return false; +#endif +} + static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type) { int ret; @@ -158,7 +202,8 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type) .l_len = len, .l_type = fl_type, }; - ret = fcntl(fd, QEMU_SETLK, &fl); + qemu_probe_lock_ops(); + ret = fcntl(fd, fcntl_op_setlk, &fl); return ret == -1 ? -errno : 0; } @@ -181,7 +226,8 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive) .l_len = len, .l_type = exclusive ? F_WRLCK : F_RDLCK, }; - ret = fcntl(fd, QEMU_GETLK, &fl); + qemu_probe_lock_ops(); + ret = fcntl(fd, fcntl_op_getlk, &fl); if (ret == -1) { return -errno; } else { From 2b218f5dbcca5fe728b1852d161d7a21fd02b2f5 Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 11 Aug 2017 19:44:47 +0800 Subject: [PATCH 6/7] file-posix: Do runtime check for ofd lock API It is reported that on Windows Subsystem for Linux, ofd operations fail with -EINVAL. In other words, QEMU binary built with system headers that exports F_OFD_SETLK doesn't necessarily run in an environment that actually supports it: $ qemu-system-aarch64 ... -drive file=test.vhdx,if=none,id=hd0 \ -device virtio-blk-pci,drive=hd0 qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock byte 100 qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to unlock byte 100 qemu-system-aarch64: -drive file=test.vhdx,if=none,id=hd0: Failed to lock byte 100 As a matter of fact this is not WSL specific. It can happen when running a QEMU compiled against a newer glibc on an older kernel, such as in a containerized environment. Let's do a runtime check to cope with that. Reported-by: Andrew Baumann Reviewed-by: Eric Blake Signed-off-by: Fam Zheng Signed-off-by: Kevin Wolf --- block/file-posix.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/block/file-posix.c b/block/file-posix.c index f4de022ae0..cb3bfce147 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -457,22 +457,19 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, switch (locking) { case ON_OFF_AUTO_ON: s->use_lock = true; -#ifndef F_OFD_SETLK - fprintf(stderr, - "File lock requested but OFD locking syscall is unavailable, " - "falling back to POSIX file locks.\n" - "Due to the implementation, locks can be lost unexpectedly.\n"); -#endif + if (!qemu_has_ofd_lock()) { + fprintf(stderr, + "File lock requested but OFD locking syscall is " + "unavailable, falling back to POSIX file locks.\n" + "Due to the implementation, locks can be lost " + "unexpectedly.\n"); + } break; case ON_OFF_AUTO_OFF: s->use_lock = false; break; case ON_OFF_AUTO_AUTO: -#ifdef F_OFD_SETLK - s->use_lock = true; -#else - s->use_lock = false; -#endif + s->use_lock = qemu_has_ofd_lock(); break; default: abort(); From 8565c3ab537e78f3e69977ec2c609dc9417a806e Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Wed, 9 Aug 2017 18:17:57 +0300 Subject: [PATCH 7/7] qemu-iotests: fix 185 185 can sometimes produce wrong output like this: 185 2s ... - output mismatch (see 185.out.bad) --- /work/src/qemu/master/tests/qemu-iotests/185.out 2017-07-14 \ 15:14:29.520343805 +0300 +++ 185.out.bad 2017-08-07 16:51:02.231922900 +0300 @@ -37,7 +37,7 @@ {"return": {}} {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, \ "event": "SHUTDOWN", "data": {"guest": false}} -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, \ "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \ "len": 4194304, "offset": 4194304, "speed": 65536, "type": \ "mirror"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, \ "event": "BLOCK_JOB_CANCELLED", "data": {"device": "disk", \ "len": 0, "offset": 0, "speed": 65536, "type": "mirror"}} === Start backup job and exit qemu === Failures: 185 Failed 1 of 1 tests This is because, under heavy load, the quit can happen before the first iteration of the mirror request has occurred. To make sure we've had time to iterate, let's just add a sleep for 0.5 seconds before quitting. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- tests/qemu-iotests/185 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/qemu-iotests/185 b/tests/qemu-iotests/185 index 0eda371f27..f5b47e4c1a 100755 --- a/tests/qemu-iotests/185 +++ b/tests/qemu-iotests/185 @@ -156,6 +156,10 @@ _send_qemu_cmd $h \ 'speed': 65536 } }" \ "return" +# If we don't sleep here 'quit' command may be handled before +# the first mirror iteration is done +sleep 0.5 + _send_qemu_cmd $h "{ 'execute': 'quit' }" "return" wait=1 _cleanup_qemu