Re: [PATCH v2 0/7] qemu: implementation of transient disk option
[FYI pkrempa is on PTO this week so he won't review this until the next one] On a Friday in 2020, Masayoshi Mizuma wrote: This patchset tries to implement transient option for qcow2 and raw format disk. This uses the snapshot cleanup codes: https://www.redhat.com/archives/libvir-list/2020-August/msg00299.html It gets user available to set to the domain xml file like as: Any changes which the Guest does to the disk is dropped when the Guest is shutdowned. There are some limitations for transient disk option so far: - Supported disk format is qcow2 and raw - blockdev capability is required for qemu - Following features are blocked with transient disk option - blockjobs - Migration - Disk hotplug Masayoshi Mizuma (7): qemuSnapshotDiskPrepareOne: Get available even if snapdisk is NULL qemu: Introduce functions to handle transient disk qemu: Add transient disk handler to start and stop the guest qemu: Transient option gets avaiable for qcow2 and raw format disk qemu: Block blockjobs when transient disk option is enabled qemu: Block migration when transient disk option is enabled qemu: Block disk hotplug when transient disk option is enabled src/qemu/qemu_domain.c| 7 ++ src/qemu/qemu_hotplug.c | 6 ++ src/qemu/qemu_migration.c | 22 ++ src/qemu/qemu_process.c | 8 +++ src/qemu/qemu_snapshot.c | 139 +- src/qemu/qemu_snapshot.h | 8 +++ src/qemu/qemu_validate.c | 9 ++- src/util/virstoragefile.h | 2 + 8 files changed, 196 insertions(+), 5 deletions(-) -- 2.27.0 signature.asc Description: PGP signature
Re: [PATCH] block/qcow2-threads: fix qcow2_decompress
On a Monday in 2020, Vladimir Sementsov-Ogievskiy wrote: On success path we return what inflate() returns instead of 0. And it most probably works for Z_STREAM_END as it is positive, but is definitely broken for Z_BUF_ERROR. While being here, switch to errno return code, to be closer to qcow2_compress API (and usual expectations). Revert condition in if to be more positive. Drop dead initialization of ret. Cc: qemu-sta...@nongnu.org # v4.0 Fixes: 341926ab83e2b Signed-off-by: Vladimir Sementsov-Ogievskiy --- Hi! Reviewing Den's series about zstd in qcow2 support, I found an existing bug. Let's fix it. This is to be a new base of zstd series. block/qcow2-threads.c | 12 +++- 1 file changed, 7 insertions(+), 5 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 3/3] qemu-img: Deprecate use of -b without -F
On a Wednesday in 2020, Eric Blake wrote: Creating an image that requires format probing of the backing image is inherently unsafe (we've had several CVEs over the years based on probes leaking information to the guest on a subsequent boot). If our probing algorithm ever changes, or if other tools like libvirt determine a different probe result than we do, then subsequent use of that backing file under a different format will present corrupted data to the guest. Start a deprecation clock so that future qemu-img can refuse to create unsafe backing chains that would rely on probing. However, there is one time where probing is safe: if we probe raw, then it is safe to record that implicitly in the image (but we still warn, as it's better to teach the user to supply -F always than to make them guess when it is safe). iotest 114 specifically wants to create an unsafe image for later amendment rather than defaulting to our new default of recording a probed format, so it needs an update. Signed-off-by: Eric Blake --- qemu-deprecated.texi | 15 +++ block.c| 21 - qemu-img.c | 8 +++- tests/qemu-iotests/114 | 4 ++-- tests/qemu-iotests/114.out | 1 + 5 files changed, 45 insertions(+), 4 deletions(-) This seems to affect code paths that are used even outside of qemu-img, should the commit message mention it? Jano signature.asc Description: PGP signature
Re: [PATCH v2 2/3] block: Add support to warn on backing file change without format
On a Wednesday in 2020, Eric Blake wrote: For now, this is a mechanical addition; all callers pass false. But the next patch will use it to improve 'qemu-img rebase -u' when selecting a backing file with no format. Signed-off-by: Eric Blake --- include/block/block.h | 4 ++-- block.c | 13 ++--- block/qcow2.c | 2 +- block/stream.c| 2 +- blockdev.c| 3 ++- qemu-img.c| 4 ++-- 6 files changed, 18 insertions(+), 10 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 1/3] iotests: Specify explicit backing format where sensible
On a Wednesday in 2020, Eric Blake wrote:
There are many existing qcow2 images that specify a backing file but
no format. This has been the source of CVEs in the past, but has
become more prominent of a problem now that libvirt has switched to
-blockdev. With older -drive, at least the probing was always done by
qemu (so the only risk of a changed format between successive boots of
a guest was if qemu was upgraded and probed differently). But with
newer -blockdev, libvirt must specify a format; if libvirt guesses raw
where the image was formatted, this results in data corruption visible
to the guest; conversely, if libvirt guesses qcow2 where qemu was
using raw, this can result in potential security holes, so modern
libvirt instead refuses to use images without explicit backing format.
The change in libvirt to reject images without explicit backing format
has pointed out that a number of tools have been far too reliant on
probing in the past. It's time to set a better example in our own
iotests of properly setting this parameter.
iotest calls to create, rebase, convert, and amend are all impacted to
some degree. It's a bit annoying that we are inconsistent on command
line - while all of those accept -o backing_file=...,backing_fmt=...,
the shortcuts are different: create and rebase have -b and -F, convert
has -B but no -F, and amend has no shortcuts.
Signed-off-by: Eric Blake
---
[...]
Test #225 still uses -b without a format:
./check -vmdk 225
QEMU --
"/home/jtomko/work/qemu/build/tests/qemu-iotests/../../x86_64-softmmu/qemu-system-x86_64"
-nodefaults -display none -accel qtest
QEMU_IMG -- "/home/jtomko/work/qemu/build/tests/qemu-iotests/../../qemu-img"
QEMU_IO -- "/home/jtomko/work/qemu/build/tests/qemu-iotests/../../qemu-io" --cache writeback --aio threads -f vmdk
QEMU_NBD -- "/home/jtomko/work/qemu/build/tests/qemu-iotests/../../qemu-nbd"
IMGFMT-- vmdk
IMGPROTO -- file
PLATFORM -- Linux/x86_64 lpt 5.4.18-200.fc31.x86_64
TEST_DIR -- /home/jtomko/work/qemu/build/tests/qemu-iotests/scratch
SOCK_DIR -- /tmp/tmp.OQIdhLcITP
SOCKET_SCM_HELPER --
/home/jtomko/work/qemu/build/tests/qemu-iotests/socket_scm_helper
225 fail [10:02:31] [10:02:32]output mismatch
(see 225.out.bad)
--- /home/jtomko/work/qemu/tests/qemu-iotests/225.out 2018-09-07
17:21:39.633931691 +0200
+++ /home/jtomko/work/qemu/build/tests/qemu-iotests/225.out.bad 2020-02-27
10:02:32.362755677 +0100
@@ -1,6 +1,7 @@
QA output created by 225
Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
Formatting 'TEST_DIR/t.IMGFMT.not_base', fmt=IMGFMT size=1048576
+qemu-img: warning: Deprecated use of backing file without explicit backing
format (detected format of IMGFMT)
Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576
backing_file=TEST_DIR/t.IMGFMT.base
=== Testing fitting VMDK backing image ===
Failures: 225
Failed 1 of 1 iotests
diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index aa911d266a13..322e31e2cd93 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -32,8 +32,12 @@ class TestSingleDrive(iotests.QMPTestCase):
def setUp(self):
iotests.create_image(backing_img, TestSingleDrive.image_len)
-qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' %
backing_img, mid_img)
-qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' %
mid_img, test_img)
+qemu_img('create', '-f', iotests.imgfmt,
+ '-o', 'backing_file=%s' % backing_img,
+ '-F', 'raw', mid_img)
+qemu_img('create', '-f', iotests.imgfmt,
+ '-o', 'backing_file=%s' % mid_img,
+ '-F', iotests.imgfmt, test_img)
Consider not mixing shortcuts with -o options.
qemu_io('-f', 'raw', '-c', 'write -P 0x1 0 512', backing_img)
qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0x1 524288 512', mid_img)
self.vm = iotests.VM().add_drive("blkdebug::" + test_img,
With test #225 fixed:
Reviewed-by: Ján Tomko
Jano
signature.asc
Description: PGP signature
Re: [PATCH v2 1/3] iotests: Specify explicit backing format where sensible
On a Thursday in 2020, Peter Krempa wrote: On Wed, Feb 26, 2020 at 20:39:26 -0600, Eric Blake wrote: There are many existing qcow2 images that specify a backing file but no format. This has been the source of CVEs in the past, but has become more prominent of a problem now that libvirt has switched to -blockdev. With older -drive, at least the probing was always done by qemu (so the only risk of a changed format between successive boots of a guest was if qemu was upgraded and probed differently). But with newer -blockdev, libvirt must specify a format; if libvirt guesses raw where the image was formatted, this results in data corruption visible to the guest; conversely, if libvirt guesses qcow2 where qemu was using raw, this can result in potential security holes, so modern libvirt instead refuses to use images without explicit backing format. The change in libvirt to reject images without explicit backing format has pointed out that a number of tools have been far too reliant on probing in the past. It's time to set a better example in our own iotests of properly setting this parameter. iotest calls to create, rebase, convert, and amend are all impacted to some degree. It's a bit annoying that we are inconsistent on command line - while all of those accept -o backing_file=...,backing_fmt=..., the shortcuts are different: create and rebase have -b and -F, convert has -B but no -F, and amend has no shortcuts. Signed-off-by: Eric Blake --- [...] 113 files changed, 414 insertions(+), 338 deletions(-) diff --git a/tests/qemu-iotests/017 b/tests/qemu-iotests/017 index 0a4b854e6520..585512bb296b 100755 --- a/tests/qemu-iotests/017 +++ b/tests/qemu-iotests/017 @@ -66,7 +66,7 @@ echo "Creating test image with backing file" echo TEST_IMG=$TEST_IMG_SAVE -_make_test_img -b "$TEST_IMG.base" 6G +_make_test_img -b "$TEST_IMG.base" -F $IMGFMT 6G My understanding of the intricacies of the qemu-iotest suite is not good enoug to be able to review this patch. Specifically $IMGFMT in this instance is also used in the '-f' switch of qemu-img in _make_test_img and I don't know if it's expected for the backing file to share the format. IMGFMT is also used for the earlier creation of the base image and I did not see it changing in any of the tests. Jano signature.asc Description: PGP signature
Re: [PATCH 5/7] commit: Fix is_read for block_job_error_action()
On Fri, Feb 14, 2020 at 09:08:10PM +0100, Kevin Wolf wrote: block_job_error_action() needs to know if reading from the top node or writing to the base node failed so that it can set the right 'operation' in the BLOCK_JOB_ERROR QMP event. Signed-off-by: Kevin Wolf --- block/commit.c | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 1/7] qapi: Document meaning of 'ignore' BlockdevOnError for jobs
On Fri, Feb 14, 2020 at 09:08:06PM +0100, Kevin Wolf wrote: It is not obvious what 'ignore' actually means for block jobs: It could be continuing the job and returning success in the end despite the error (no block job does this). It could also mean continuing and returning failure in the end (this is what stream does). And it can mean retrying the failed request later (this is what backup, commit and mirror do). This (somewhat inconsistent) behaviour was introduced and described for stream and mirror in commit ae586d6158. backup and commit were fatal: ambiguous argument 'ae586d6158': unknown revision or path not in the working tree. introduced later and use the same model as mirror. Signed-off-by: Kevin Wolf --- qapi/block-core.json | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 3/7] commit: Fix argument order for block_job_error_action()
On Fri, Feb 14, 2020 at 09:08:08PM +0100, Kevin Wolf wrote: The block_job_error_action() error call in the commit job gives the on_err and is_read arguments in the wrong order. Fix this. (Of course, hard-coded is_read = false is wrong, too, but that's a separate problem for a separate patch.) Signed-off-by: Kevin Wolf --- block/commit.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 6/7] commit: Expose on-error option in QMP
On Fri, Feb 14, 2020 at 09:08:11PM +0100, Kevin Wolf wrote: Now that the error handling in the common block job is fixed, we can expose the on-error option in QMP instead of hard-coding it as 'report' in qmp_block_commit(). This fulfills the promise that the old comment in that function made, even if a bit later than expected: "This will be part of the QMP command, if/when the BlockdevOnError change for blkmirror makes it in". Signed-off-by: Kevin Wolf --- qapi/block-core.json | 4 blockdev.c | 8 2 files changed, 8 insertions(+), 4 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 4/7] commit: Inline commit_populate()
On Fri, Feb 14, 2020 at 09:08:09PM +0100, Kevin Wolf wrote: commit_populate() is a very short function and only called in a single place. Its return value doesn't tell us whether an error happened while reading or writing, which would be necessary for sending the right data in the BLOCK_JOB_ERROR QMP event. Signed-off-by: Kevin Wolf --- block/commit.c | 28 ++-- 1 file changed, 6 insertions(+), 22 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 2/7] commit: Remove unused bytes_written
On Fri, Feb 14, 2020 at 09:08:07PM +0100, Kevin Wolf wrote: The bytes_written variable is only ever written to, it serves no purpose. This has actually been the case since the commit job was first introduced in commit 747ff602636. Signed-off-by: Kevin Wolf --- block/commit.c | 2 -- 1 file changed, 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH 1/3] block/qcow2-bitmap: Remove unneeded variable assignment
On Sat, Feb 15, 2020 at 05:15:55PM +0100, Philippe Mathieu-Daudé wrote: Fix warning reported by Clang static code analyzer: CC block/qcow2-bitmap.o block/qcow2-bitmap.c:650:5: warning: Value stored to 'ret' is never read ret = -EINVAL; ^ ~~~ Reported-by: Clang Static Analyzer Signed-off-by: Philippe Mathieu-Daudé --- block/qcow2-bitmap.c | 1 - 1 file changed, 1 deletion(-) Reviewed-by: Ján Tomko Unused since its introduction in 88ddffae8fc1e30cc907c2dbb989b7eba9e62319 Jano signature.asc Description: PGP signature
Re: [PATCH] nbd-client: Support leading / in NBD URI
On Tue, Feb 11, 2020 at 08:31:01PM -0600, Eric Blake wrote: The NBD URI specification [1] states that only one leading slash at the beginning of the URI path component is stripped, not all such slashes. This becomes important to a patch I just proposed to nbdkit [2], which would allow the exportname to select a file embedded within an ext2 image: ext2fs demands an absolute pathname beginning with '/', and because qemu was inadvertantly stripping it, my nbdkit patch had to work around the behavior. [1] https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md [2] https://www.redhat.com/archives/libguestfs/2020-February/msg00109.html Note that the qemu bug only affects handling of URIs such as nbd://host:port//abs/path (where '/abs/path' should be the export name); it is still possible to use --image-opts and pass the desired export name with a leading slash directly through JSON even without this patch. Signed-off-by: Eric Blake --- block/nbd.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v2 2/2] qemu-nbd: Removed deprecated --partition option
On Thu, Jan 23, 2020 at 10:46:50AM -0600, Eric Blake wrote: The option was deprecated in 4.0.0 (commit 0ae2d546); it's now been long enough with no complaints to follow through with that process. Signed-off-by: Eric Blake --- docs/interop/qemu-nbd.rst | 15 ++--- qemu-deprecated.texi | 49 ++ qemu-nbd.c| 133 +- 3 files changed, 24 insertions(+), 173 deletions(-) Reviewed-by: Ján Tomko Jano signature.asc Description: PGP signature
Re: [PATCH v3 00/13] RFC: [for 5.0]: HMP monitor handlers cleanups
On Mon, Jan 27, 2020 at 04:01:31PM -0500, John Snow wrote: On 1/27/20 3:43 PM, Peter Krempa wrote: On Mon, Jan 27, 2020 at 14:39:02 -0500, John Snow wrote: On 1/27/20 5:36 AM, Maxim Levitsky wrote: This patch series is bunch of cleanups to the hmp monitor code. This series only touched blockdev related hmp handlers. No functional changes expected other that light error message changes by the last patch. This was inspired by this bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1719169 Basically some users still parse hmp error messages, and they would like to have them prefixed with 'Error:' [...] The bug was reported at the time when libvirt didn't use blockdev yet, but at this point it's pointless from our side. Just for the record, this bug was the motivation for the prefix request: https://bugzilla.redhat.com/show_bug.cgi?id=1718255 This wouldn't even fix the scenario when old (pre-5.10) libvirt would use new qemu because the drive-add handler never checked the error prefix. And running older libvirt with newer QEMU is not something we care about. Jano [1] https://libvirt.org/git/?p=libvirt.git;a=blob;f=src/qemu/qemu_monitor_text.c;h=9135a11f0a3aae718c86bb199112fba8d16d4d80;hb=HEAD Thank you for the report from libvirtville :) --js signature.asc Description: PGP signature
