From edafc70c0c8510862f2f213a3acf7067113bcd08 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Mon, 22 Jun 2020 17:12:03 +0200 Subject: [PATCH 1/7] qemu-img convert: Don't pre-zero images Since commit 5a37b60a61c, qemu-img create will pre-zero the target image if it isn't already zero-initialised (most importantly, for host block devices, but also iscsi etc.), so that writing explicit zeros wouldn't be necessary later. This could speed up the operation significantly, in particular when the source image file was only sparsely populated. However, it also means that some block are written twice: Once when pre-zeroing them, and then when they are overwritten with actual data. On a full image, the pre-zeroing is wasted work because everything will be overwritten. In practice, write_zeroes typically turns out faster than writing explicit zero buffers, but slow enough that first zeroing everything and then overwriting parts can be a significant net loss. Meanwhile, qemu-img convert was rewritten in 690c7301600 and zero blocks are now written to the target using bdrv_co_pwrite_zeroes() if the target could be pre-zeroed. This way we already make use of the faster write_zeroes operation, but avoid writing any blocks twice. Remove the pre-zeroing because these days this former optimisation has actually turned into a pessimisation in the common case. Reported-by: Nir Soffer Signed-off-by: Kevin Wolf Message-Id: <20200622151203.35624-1-kwolf@redhat.com> Tested-by: Nir Soffer Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- qemu-img.c | 9 --------- 1 file changed, 9 deletions(-) diff --git a/qemu-img.c b/qemu-img.c index d7e846e607..bdb9f6aa46 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -2084,15 +2084,6 @@ static int convert_do_copy(ImgConvertState *s) s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target)); } - if (!s->has_zero_init && !s->target_has_backing && - bdrv_can_write_zeroes_with_unmap(blk_bs(s->target))) - { - ret = blk_make_zero(s->target, BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK); - if (ret == 0) { - s->has_zero_init = true; - } - } - /* Allocate buffer for copied data. For compressed images, only one cluster * can be copied at a time. */ if (s->compressed) { From 5b99bdea842d4190d2471769903cdcc1621eb493 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Fri, 19 Jun 2020 11:11:31 +0100 Subject: [PATCH 2/7] qemu-storage-daemon: remember to add qemu_object_opts The --object option is supported by qemu-storage-daemon but the qemu_object_opts QemuOptsList wasn't being added. As a result calls to qemu_find_opts("object") failed with "There is no option group 'object'". This patch fixes the object-del QMP command. Signed-off-by: Stefan Hajnoczi Message-Id: <20200619101132.2401756-2-stefanha@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- qemu-storage-daemon.c | 1 + 1 file changed, 1 insertion(+) diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c index 9e7adfe3a6..a01cbd6371 100644 --- a/qemu-storage-daemon.c +++ b/qemu-storage-daemon.c @@ -316,6 +316,7 @@ int main(int argc, char *argv[]) module_call_init(MODULE_INIT_QOM); module_call_init(MODULE_INIT_TRACE); + qemu_add_opts(&qemu_object_opts); qemu_add_opts(&qemu_trace_opts); qcrypto_init(&error_fatal); bdrv_init(); From f10802d2c9fd8bfd92c70f465da1a5992445157f Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Fri, 19 Jun 2020 11:11:32 +0100 Subject: [PATCH 3/7] qemu-storage-daemon: add missing cleanup calls Several components used by qemu-storage-daemon have cleanup functions that aren't called. Keep the "valgrind --leak-check=full" as clean as possible by invoking the necessary cleanup functions. Signed-off-by: Stefan Hajnoczi Message-Id: <20200619101132.2401756-3-stefanha@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- qemu-storage-daemon.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/qemu-storage-daemon.c b/qemu-storage-daemon.c index a01cbd6371..7e9b0e0d3f 100644 --- a/qemu-storage-daemon.c +++ b/qemu-storage-daemon.c @@ -335,5 +335,9 @@ int main(int argc, char *argv[]) main_loop_wait(false); } + monitor_cleanup(); + qemu_chr_cleanup(); + user_creatable_cleanup(); + return EXIT_SUCCESS; } From c79e243ed67683d6d06692bd7040f7394da178b0 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 23 Jun 2020 19:55:33 +0200 Subject: [PATCH 4/7] vvfat: Check that updated filenames are valid FAT allows only a restricted set of characters in file names, and for some of the illegal characters, it's actually important that we catch them: If filenames can contain '/', the guest can construct filenames containing "../" and escape from the assigned vvfat directory. The same problem could arise if ".." was ever accepted as a literal filename. Fix this by adding a check that all filenames are valid in check_directory_consistency(). Reported-by: Nathan Huckleberry Signed-off-by: Kevin Wolf Message-Id: <20200623175534.38286-2-kwolf@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/vvfat.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/block/vvfat.c b/block/vvfat.c index c65a98e3ee..62230542e5 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -520,12 +520,31 @@ static void set_begin_of_direntry(direntry_t* direntry, uint32_t begin) direntry->begin_hi = cpu_to_le16((begin >> 16) & 0xffff); } +static bool valid_filename(const unsigned char *name) +{ + unsigned char c; + if (!strcmp((const char*)name, ".") || !strcmp((const char*)name, "..")) { + return false; + } + for (; (c = *name); name++) { + if (!((c >= '0' && c <= '9') || + (c >= 'A' && c <= 'Z') || + (c >= 'a' && c <= 'z') || + c > 127 || + strchr("$%'-_@~`!(){}^#&.+,;=[]", c) != NULL)) + { + return false; + } + } + return true; +} + static uint8_t to_valid_short_char(gunichar c) { c = g_unichar_toupper(c); if ((c >= '0' && c <= '9') || (c >= 'A' && c <= 'Z') || - strchr("$%'-_@~`!(){}^#&", c) != 0) { + strchr("$%'-_@~`!(){}^#&", c) != NULL) { return c; } else { return 0; @@ -2098,6 +2117,10 @@ DLOG(fprintf(stderr, "check direntry %d:\n", i); print_direntry(direntries + i)) } lfn.checksum = 0x100; /* cannot use long name twice */ + if (!valid_filename(lfn.name)) { + fprintf(stderr, "Invalid file name\n"); + goto fail; + } if (path_len + 1 + lfn.len >= PATH_MAX) { fprintf(stderr, "Name too long: %s/%s\n", path, lfn.name); goto fail; From 3dfa23b9eff4315817c1acf59711b8414347de31 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Tue, 23 Jun 2020 19:55:34 +0200 Subject: [PATCH 5/7] vvfat: Fix array_remove_slice() array_remove_slice() calls array_roll() with array->next - 1 as the destination index. This is only correct for count == 1, otherwise we're writing past the end of the array. array->next - count would be correct. However, this is the only place ever calling array_roll(), so this rather complicated operation isn't even necessary. Fix the problem and simplify the code by replacing it with a single memmove() call. array_roll() can now be removed. Reported-by: Nathan Huckleberry Signed-off-by: Kevin Wolf Message-Id: <20200623175534.38286-3-kwolf@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block/vvfat.c | 42 +++++------------------------------------- 1 file changed, 5 insertions(+), 37 deletions(-) diff --git a/block/vvfat.c b/block/vvfat.c index 62230542e5..2eb8cbb19f 100644 --- a/block/vvfat.c +++ b/block/vvfat.c @@ -140,48 +140,16 @@ static inline void* array_insert(array_t* array,unsigned int index,unsigned int return array->pointer+index*array->item_size; } -/* this performs a "roll", so that the element which was at index_from becomes - * index_to, but the order of all other elements is preserved. */ -static inline int array_roll(array_t* array,int index_to,int index_from,int count) -{ - char* buf; - char* from; - char* to; - int is; - - if(!array || - index_to<0 || index_to>=array->next || - index_from<0 || index_from>=array->next) - return -1; - - if(index_to==index_from) - return 0; - - is=array->item_size; - from=array->pointer+index_from*is; - to=array->pointer+index_to*is; - buf=g_malloc(is*count); - memcpy(buf,from,is*count); - - if(index_to=0); assert(count > 0); assert(index + count <= array->next); - if(array_roll(array,array->next-1,index,count)) - return -1; + + memmove(array->pointer + index * array->item_size, + array->pointer + (index + count) * array->item_size, + (array->next - index - count) * array->item_size); + array->next -= count; return 0; } From 49438972b8c2e16cf1f98b1b2355926e8c633e63 Mon Sep 17 00:00:00 2001 From: Max Reitz Date: Tue, 30 Jun 2020 10:37:11 +0200 Subject: [PATCH 6/7] iotests.py: Do not wait() before communicate() Waiting on a process for which we have a pipe will stall if the process outputs more data than fits into the OS-provided buffer. We must use communicate() before wait(), and in fact, communicate() perfectly replaces wait() already. We have to drop the stderr=subprocess.STDOUT parameter from subprocess.Popen() in qemu_nbd_early_pipe(), because stderr is passed on to the child process, so if we do not drop this parameter, communicate() will hang (because the pipe is not closed). Signed-off-by: Max Reitz Message-Id: <20200630083711.40567-1-mreitz@redhat.com> Signed-off-by: Kevin Wolf --- tests/qemu-iotests/iotests.py | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py index 5ea4c4df8b..ef739dd1e3 100644 --- a/tests/qemu-iotests/iotests.py +++ b/tests/qemu-iotests/iotests.py @@ -146,11 +146,12 @@ def qemu_img_pipe(*args): stdout=subprocess.PIPE, stderr=subprocess.STDOUT, universal_newlines=True) - exitcode = subp.wait() - if exitcode < 0: + output = subp.communicate()[0] + if subp.returncode < 0: sys.stderr.write('qemu-img received signal %i: %s\n' - % (-exitcode, ' '.join(qemu_img_args + list(args)))) - return subp.communicate()[0] + % (-subp.returncode, + ' '.join(qemu_img_args + list(args)))) + return output def qemu_img_log(*args): result = qemu_img_pipe(*args) @@ -177,11 +178,11 @@ def qemu_io(*args): subp = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, universal_newlines=True) - exitcode = subp.wait() - if exitcode < 0: + output = subp.communicate()[0] + if subp.returncode < 0: sys.stderr.write('qemu-io received signal %i: %s\n' - % (-exitcode, ' '.join(args))) - return subp.communicate()[0] + % (-subp.returncode, ' '.join(args))) + return output def qemu_io_log(*args): result = qemu_io(*args) @@ -257,15 +258,14 @@ def qemu_nbd_early_pipe(*args): and its output in case of an error''' subp = subprocess.Popen(qemu_nbd_args + ['--fork'] + list(args), stdout=subprocess.PIPE, - stderr=subprocess.STDOUT, universal_newlines=True) - exitcode = subp.wait() - if exitcode < 0: + output = subp.communicate()[0] + if subp.returncode < 0: sys.stderr.write('qemu-nbd received signal %i: %s\n' % - (-exitcode, + (-subp.returncode, ' '.join(qemu_nbd_args + ['--fork'] + list(args)))) - return exitcode, subp.communicate()[0] if exitcode else '' + return subp.returncode, output if subp.returncode else '' def qemu_nbd_popen(*args): '''Run qemu-nbd in daemon mode and return the parent's exit code''' @@ -1062,11 +1062,11 @@ def qemu_pipe(*args): subp = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, universal_newlines=True) - exitcode = subp.wait() - if exitcode < 0: + output = subp.communicate()[0] + if subp.returncode < 0: sys.stderr.write('qemu received signal %i: %s\n' % - (-exitcode, ' '.join(args))) - return subp.communicate()[0] + (-subp.returncode, ' '.join(args))) + return output def supported_formats(read_only=False): '''Set 'read_only' to True to check ro-whitelist From 4f071a9460886667fde061c05b79dc786cc22e3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Wed, 24 Jun 2020 15:04:22 +0100 Subject: [PATCH 7/7] iotests: Fix 051 output after qdev_init_nofail() removal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Commit 96927c744 replaced qdev_init_nofail() call by isa_realize_and_unref() which has a different error message. Update the test output accordingly. Gitlab CI error after merging b77b5b3dc7: https://gitlab.com/qemu-project/qemu/-/jobs/597414772#L4375 Reported-by: Thomas Huth Signed-off-by: Philippe Mathieu-Daudé Signed-off-by: Alex Bennée Reviewed-by: John Snow Reviewed-by: Thomas Huth Message-Id: <20200616154949.6586-1-philmd@redhat.com> Message-Id: <20200624140446.15380-2-alex.bennee@linaro.org> Signed-off-by: Kevin Wolf --- tests/qemu-iotests/051.pc.out | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out index 0ea80d35f0..da8ad87187 100644 --- a/tests/qemu-iotests/051.pc.out +++ b/tests/qemu-iotests/051.pc.out @@ -142,7 +142,7 @@ QEMU X.Y.Z monitor - type 'help' for more information Testing: -drive if=ide QEMU X.Y.Z monitor - type 'help' for more information -(qemu) QEMU_PROG: Initialization of device ide-hd failed: Device needs media, but drive is empty +(qemu) QEMU_PROG: Device needs media, but drive is empty Testing: -drive if=virtio QEMU X.Y.Z monitor - type 'help' for more information @@ -214,7 +214,7 @@ QEMU X.Y.Z monitor - type 'help' for more information Testing: -drive file=TEST_DIR/t.qcow2,if=ide,readonly=on QEMU X.Y.Z monitor - type 'help' for more information -(qemu) QEMU_PROG: Initialization of device ide-hd failed: Block node is read-only +(qemu) QEMU_PROG: Block node is read-only Testing: -drive file=TEST_DIR/t.qcow2,if=virtio,readonly=on QEMU X.Y.Z monitor - type 'help' for more information