From a1d4e38a8b01a6699355c31867d524f8d4cd480e Mon Sep 17 00:00:00 2001 From: Ashijeet Acharya Date: Wed, 2 Nov 2016 16:10:03 +0530 Subject: [PATCH 01/12] block/nbd: Fix the leaked visitor This patch frees the leaked visitor in nbd_refresh_filename() and uses visit_free() to fix it. The leak was introduced by the commit 491d6c7. Signed-off-by: Ashijeet Acharya Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/nbd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/nbd.c b/block/nbd.c index 9cff8396f9..35f24be069 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -536,6 +536,7 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options) ov = qobject_output_visitor_new(&saddr_qdict); visit_type_SocketAddress(ov, NULL, &s->saddr, &error_abort); visit_complete(ov, &saddr_qdict); + visit_free(ov); assert(qobject_type(saddr_qdict) == QTYPE_QDICT); qdict_put_obj(opts, "server", saddr_qdict); From 9a80832abff65044a18405f4eb22c94eba341ff6 Mon Sep 17 00:00:00 2001 From: Ashijeet Acharya Date: Wed, 2 Nov 2016 16:23:46 +0530 Subject: [PATCH 02/12] block/ssh: Code cleanup for unused parameter This patch drops the unused parameter "BDRVSSHState" being passed into the ssh_config() function and does code cleanup. The unused parameter was introduced by the commit c322712. Signed-off-by: Ashijeet Acharya Signed-off-by: Kevin Wolf --- block/ssh.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/block/ssh.c b/block/ssh.c index ca071c569c..15ed2818c5 100644 --- a/block/ssh.c +++ b/block/ssh.c @@ -582,8 +582,7 @@ static bool ssh_process_legacy_socket_options(QDict *output_opts, return true; } -static InetSocketAddress *ssh_config(BDRVSSHState *s, QDict *options, - Error **errp) +static InetSocketAddress *ssh_config(QDict *options, Error **errp) { InetSocketAddress *inet = NULL; QDict *addr = NULL; @@ -661,7 +660,7 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options, } /* Pop the config into our state object, Exit if invalid */ - s->inet = ssh_config(s, options, errp); + s->inet = ssh_config(options, errp); if (!s->inet) { ret = -EINVAL; goto err; From 11d6fbe05fd67610a7735e5350e4299f93bf7655 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Fri, 4 Nov 2016 15:44:43 +0200 Subject: [PATCH 03/12] hmp: Make block_stream set an explicit job ID A job ID is always required in order to create a block job on a non-root node. The default ID (obtained with bdrv_get_device_name()) is otherwise empty in this scenario and the job cannot be created. The HMP block_stream command doesn't set a job ID and therefore it doesn't allow streaming to intermediate nodes. One solution is to add an extra parameter to set a job ID. The other solution is to simply use the node name passed to block_stream as job ID. This won't work if it's automatically generated (because it contains a '#') but is otherwise simple enough for all other cases. This way 'block_stream node3' will create a job with the ID 'node3' and the good old 'block_stream virtio0' will keep the previous behaviour and use 'virtio0' for the job ID. Signed-off-by: Alberto Garcia Signed-off-by: Kevin Wolf --- hmp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hmp.c b/hmp.c index b5e3f54534..819166d423 100644 --- a/hmp.c +++ b/hmp.c @@ -1570,7 +1570,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict) const char *base = qdict_get_try_str(qdict, "base"); int64_t speed = qdict_get_try_int(qdict, "speed", 0); - qmp_block_stream(false, NULL, device, base != NULL, base, false, NULL, + qmp_block_stream(true, device, device, base != NULL, base, false, NULL, false, NULL, qdict_haskey(qdict, "speed"), speed, true, BLOCKDEV_ON_ERROR_REPORT, &error); From 40332872fec584d2557ed2c3f48d55d15d95eddb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Golembiovsk=C3=BD?= Date: Thu, 3 Nov 2016 14:47:48 +0100 Subject: [PATCH 04/12] raw_bsd: move check to prevent overflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When only offset is specified but no size and the offset is greater than the real size of the containing device an overflow occurs when parsing the options. This overflow is harmless because we do check for this exact situation little bit later, but it leads to an error message with weird values. It is better to do the check is sooner and prevent the overflow. Signed-off-by: Tomáš Golembiovský Signed-off-by: Kevin Wolf --- block/raw_bsd.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/block/raw_bsd.c b/block/raw_bsd.c index 7c9bebb507..cf7a5606ed 100644 --- a/block/raw_bsd.c +++ b/block/raw_bsd.c @@ -91,6 +91,14 @@ static int raw_read_options(QDict *options, BlockDriverState *bs, } s->offset = qemu_opt_get_size(opts, "offset", 0); + if (s->offset > real_size) { + error_setg(errp, "Offset (%" PRIu64 ") cannot be greater than " + "size of the containing file (%" PRId64 ")", + s->offset, real_size); + ret = -EINVAL; + goto end; + } + if (qemu_opt_find(opts, "size") != NULL) { s->size = qemu_opt_get_size(opts, "size", 0); s->has_size = true; @@ -100,7 +108,7 @@ static int raw_read_options(QDict *options, BlockDriverState *bs, } /* Check size and offset */ - if (real_size < s->offset || (real_size - s->offset) < s->size) { + if ((real_size - s->offset) < s->size) { error_setg(errp, "The sum of offset (%" PRIu64 ") and size " "(%" PRIu64 ") has to be smaller or equal to the " " actual size of the containing file (%" PRId64 ")", From 80a15e3e2eed96926d886693663503985c9a98bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Golembiovsk=C3=BD?= Date: Thu, 3 Nov 2016 14:47:49 +0100 Subject: [PATCH 05/12] raw_bsd: don't check size alignment when only offset is set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We make sure that the size is aligned to sector length to prevent any round ups. Otherwise we could end up reading/writing data outside the area specified by user. This is only needed when user supplies the size option to avoid any surprises. It is not necessary when only offset is set. More over, the check made it difficult to use the offset option without size option. The check puts unneeded restriction on the offset which had to be aligned too. Because bdrv_getlength() returns aligned value having unaligned offset would make the check fail. Signed-off-by: Tomáš Golembiovský Signed-off-by: Kevin Wolf --- block/raw_bsd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block/raw_bsd.c b/block/raw_bsd.c index cf7a5606ed..8a5b9b0424 100644 --- a/block/raw_bsd.c +++ b/block/raw_bsd.c @@ -119,7 +119,7 @@ static int raw_read_options(QDict *options, BlockDriverState *bs, /* Make sure size is multiple of BDRV_SECTOR_SIZE to prevent rounding * up and leaking out of the specified area. */ - if (!QEMU_IS_ALIGNED(s->size, BDRV_SECTOR_SIZE)) { + if (s->has_size && !QEMU_IS_ALIGNED(s->size, BDRV_SECTOR_SIZE)) { error_setg(errp, "Specified size is not multiple of %llu", BDRV_SECTOR_SIZE); ret = -EINVAL; From 9dd76f82d9a8d060c217d543304a350ef227e997 Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Fri, 4 Nov 2016 17:00:48 +0200 Subject: [PATCH 06/12] qcow2: Remove stale FIXME comment It was from the time when none of the global functions had a qcow2_ prefix. Signed-off-by: Alberto Garcia Signed-off-by: Kevin Wolf --- block/qcow2.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/block/qcow2.h b/block/qcow2.h index 92203a8b8c..182341483a 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -473,8 +473,6 @@ static inline uint64_t refcount_diff(uint64_t r1, uint64_t r2) return r1 > r2 ? r1 - r2 : r2 - r1; } -// FIXME Need qcow2_ prefix to global functions - /* qcow2.c functions */ int qcow2_backing_read1(BlockDriverState *bs, QEMUIOVector *qiov, int64_t sector_num, int nb_sectors); From 07555ba6f303d4be8af538c3a66cc46ccb2e5751 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 7 Nov 2016 10:27:51 +0100 Subject: [PATCH 07/12] nfs: Fix memory leak in nfs_file_create() The leak was introduced in commit 94d6a7a7. Signed-off-by: Kevin Wolf Reviewed-by: Stefan Hajnoczi --- block/nfs.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/nfs.c b/block/nfs.c index 55c4e0b073..d08278323f 100644 --- a/block/nfs.c +++ b/block/nfs.c @@ -676,6 +676,7 @@ static int nfs_file_create(const char *url, QemuOpts *opts, Error **errp) ret = nfs_ftruncate(client->context, client->fh, total_size); nfs_client_close(client); out: + QDECREF(options); g_free(client); return ret; } From ceff5bd79cdf94c650902fe9a20d316dcbfa1cc9 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Wed, 12 Oct 2016 22:49:05 +0200 Subject: [PATCH 08/12] block: Fix bdrv_iterate_format() sorting bdrv_iterate_format() did not actually sort the formats by name but by "pointer interpreted as string". That is probably not what we intended to do, so fix it (by changing qsort_strcmp() so it matches the example from qsort()'s manual page). Signed-off-by: Max Reitz Message-id: 20161012204907.25941-2-mreitz@redhat.com Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block.c b/block.c index c19c6c6d42..67c70c1010 100644 --- a/block.c +++ b/block.c @@ -2796,7 +2796,7 @@ const char *bdrv_get_format_name(BlockDriverState *bs) static int qsort_strcmp(const void *a, const void *b) { - return strcmp(a, b); + return strcmp(*(char *const *)a, *(char *const *)b); } void bdrv_iterate_format(void (*it)(void *opaque, const char *name), From eb0df69f50bd64454bcb9457e875cd52f00dced8 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Wed, 12 Oct 2016 22:49:06 +0200 Subject: [PATCH 09/12] block: Emit modules in bdrv_iterate_format() Some block drivers may not be loaded yet, but qemu supports them nonetheless. bdrv_iterate_format() should report them, too. Signed-off-by: Max Reitz Message-id: 20161012204907.25941-3-mreitz@redhat.com Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- block.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/block.c b/block.c index 67c70c1010..39ddea3411 100644 --- a/block.c +++ b/block.c @@ -2822,6 +2822,24 @@ void bdrv_iterate_format(void (*it)(void *opaque, const char *name), } } + for (i = 0; i < (int)ARRAY_SIZE(block_driver_modules); i++) { + const char *format_name = block_driver_modules[i].format_name; + + if (format_name) { + bool found = false; + int j = count; + + while (formats && j && !found) { + found = !strcmp(formats[--j], format_name); + } + + if (!found) { + formats = g_renew(const char *, formats, count + 1); + formats[count++] = format_name; + } + } + } + qsort(formats, count, sizeof(formats[0]), qsort_strcmp); for (i = 0; i < count; i++) { From eaed090735ac0dbcaad6d39074da6607f2f81b8a Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Wed, 12 Oct 2016 22:49:07 +0200 Subject: [PATCH 10/12] iotests: Skip test 162 if there is no SSH support Signed-off-by: Max Reitz Message-id: 20161012204907.25941-4-mreitz@redhat.com Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- tests/qemu-iotests/162 | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/qemu-iotests/162 b/tests/qemu-iotests/162 index f8eecb325b..cad2bd70ab 100755 --- a/tests/qemu-iotests/162 +++ b/tests/qemu-iotests/162 @@ -35,6 +35,9 @@ status=1 # failure is the default! _supported_fmt generic _supported_os Linux +test_ssh=$($QEMU_IMG --help | grep '^Supported formats:.* ssh\( \|$\)') +[ "$test_ssh" = "" ] && _notrun "ssh support required" + echo echo '=== NBD ===' # NBD expects all of its arguments to be strings From 3bb8ef4b7aafe9de0b93a4d6ba2843b11acbefe4 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Mon, 17 Oct 2016 20:39:17 +0200 Subject: [PATCH 11/12] iotests: Always use -machine accel=qtest Currently, we only use -machine accel=qtest when qemu is invoked through the common.qemu functions. However, we always want to use it, so move it from common.qemu directly into QEMU_OPTIONS. Signed-off-by: Max Reitz Message-id: 20161017183917.8837-1-mreitz@redhat.com Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- tests/qemu-iotests/common | 2 +- tests/qemu-iotests/common.qemu | 12 +++++------- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/tests/qemu-iotests/common b/tests/qemu-iotests/common index d60ea2ce3c..b6274bee0a 100644 --- a/tests/qemu-iotests/common +++ b/tests/qemu-iotests/common @@ -51,7 +51,7 @@ export IMGOPTS="" export CACHEMODE="writeback" export QEMU_IO_OPTIONS="" export CACHEMODE_IS_DEFAULT=true -export QEMU_OPTIONS="-nodefaults" +export QEMU_OPTIONS="-nodefaults -machine accel=qtest" export VALGRIND_QEMU= export IMGKEYSECRET= export IMGOPTSSYNTAX=false diff --git a/tests/qemu-iotests/common.qemu b/tests/qemu-iotests/common.qemu index 2548a8700b..e657361790 100644 --- a/tests/qemu-iotests/common.qemu +++ b/tests/qemu-iotests/common.qemu @@ -155,15 +155,13 @@ function _launch_qemu() if [ -z "$keep_stderr" ]; then QEMU_NEED_PID='y'\ - ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \ - >"${fifo_out}" \ - 2>&1 \ - <"${fifo_in}" & + ${QEMU} -nographic -serial none ${comm} "${@}" >"${fifo_out}" \ + 2>&1 \ + <"${fifo_in}" & elif [ "$keep_stderr" = "y" ]; then QEMU_NEED_PID='y'\ - ${QEMU} -nographic -serial none ${comm} -machine accel=qtest "${@}" \ - >"${fifo_out}" \ - <"${fifo_in}" & + ${QEMU} -nographic -serial none ${comm} "${@}" >"${fifo_out}" \ + <"${fifo_in}" & else exit 1 fi From 4e6d13c9839915ba90d7d561c54cc10416c8c4bd Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Thu, 27 Oct 2016 18:45:17 +0800 Subject: [PATCH 12/12] raw-posix: Rename 'raw_s' to 'rs' It is too confusing because it sounds like a BDRVRawState variable. Suggested-by: Max Reitz Signed-off-by: Fam Zheng Message-id: 1477565117-17230-1-git-send-email-famz@redhat.com Reviewed-by: Eric Blake Signed-off-by: Max Reitz --- block/raw-posix.c | 56 +++++++++++++++++++++++------------------------ 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/block/raw-posix.c b/block/raw-posix.c index 247e47b88f..28b47d977b 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -542,7 +542,7 @@ static int raw_reopen_prepare(BDRVReopenState *state, BlockReopenQueue *queue, Error **errp) { BDRVRawState *s; - BDRVRawReopenState *raw_s; + BDRVRawReopenState *rs; int ret = 0; Error *local_err = NULL; @@ -552,15 +552,15 @@ static int raw_reopen_prepare(BDRVReopenState *state, s = state->bs->opaque; state->opaque = g_new0(BDRVRawReopenState, 1); - raw_s = state->opaque; + rs = state->opaque; if (s->type == FTYPE_CD) { - raw_s->open_flags |= O_NONBLOCK; + rs->open_flags |= O_NONBLOCK; } - raw_parse_flags(state->flags, &raw_s->open_flags); + raw_parse_flags(state->flags, &rs->open_flags); - raw_s->fd = -1; + rs->fd = -1; int fcntl_flags = O_APPEND | O_NONBLOCK; #ifdef O_NOATIME @@ -569,35 +569,35 @@ static int raw_reopen_prepare(BDRVReopenState *state, #ifdef O_ASYNC /* Not all operating systems have O_ASYNC, and those that don't - * will not let us track the state into raw_s->open_flags (typically + * will not let us track the state into rs->open_flags (typically * you achieve the same effect with an ioctl, for example I_SETSIG * on Solaris). But we do not use O_ASYNC, so that's fine. */ assert((s->open_flags & O_ASYNC) == 0); #endif - if ((raw_s->open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) { + if ((rs->open_flags & ~fcntl_flags) == (s->open_flags & ~fcntl_flags)) { /* dup the original fd */ - raw_s->fd = qemu_dup(s->fd); - if (raw_s->fd >= 0) { - ret = fcntl_setfl(raw_s->fd, raw_s->open_flags); + rs->fd = qemu_dup(s->fd); + if (rs->fd >= 0) { + ret = fcntl_setfl(rs->fd, rs->open_flags); if (ret) { - qemu_close(raw_s->fd); - raw_s->fd = -1; + qemu_close(rs->fd); + rs->fd = -1; } } } /* If we cannot use fcntl, or fcntl failed, fall back to qemu_open() */ - if (raw_s->fd == -1) { + if (rs->fd == -1) { const char *normalized_filename = state->bs->filename; ret = raw_normalize_devicepath(&normalized_filename); if (ret < 0) { error_setg_errno(errp, -ret, "Could not normalize device path"); } else { - assert(!(raw_s->open_flags & O_CREAT)); - raw_s->fd = qemu_open(normalized_filename, raw_s->open_flags); - if (raw_s->fd == -1) { + assert(!(rs->open_flags & O_CREAT)); + rs->fd = qemu_open(normalized_filename, rs->open_flags); + if (rs->fd == -1) { error_setg_errno(errp, errno, "Could not reopen file"); ret = -1; } @@ -606,11 +606,11 @@ static int raw_reopen_prepare(BDRVReopenState *state, /* Fail already reopen_prepare() if we can't get a working O_DIRECT * alignment with the new fd. */ - if (raw_s->fd != -1) { - raw_probe_alignment(state->bs, raw_s->fd, &local_err); + if (rs->fd != -1) { + raw_probe_alignment(state->bs, rs->fd, &local_err); if (local_err) { - qemu_close(raw_s->fd); - raw_s->fd = -1; + qemu_close(rs->fd); + rs->fd = -1; error_propagate(errp, local_err); ret = -EINVAL; } @@ -621,13 +621,13 @@ static int raw_reopen_prepare(BDRVReopenState *state, static void raw_reopen_commit(BDRVReopenState *state) { - BDRVRawReopenState *raw_s = state->opaque; + BDRVRawReopenState *rs = state->opaque; BDRVRawState *s = state->bs->opaque; - s->open_flags = raw_s->open_flags; + s->open_flags = rs->open_flags; qemu_close(s->fd); - s->fd = raw_s->fd; + s->fd = rs->fd; g_free(state->opaque); state->opaque = NULL; @@ -636,16 +636,16 @@ static void raw_reopen_commit(BDRVReopenState *state) static void raw_reopen_abort(BDRVReopenState *state) { - BDRVRawReopenState *raw_s = state->opaque; + BDRVRawReopenState *rs = state->opaque; /* nothing to do if NULL, we didn't get far enough */ - if (raw_s == NULL) { + if (rs == NULL) { return; } - if (raw_s->fd >= 0) { - qemu_close(raw_s->fd); - raw_s->fd = -1; + if (rs->fd >= 0) { + qemu_close(rs->fd); + rs->fd = -1; } g_free(state->opaque); state->opaque = NULL;