Re: [Qemu-block] [Qemu-devel] [PATCH 0/7] jobs: remove job_defer_to_main_loop
Hi, This series failed docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. Type: series Message-id: 20180817190457.8292-1-js...@redhat.com Subject: [Qemu-devel] [PATCH 0/7] jobs: remove job_defer_to_main_loop === TEST SCRIPT BEGIN === #!/bin/bash time make docker-test-quick@centos7 SHOW_ENV=1 J=8 === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 8a7a269a6d jobs: remove job_defer_to_main_loop f0b09a2775 jobs: utilize job_exit shim c7e2df3361 block/mirror: utilize job_exit shim d45d9fd099 block/commit: utilize job_exit shim d5b28e614b jobs: add exit shim 6fb5dac7b5 jobs: canonize Error object c6bee15d43 jobs: change start callback to run callback === OUTPUT BEGIN === BUILD centos7 make[1]: Entering directory '/var/tmp/patchew-tester-tmp-jxjt0g36/src' GEN /var/tmp/patchew-tester-tmp-jxjt0g36/src/docker-src.2018-09-03-22.07.23.2969/qemu.tar Cloning into '/var/tmp/patchew-tester-tmp-jxjt0g36/src/docker-src.2018-09-03-22.07.23.2969/qemu.tar.vroot'... done. Checking out files: 45% (2903/6332) Checking out files: 46% (2913/6332) Checking out files: 47% (2977/6332) Checking out files: 48% (3040/6332) Checking out files: 49% (3103/6332) Checking out files: 50% (3166/6332) Checking out files: 51% (3230/6332) Checking out files: 52% (3293/6332) Checking out files: 53% (3356/6332) Checking out files: 54% (3420/6332) Checking out files: 55% (3483/6332) Checking out files: 56% (3546/6332) Checking out files: 57% (3610/6332) Checking out files: 58% (3673/6332) Checking out files: 59% (3736/6332) Checking out files: 60% (3800/6332) Checking out files: 61% (3863/6332) Checking out files: 62% (3926/6332) Checking out files: 63% (3990/6332) Checking out files: 64% (4053/6332) Checking out files: 65% (4116/6332) Checking out files: 66% (4180/6332) Checking out files: 67% (4243/6332) Checking out files: 68% (4306/6332) Checking out files: 69% (4370/6332) Checking out files: 70% (4433/6332) Checking out files: 71% (4496/6332) Checking out files: 72% (4560/6332) Checking out files: 73% (4623/6332) Checking out files: 74% (4686/6332) Checking out files: 75% (4749/6332) Checking out files: 76% (4813/6332) Checking out files: 77% (4876/6332) Checking out files: 78% (4939/6332) Checking out files: 79% (5003/6332) Checking out files: 80% (5066/6332) Checking out files: 81% (5129/6332) Checking out files: 82% (5193/6332) Checking out files: 82% (5210/6332) Checking out files: 83% (5256/6332) Checking out files: 84% (5319/6332) Checking out files: 85% (5383/6332) Checking out files: 86% (5446/6332) Checking out files: 87% (5509/6332) Checking out files: 88% (5573/6332) Checking out files: 89% (5636/6332) Checking out files: 90% (5699/6332) Checking out files: 91% (5763/6332) Checking out files: 92% (5826/6332) Checking out files: 93% (5889/6332) Checking out files: 94% (5953/6332) Checking out files: 95% (6016/6332) Checking out files: 96% (6079/6332) Checking out files: 97% (6143/6332) Checking out files: 98% (6206/6332) Checking out files: 99% (6269/6332) Checking out files: 100% (6332/6332) Checking out files: 100% (6332/6332), done. Your branch is up-to-date with 'origin/test'. Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-jxjt0g36/src/docker-src.2018-09-03-22.07.23.2969/qemu.tar.vroot/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into '/var/tmp/patchew-tester-tmp-jxjt0g36/src/docker-src.2018-09-03-22.07.23.2969/qemu.tar.vroot/ui/keycodemapdb'... Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce' COPYRUNNER RUN test-quick in qemu:centos7 Packages installed: SDL-devel-1.2.15-14.el7.x86_64 bison-3.0.4-1.el7.x86_64 bzip2-1.0.6-13.el7.x86_64 bzip2-devel-1.0.6-13.el7.x86_64 ccache-3.3.4-1.el7.x86_64 csnappy-devel-0-6.20150729gitd7bc683.el7.x86_64 flex-2.5.37-3.el7.x86_64 gcc-4.8.5-28.el7_5.1.x86_64 gettext-0.19.8.1-2.el7.x86_64 git-1.8.3.1-14.el7_5.x86_64 glib2-devel-2.54.2-2.el7.x86_64 libaio-devel-0.3.109-13.el7.x86_64 libepoxy-devel-1.3.1-2.el7_5.x86_64 libfdt-devel-1.4.6-1.el7.x86_64 lzo-devel-2.06-8.el7.x86_64 make-3.82-23.el7.x86_64 mesa-libEGL-devel-17.2.3-8.20171019.el7.x86_64 mesa-libgbm-devel-17.2.3-8.20171019.el7.x86_64 nettle-devel-2.7.1-8.el7.x86_64 package g++ is not installed package librdmacm-devel is not installed pixman-devel-0.34.0-1.el7.x86_64 spice-glib-devel-0.34-3.el7_5.1.x86_64 spice-server-devel-0.14.0-2.el7_5.4.x86_64
Re: [Qemu-block] [Qemu-devel] [PATCH 0/7] jobs: remove job_defer_to_main_loop
Hi, This series failed docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. Type: series Message-id: 20180817190457.8292-1-js...@redhat.com Subject: [Qemu-devel] [PATCH 0/7] jobs: remove job_defer_to_main_loop === TEST SCRIPT BEGIN === #!/bin/bash time make docker-test-mingw@fedora SHOW_ENV=1 J=8 === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 Switched to a new branch 'test' 8a7a269a6d jobs: remove job_defer_to_main_loop f0b09a2775 jobs: utilize job_exit shim c7e2df3361 block/mirror: utilize job_exit shim d45d9fd099 block/commit: utilize job_exit shim d5b28e614b jobs: add exit shim 6fb5dac7b5 jobs: canonize Error object c6bee15d43 jobs: change start callback to run callback === OUTPUT BEGIN === BUILD fedora make[1]: Entering directory '/var/tmp/patchew-tester-tmp-n53ljno8/src' GEN /var/tmp/patchew-tester-tmp-n53ljno8/src/docker-src.2018-09-03-22.04.33.30916/qemu.tar Cloning into '/var/tmp/patchew-tester-tmp-n53ljno8/src/docker-src.2018-09-03-22.04.33.30916/qemu.tar.vroot'... done. Your branch is up-to-date with 'origin/test'. Submodule 'dtc' (git://git.qemu-project.org/dtc.git) registered for path 'dtc' Cloning into '/var/tmp/patchew-tester-tmp-n53ljno8/src/docker-src.2018-09-03-22.04.33.30916/qemu.tar.vroot/dtc'... Submodule path 'dtc': checked out 'e54388015af1fb4bf04d0bca99caba1074d9cc42' Submodule 'ui/keycodemapdb' (git://git.qemu.org/keycodemapdb.git) registered for path 'ui/keycodemapdb' Cloning into '/var/tmp/patchew-tester-tmp-n53ljno8/src/docker-src.2018-09-03-22.04.33.30916/qemu.tar.vroot/ui/keycodemapdb'... Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce' COPYRUNNER RUN test-mingw in qemu:fedora Packages installed: SDL2-devel-2.0.8-5.fc28.x86_64 bc-1.07.1-5.fc28.x86_64 bison-3.0.4-9.fc28.x86_64 bluez-libs-devel-5.49-3.fc28.x86_64 brlapi-devel-0.6.7-12.fc28.x86_64 bzip2-1.0.6-26.fc28.x86_64 bzip2-devel-1.0.6-26.fc28.x86_64 ccache-3.4.2-2.fc28.x86_64 clang-6.0.0-5.fc28.x86_64 device-mapper-multipath-devel-0.7.4-2.git07e7bd5.fc28.x86_64 findutils-4.6.0-19.fc28.x86_64 flex-2.6.1-7.fc28.x86_64 gcc-8.1.1-1.fc28.x86_64 gcc-c++-8.1.1-1.fc28.x86_64 gettext-0.19.8.1-14.fc28.x86_64 git-2.17.1-2.fc28.x86_64 glib2-devel-2.56.1-3.fc28.x86_64 glusterfs-api-devel-4.0.2-1.fc28.x86_64 gnutls-devel-3.6.2-1.fc28.x86_64 gtk3-devel-3.22.30-1.fc28.x86_64 hostname-3.20-3.fc28.x86_64 libaio-devel-0.3.110-11.fc28.x86_64 libasan-8.1.1-1.fc28.x86_64 libattr-devel-2.4.47-23.fc28.x86_64 libcap-devel-2.25-9.fc28.x86_64 libcap-ng-devel-0.7.9-1.fc28.x86_64 libcurl-devel-7.59.0-3.fc28.x86_64 libfdt-devel-1.4.6-4.fc28.x86_64 libpng-devel-1.6.34-3.fc28.x86_64 librbd-devel-12.2.5-1.fc28.x86_64 libssh2-devel-1.8.0-7.fc28.x86_64 libubsan-8.1.1-1.fc28.x86_64 libusbx-devel-1.0.21-6.fc28.x86_64 libxml2-devel-2.9.7-4.fc28.x86_64 llvm-6.0.0-11.fc28.x86_64 lzo-devel-2.08-12.fc28.x86_64 make-4.2.1-6.fc28.x86_64 mingw32-SDL2-2.0.5-3.fc27.noarch mingw32-bzip2-1.0.6-9.fc27.noarch mingw32-curl-7.57.0-1.fc28.noarch mingw32-glib2-2.54.1-1.fc28.noarch mingw32-gmp-6.1.2-2.fc27.noarch mingw32-gnutls-3.5.13-2.fc27.noarch mingw32-gtk3-3.22.16-1.fc27.noarch mingw32-libjpeg-turbo-1.5.1-3.fc27.noarch mingw32-libpng-1.6.29-2.fc27.noarch mingw32-libssh2-1.8.0-3.fc27.noarch mingw32-libtasn1-4.13-1.fc28.noarch mingw32-nettle-3.3-3.fc27.noarch mingw32-pixman-0.34.0-3.fc27.noarch mingw32-pkg-config-0.28-9.fc27.x86_64 mingw64-SDL2-2.0.5-3.fc27.noarch mingw64-bzip2-1.0.6-9.fc27.noarch mingw64-curl-7.57.0-1.fc28.noarch mingw64-glib2-2.54.1-1.fc28.noarch mingw64-gmp-6.1.2-2.fc27.noarch mingw64-gnutls-3.5.13-2.fc27.noarch mingw64-gtk3-3.22.16-1.fc27.noarch mingw64-libjpeg-turbo-1.5.1-3.fc27.noarch mingw64-libpng-1.6.29-2.fc27.noarch mingw64-libssh2-1.8.0-3.fc27.noarch mingw64-libtasn1-4.13-1.fc28.noarch mingw64-nettle-3.3-3.fc27.noarch mingw64-pixman-0.34.0-3.fc27.noarch mingw64-pkg-config-0.28-9.fc27.x86_64 ncurses-devel-6.1-5.20180224.fc28.x86_64 nettle-devel-3.4-2.fc28.x86_64 nss-devel-3.36.1-1.1.fc28.x86_64 numactl-devel-2.0.11-8.fc28.x86_64 package PyYAML is not installed package libjpeg-devel is not installed perl-5.26.2-411.fc28.x86_64 pixman-devel-0.34.0-8.fc28.x86_64 python3-3.6.5-1.fc28.x86_64 snappy-devel-1.1.7-5.fc28.x86_64 sparse-0.5.2-1.fc28.x86_64 spice-server-devel-0.14.0-4.fc28.x86_64 systemtap-sdt-devel-3.2-11.fc28.x86_64 tar-1.30-3.fc28.x86_64 usbredir-devel-0.7.1-7.fc28.x86_64 virglrenderer-devel-0.6.0-4.20170210git76b3da97b.fc28.x86_64 vte3-devel-0.36.5-6.fc28.x86_64 which-2.21-8.fc28.x86_64 xen-devel-4.10.1-3.fc28.x86_64 zlib-devel-1.2.11-8.fc28.x86_64 Environment variables: TARGET_LIST= PACKAGES=ccache gettext git tar PyYAML sparse flex bison python3 bzip2 hostname gcc gcc-c++ llvm clang make perl which bc findutils glib2-devel libaio-devel pixman-devel zlib-devel libfdt-devel libasan libubsan bluez-libs-devel brlapi-devel
Re: [Qemu-block] [Qemu-stable] [PATCH v2] job: Fix nested aio_poll() hanging in job_txn_apply
On Fri, 08/24 10:43, Fam Zheng wrote: > All callers have acquired ctx already. Doing that again results in > aio_poll() hang. This fixes the problem that a BDRV_POLL_WHILE() in the > callback cannot make progress because ctx is recursively locked, for > example, when drive-backup finishes. > > There are two callers of job_finalize(): > > fam@lemon:~/work/qemu [master]$ git grep -w -A1 '^\s*job_finalize' > blockdev.c:job_finalize(>job, errp); > blockdev.c-aio_context_release(aio_context); > -- > job-qmp.c:job_finalize(job, errp); > job-qmp.c-aio_context_release(aio_context); > -- > tests/test-blockjob.c:job_finalize(>job, _abort); > tests/test-blockjob.c-assert(job->job.status == JOB_STATUS_CONCLUDED); > > Ignoring the test, it's easy to see both callers to job_finalize (and > job_do_finalize) have acquired the context. > > Cc: qemu-sta...@nongnu.org > Reported-by: Gu Nini > Reviewed-by: Eric Blake > Signed-off-by: Fam Zheng Ping?
Re: [Qemu-block] [Qemu-devel] [PATCH 1/2] commit: Add top-node/base-node options
Am 28.08.2018 um 16:26 hat Peter Krempa geschrieben:
> On Fri, Aug 10, 2018 at 18:26:57 +0200, Kevin Wolf wrote:
> > The block-commit QMP command required specifying the top and base nodes
> > of the commit jobs using the file name of that node. While this works
> > in simple cases (local files with absolute paths), the file names
> > generated for more complicated setups can be hard to predict.
> >
> > This adds two new options top-node and base-node to the command, which
> > allow specifying node names instead. They are mutually exclusive with
> > the old options.
> >
> > Signed-off-by: Kevin Wolf
> > ---
> > qapi/block-core.json | 24 ++--
> > blockdev.c | 32 ++--
> > 2 files changed, 48 insertions(+), 8 deletions(-)
>
> While the below is not strictly relevant to this patch usage
> block-commit is not possible when using -blockdev. Thus the new
> arguments are not very useful otherwise.
>
> With the new options I'm getting:
>
> {"execute":"block-commit",
> "arguments": { "device":"libvirt-3-format",
> "job-id":"libvirt-3-format",
> "top-node":"libvirt-8-format",
> "base-node":"libvirt-9-format",
> "auto-finalize":true,
> "auto-dismiss":false},
> "id":"libvirt-16"}
>
> {"id":"libvirt-16",
> "error":{"class":"GenericError",
> "desc":"Block node is read-only"}}
>
> I'm pointing into the backing chain so the files are declared as read-only.
>
> It works just-fine if I open them as read-write with
> -blockdev/blockdev-add but that obviously is not correct as you can't
> then share parts of the backing chain with other VMs due to image
> locking.
>
> libvirt-3-format is read-write and all other node names are readonly in
> the above example.
>
> The same also happens when using filenames:
>
> {"execute":"block-commit",
> "arguments" : {"device":"libvirt-3-format",
> "job-id":"libvirt-3-format",
> "top":"/var/lib/libvirt/images/rhel7.3.1483615252",
> "base":"/var/lib/libvirt/images/rhel7.3.1483605924",
> "auto-finalize":true,
> "auto-dismiss":false},
> "id":"libvirt-13"}
>
> {"id":"libvirt-13","error":{"class":"GenericError","desc":"Block node is
> read-only"}}
I see what's happening here. So we have a graph like this:
guest device
|
v
overlay-format ---> backing-format
[read-only=off] [read-only=on]
||
vv
overlay-proto backing-proto
[read-only=off] [read-only=on]
The difference between your -blockdev use and -drive is that you
explicitly specify the read-only option for backing-proto (and you use a
separate -blockdev option anyway), so it doesn't just inherit it from
backing-format.
Now, when the commit job tries to reopen backing-format, your explicit
read-only=on for backing-proto still takes precedence and the node stays
read-only. If you hadn't used a separate -blockdev for backing-proto,
but included it in the definition for backing-format and left out the
read-only option, it would have inherited the option and reopen would
adjust both nodes. This is what happens with -drive.
So essentially, I guess, all places that want to switch between
read-only and read-write need to learn which other nodes (apart from the
top-level node they are interested in) must be reopened as well.
This looks a bit messy. :-/
Any good ideas anyone?
Kevin
signature.asc
Description: PGP signature
[Qemu-block] [PATCH v2 01/10] qemu-io: Fix writethrough check in reopen
"qemu-io reopen" doesn't allow changing the writethrough setting of
the cache, but the check is wrong, causing an error even on a simple
reopen with the default parameters:
$ qemu-img create -f qcow2 hd.qcow2 1M
$ qemu-system-x86_64 -monitor stdio -drive if=virtio,file=hd.qcow2
(qemu) qemu-io virtio0 reopen
Cannot change cache.writeback: Device attached
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
qemu-io-cmds.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 5bf5f28178..db0b3ee5ef 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -2025,7 +2025,7 @@ static int reopen_f(BlockBackend *blk, int argc, char
**argv)
return -EINVAL;
}
-if (writethrough != blk_enable_write_cache(blk) &&
+if (!writethrough != blk_enable_write_cache(blk) &&
blk_get_attached_dev(blk))
{
error_report("Cannot change cache.writeback: Device attached");
--
2.11.0
[Qemu-block] [PATCH v2 10/10] block: Allow changing 'force-share' on reopen
'force-share' is one of the basic BlockdevOptions available for all
drivers, but it's not handled by bdrv_reopen_prepare() so any attempt
to change it results in a "Cannot change the option" error:
(qemu) qemu-io virtio0 "reopen -o force-share=on"
Cannot change the option 'force-share'
Since there's no reason why we shouldn't allow changing it and the
implementation is simple let's just do it.
It's worth noting that after this patch the above reopen call will
still return an error -although a different one- if the image is not
read-only:
(qemu) qemu-io virtio0 "reopen -o force-share=on"
force-share=on can only be used with read-only images
Signed-off-by: Alberto Garcia
Cc: Max Reitz
---
block.c | 30 --
include/block/block.h | 1 +
2 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/block.c b/block.c
index 3865ef8517..7ee29ec9eb 100644
--- a/block.c
+++ b/block.c
@@ -788,6 +788,18 @@ static BlockdevDetectZeroesOptions
bdrv_parse_detect_zeroes(QemuOpts *opts,
return detect_zeroes;
}
+static bool bdrv_parse_force_share(QemuOpts *opts, int flags, Error **errp)
+{
+bool value = qemu_opt_get_bool_del(opts, BDRV_OPT_FORCE_SHARE, false);
+
+if (value && (flags & BDRV_O_RDWR)) {
+error_setg(errp, BDRV_OPT_FORCE_SHARE
+ "=on can only be used with read-only images");
+}
+
+return value;
+}
+
/**
* Set open flags for a given discard mode
*
@@ -1373,12 +1385,9 @@ static int bdrv_open_common(BlockDriverState *bs,
BlockBackend *file,
drv = bdrv_find_format(driver_name);
assert(drv != NULL);
-bs->force_share = qemu_opt_get_bool(opts, BDRV_OPT_FORCE_SHARE, false);
-
-if (bs->force_share && (bs->open_flags & BDRV_O_RDWR)) {
-error_setg(errp,
- BDRV_OPT_FORCE_SHARE
- "=on can only be used with read-only images");
+bs->force_share = bdrv_parse_force_share(opts, bs->open_flags, _err);
+if (local_err) {
+error_propagate(errp, local_err);
ret = -EINVAL;
goto fail_opts;
}
@@ -3201,6 +3210,14 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
goto error;
}
+reopen_state->force_share =
+bdrv_parse_force_share(opts, reopen_state->flags, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto error;
+}
+
/* All other options (including node-name and driver) must be unchanged.
* Put them back into the QDict, so that they are checked at the end
* of this function. */
@@ -3351,6 +3368,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
bs->open_flags = reopen_state->flags;
bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
bs->detect_zeroes = reopen_state->detect_zeroes;
+bs->force_share= reopen_state->force_share;
/* Remove child references from bs->options and bs->explicit_options.
* Child options were already removed in bdrv_reopen_queue_child() */
diff --git a/include/block/block.h b/include/block/block.h
index f71fa5a1c4..a49a027c54 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -185,6 +185,7 @@ typedef struct BDRVReopenState {
BlockDriverState *bs;
int flags;
BlockdevDetectZeroesOptions detect_zeroes;
+bool force_share;
uint64_t perm, shared_perm;
QDict *options;
QDict *explicit_options;
--
2.11.0
[Qemu-block] [PATCH v2 03/10] block: Remove child references from bs->{options, explicit_options}
Block drivers allow opening their children using a reference to an
existing BlockDriverState. These references remain stored in the
'options' and 'explicit_options' QDicts, but we don't need to keep
them once everything is open.
What is more important, these values can become wrong if the children
change:
$ qemu-img create -f qcow2 hd0.qcow2 10M
$ qemu-img create -f qcow2 hd1.qcow2 10M
$ qemu-img create -f qcow2 hd2.qcow2 10M
$ $QEMU -drive if=none,file=hd0.qcow2,node-name=hd0 \
-drive if=none,file=hd1.qcow2,node-name=hd1,backing=hd0 \
-drive file=hd2.qcow2,node-name=hd2,backing=hd1
After this hd2 has hd1 as its backing file. Now let's remove it using
block_stream:
(qemu) block_stream hd2 0 hd0.qcow2
Now hd0 is the backing file of hd2, but hd2's options QDicts still
contain backing=hd1.
Signed-off-by: Alberto Garcia
---
block.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/block.c b/block.c
index 0dbb1fcc7b..c764eb731c 100644
--- a/block.c
+++ b/block.c
@@ -2763,12 +2763,15 @@ static BlockDriverState *bdrv_open_inherit(const char
*filename,
}
}
-/* Remove all children options from bs->options and bs->explicit_options */
+/* Remove all children options and references
+ * from bs->options and bs->explicit_options */
QLIST_FOREACH(child, >children, next) {
char *child_key_dot;
child_key_dot = g_strdup_printf("%s.", child->name);
qdict_extract_subqdict(bs->explicit_options, NULL, child_key_dot);
qdict_extract_subqdict(bs->options, NULL, child_key_dot);
+qdict_del(bs->explicit_options, child->name);
+qdict_del(bs->options, child->name);
g_free(child_key_dot);
}
@@ -3289,6 +3292,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
{
BlockDriver *drv;
BlockDriverState *bs;
+BdrvChild *child;
bool old_can_write, new_can_write;
assert(reopen_state != NULL);
@@ -3313,6 +3317,13 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
bs->open_flags = reopen_state->flags;
bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
+/* Remove child references from bs->options and bs->explicit_options.
+ * Child options were already removed in bdrv_reopen_queue_child() */
+QLIST_FOREACH(child, >children, next) {
+qdict_del(bs->explicit_options, child->name);
+qdict_del(bs->options, child->name);
+}
+
bdrv_refresh_limits(bs, NULL);
bdrv_set_perm(reopen_state->bs, reopen_state->perm,
--
2.11.0
[Qemu-block] [PATCH v2 06/10] block: Forbid trying to change unsupported options during reopen
The bdrv_reopen_prepare() function checks all options passed to each
BlockDriverState (in the reopen_state->options QDict) and makes all
necessary preparations to apply the option changes requested by the
user.
Options are removed from the QDict as they are processed, so at the
end of bdrv_reopen_prepare() only the options that can't be changed
are left. Then a loop goes over all remaining options and verifies
that the old and new values are identical, returning an error if
they're not.
The problem is that at the moment there are options that are removed
from the QDict although they can't be changed. The consequence of this
is any modification to any of those options is silently ignored:
(qemu) qemu-io virtio0 "reopen -o discard=on"
This happens when all options from bdrv_runtime_opts are removed
from the QDict but then only a few of them are processed. Since
it's especially important that "node-name" and "driver" are not
changed, the code puts them back into the QDict so they are checked
at the end of the function. Instead of putting only those two options
back into the QDict, this patch puts all unprocessed options using
qemu_opts_to_qdict().
update_flags_from_options() also needs to be modified to prevent
BDRV_OPT_CACHE_NO_FLUSH, BDRV_OPT_CACHE_DIRECT and BDRV_OPT_READ_ONLY
from going back to the QDict.
Signed-off-by: Alberto Garcia
---
block.c | 22 +++---
1 file changed, 7 insertions(+), 15 deletions(-)
diff --git a/block.c b/block.c
index 5223d16f1b..1c0f72454a 100644
--- a/block.c
+++ b/block.c
@@ -1094,19 +1094,19 @@ static void update_flags_from_options(int *flags,
QemuOpts *opts)
*flags &= ~BDRV_O_CACHE_MASK;
assert(qemu_opt_find(opts, BDRV_OPT_CACHE_NO_FLUSH));
-if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) {
+if (qemu_opt_get_bool_del(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) {
*flags |= BDRV_O_NO_FLUSH;
}
assert(qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT));
-if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_DIRECT, false)) {
+if (qemu_opt_get_bool_del(opts, BDRV_OPT_CACHE_DIRECT, false)) {
*flags |= BDRV_O_NOCACHE;
}
*flags &= ~BDRV_O_RDWR;
assert(qemu_opt_find(opts, BDRV_OPT_READ_ONLY));
-if (!qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false)) {
+if (!qemu_opt_get_bool_del(opts, BDRV_OPT_READ_ONLY, false)) {
*flags |= BDRV_O_RDWR;
}
@@ -3155,7 +3155,6 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
BlockDriver *drv;
QemuOpts *opts;
QDict *orig_reopen_opts;
-const char *value;
bool read_only;
assert(reopen_state != NULL);
@@ -3178,17 +3177,10 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
update_flags_from_options(_state->flags, opts);
-/* node-name and driver must be unchanged. Put them back into the QDict, so
- * that they are checked at the end of this function. */
-value = qemu_opt_get(opts, "node-name");
-if (value) {
-qdict_put_str(reopen_state->options, "node-name", value);
-}
-
-value = qemu_opt_get(opts, "driver");
-if (value) {
-qdict_put_str(reopen_state->options, "driver", value);
-}
+/* All other options (including node-name and driver) must be unchanged.
+ * Put them back into the QDict, so that they are checked at the end
+ * of this function. */
+qemu_opts_to_qdict(opts, reopen_state->options);
/* If we are to stay read-only, do not allow permission change
* to r/w. Attempting to set to r/w may fail if either BDRV_O_ALLOW_RDWR is
--
2.11.0
[Qemu-block] [PATCH v2 00/10] Misc reopen-related patches
Hi,
as part of my blockdev-reopen work here's a new set of patches. This
doesn't implement yet the core functionality of the new reopen
command, but it does fix a few things that help us pave the way.
I believe that the next series after this one will be the last.
The main change is the removal of child references from the options
and explicit_options QDicts. This was already discussed in the
previous series[1], and here's the implementation.
Regards,
Berto
[1] https://lists.gnu.org/archive/html/qemu-block/2018-08/msg00474.html
v2:
- Patches 3 and 5: Make comments more explicit. [Max]
- Patch 6: Use qemu_opts_to_qdict() in bdrv_reopen_prepare() to put
all unprocessed options back into the QDict. [Max]
- Patches 8-10: Use qemu_opt_get_del() and update commit messages to
reflect the changes in patch 6. [Max]
v1: https://lists.gnu.org/archive/html/qemu-block/2018-08/msg00846.html
- Initial version
Output of backport-diff against v1:
Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
001/10:[] [--] 'qemu-io: Fix writethrough check in reopen'
002/10:[] [--] 'file-posix: x-check-cache-dropped should default to false
on reopen'
003/10:[0003] [FC] 'block: Remove child references from
bs->{options,explicit_options}'
004/10:[] [--] 'block: Don't look for child references in
append_open_options()'
005/10:[0005] [FC] 'block: Allow child references on reopen'
006/10:[down] 'block: Forbid trying to change unsupported options during reopen'
007/10:[] [--] 'file-posix: Forbid trying to change unsupported options
during reopen'
008/10:[0003] [FC] 'block: Allow changing 'discard' on reopen'
009/10:[0002] [FC] 'block: Allow changing 'detect-zeroes' on reopen'
010/10:[0002] [FC] 'block: Allow changing 'force-share' on reopen'
Alberto Garcia (10):
qemu-io: Fix writethrough check in reopen
file-posix: x-check-cache-dropped should default to false on reopen
block: Remove child references from bs->{options,explicit_options}
block: Don't look for child references in append_open_options()
block: Allow child references on reopen
block: Forbid trying to change unsupported options during reopen
file-posix: Forbid trying to change unsupported options during reopen
block: Allow changing 'discard' on reopen
block: Allow changing 'detect-zeroes' on reopen
block: Allow changing 'force-share' on reopen
block.c | 161 +-
block/file-posix.c| 9 ++-
include/block/block.h | 2 +
qemu-io-cmds.c| 2 +-
4 files changed, 117 insertions(+), 57 deletions(-)
--
2.11.0
Re: [Qemu-block] [PATCH 0/2] commit: Add top-node/base-node options
Am 10.08.2018 um 18:26 hat Kevin Wolf geschrieben: > Kevin Wolf (2): > commit: Add top-node/base-node options > qemu-iotests: Test commit with top-node/base-node Fixed the version numbers in the QMP documentation and applied to the block branch. I'll still have to look into Peter's problem, but as the same happens with filenames, there's no reason why it should hold up this series. Kevin
[Qemu-block] [PATCH v2 07/10] file-posix: Forbid trying to change unsupported options during reopen
The file-posix code is used for the "file", "host_device" and
"host_cdrom" drivers, and it allows reopening images. However the only
option that is actually processed is "x-check-cache-dropped", and
changes in all other options (e.g. "filename") are silently ignored:
(qemu) qemu-io virtio0 "reopen -o file.filename=no-such-file"
While we could allow changing some of the other options, let's keep
things as they are for now but return an error if the user tries to
change any of them.
Signed-off-by: Alberto Garcia
---
block/file-posix.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index ddac0cc3e6..d4ec2cb3dd 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -850,8 +850,13 @@ static int raw_reopen_prepare(BDRVReopenState *state,
goto out;
}
-rs->check_cache_dropped = qemu_opt_get_bool(opts, "x-check-cache-dropped",
-false);
+rs->check_cache_dropped =
+qemu_opt_get_bool_del(opts, "x-check-cache-dropped", false);
+
+/* This driver's reopen function doesn't currently allow changing
+ * other options, so let's put them back in the original QDict and
+ * bdrv_reopen_prepare() will detect changes and complain. */
+qemu_opts_to_qdict(opts, state->options);
if (s->type == FTYPE_CD) {
rs->open_flags |= O_NONBLOCK;
--
2.11.0
[Qemu-block] [PATCH v2 09/10] block: Allow changing 'detect-zeroes' on reopen
'detect-zeroes' is one of the basic BlockdevOptions available for all
drivers, but it's not handled by bdrv_reopen_prepare(), so any attempt
to change it results in an error:
(qemu) qemu-io virtio0 "reopen -o detect-zeroes=on"
Cannot change the option 'detect-zeroes'
Since there's no reason why we shouldn't allow changing it and the
implementation is simple let's just do it.
Signed-off-by: Alberto Garcia
Cc: Max Reitz
---
block.c | 63 +++
include/block/block.h | 1 +
2 files changed, 40 insertions(+), 24 deletions(-)
diff --git a/block.c b/block.c
index 3f4b7640fa..3865ef8517 100644
--- a/block.c
+++ b/block.c
@@ -764,6 +764,30 @@ static void bdrv_join_options(BlockDriverState *bs, QDict
*options,
}
}
+static BlockdevDetectZeroesOptions bdrv_parse_detect_zeroes(QemuOpts *opts,
+int open_flags,
+Error **errp)
+{
+Error *local_err = NULL;
+const char *value = qemu_opt_get_del(opts, "detect-zeroes");
+BlockdevDetectZeroesOptions detect_zeroes =
+qapi_enum_parse(_lookup, value,
+BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return detect_zeroes;
+}
+
+if (detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
+!(open_flags & BDRV_O_UNMAP))
+{
+error_setg(errp, "setting detect-zeroes to unmap is not allowed "
+ "without setting discard operation to unmap");
+}
+
+return detect_zeroes;
+}
+
/**
* Set open flags for a given discard mode
*
@@ -1328,7 +1352,6 @@ static int bdrv_open_common(BlockDriverState *bs,
BlockBackend *file,
const char *driver_name = NULL;
const char *node_name = NULL;
const char *discard;
-const char *detect_zeroes;
QemuOpts *opts;
BlockDriver *drv;
Error *local_err = NULL;
@@ -1417,29 +1440,12 @@ static int bdrv_open_common(BlockDriverState *bs,
BlockBackend *file,
}
}
-detect_zeroes = qemu_opt_get(opts, "detect-zeroes");
-if (detect_zeroes) {
-BlockdevDetectZeroesOptions value =
-qapi_enum_parse(_lookup,
-detect_zeroes,
-BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
-_err);
-if (local_err) {
-error_propagate(errp, local_err);
-ret = -EINVAL;
-goto fail_opts;
-}
-
-if (value == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
-!(bs->open_flags & BDRV_O_UNMAP))
-{
-error_setg(errp, "setting detect-zeroes to unmap is not allowed "
- "without setting discard operation to unmap");
-ret = -EINVAL;
-goto fail_opts;
-}
-
-bs->detect_zeroes = value;
+bs->detect_zeroes =
+bdrv_parse_detect_zeroes(opts, bs->open_flags, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto fail_opts;
}
if (filename != NULL) {
@@ -3187,6 +3193,14 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
}
}
+reopen_state->detect_zeroes =
+bdrv_parse_detect_zeroes(opts, reopen_state->flags, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto error;
+}
+
/* All other options (including node-name and driver) must be unchanged.
* Put them back into the QDict, so that they are checked at the end
* of this function. */
@@ -3336,6 +3350,7 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
bs->options= reopen_state->options;
bs->open_flags = reopen_state->flags;
bs->read_only = !(reopen_state->flags & BDRV_O_RDWR);
+bs->detect_zeroes = reopen_state->detect_zeroes;
/* Remove child references from bs->options and bs->explicit_options.
* Child options were already removed in bdrv_reopen_queue_child() */
diff --git a/include/block/block.h b/include/block/block.h
index 4e0871aaf9..f71fa5a1c4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -184,6 +184,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue,
BlockReopenQueueEntry) BlockReopenQueue;
typedef struct BDRVReopenState {
BlockDriverState *bs;
int flags;
+BlockdevDetectZeroesOptions detect_zeroes;
uint64_t perm, shared_perm;
QDict *options;
QDict *explicit_options;
--
2.11.0
[Qemu-block] [PATCH v2 08/10] block: Allow changing 'discard' on reopen
'discard' is one of the basic BlockdevOptions available for all
drivers, but it's not handled by bdrv_reopen_prepare() so any attempt
to change it results in an error:
(qemu) qemu-io virtio0 "reopen -o discard=on"
Cannot change the option 'discard'
Since there's no reason why we shouldn't allow changing it and the
implementation is simple let's just do it.
Signed-off-by: Alberto Garcia
Cc: Max Reitz
---
block.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/block.c b/block.c
index 1c0f72454a..3f4b7640fa 100644
--- a/block.c
+++ b/block.c
@@ -3155,6 +3155,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
BlockDriver *drv;
QemuOpts *opts;
QDict *orig_reopen_opts;
+const char *value;
bool read_only;
assert(reopen_state != NULL);
@@ -3177,6 +3178,15 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
update_flags_from_options(_state->flags, opts);
+value = qemu_opt_get_del(opts, "discard");
+if (value != NULL) {
+if (bdrv_parse_discard_flags(value, _state->flags) != 0) {
+error_setg(errp, "Invalid discard option");
+ret = -EINVAL;
+goto error;
+}
+}
+
/* All other options (including node-name and driver) must be unchanged.
* Put them back into the QDict, so that they are checked at the end
* of this function. */
--
2.11.0
[Qemu-block] [PATCH v2 05/10] block: Allow child references on reopen
In the previous patches we removed all child references from
bs->{options,explicit_options} because keeping them is useless and
wrong.
Because of this, any attempt to reopen a BlockDriverState using a
child reference as one of its options would result in a failure,
because bdrv_reopen_prepare() would detect that there's a new option
(the child reference) that wasn't present in bs->options.
But passing child references on reopen can be useful. It's a way to
specify a BDS's child without having to pass recursively all of the
child's options, and if the reference points to a different BDS then
this can allow us to replace the child.
However, replacing the child is something that needs to be implemented
case by case and only when it makes sense. For now, this patch allows
passing a child reference as long as it points to the current child of
the BlockDriverState.
It's also important to remember that, as a consequence of the
previous patches, this child reference will be removed from
bs->{options,explicit_options} after the reopening has been completed.
Signed-off-by: Alberto Garcia
---
block.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/block.c b/block.c
index aaa4cf6897..5223d16f1b 100644
--- a/block.c
+++ b/block.c
@@ -3241,6 +3241,24 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
QObject *new = entry->value;
QObject *old = qdict_get(reopen_state->bs->options, entry->key);
+/* Allow child references (child_name=node_name) as long as they
+ * point to the current child (i.e. everything stays the same). */
+if (qobject_type(new) == QTYPE_QSTRING) {
+BdrvChild *child;
+QLIST_FOREACH(child, _state->bs->children, next) {
+if (!strcmp(child->name, entry->key)) {
+break;
+}
+}
+
+if (child) {
+const char *str = qobject_get_try_str(new);
+if (!strcmp(child->bs->node_name, str)) {
+continue; /* Found child with this name, skip option */
+}
+}
+}
+
/*
* TODO: When using -drive to specify blockdev options, all values
* will be strings; however, when using -blockdev, blockdev-add or
--
2.11.0
[Qemu-block] [PATCH v2 04/10] block: Don't look for child references in append_open_options()
In the previous patch we removed child references from bs->options, so
there's no need to look for them here anymore.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block.c | 13 +
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/block.c b/block.c
index c764eb731c..aaa4cf6897 100644
--- a/block.c
+++ b/block.c
@@ -5154,23 +5154,12 @@ static bool append_open_options(QDict *d,
BlockDriverState *bs)
{
const QDictEntry *entry;
QemuOptDesc *desc;
-BdrvChild *child;
bool found_any = false;
for (entry = qdict_first(bs->options); entry;
entry = qdict_next(bs->options, entry))
{
-/* Exclude node-name references to children */
-QLIST_FOREACH(child, >children, next) {
-if (!strcmp(entry->key, child->name)) {
-break;
-}
-}
-if (child) {
-continue;
-}
-
-/* And exclude all non-driver-specific options */
+/* Exclude all non-driver-specific options */
for (desc = bdrv_runtime_opts.desc; desc->name; desc++) {
if (!strcmp(qdict_entry_key(entry), desc->name)) {
break;
--
2.11.0
[Qemu-block] [PATCH v2 02/10] file-posix: x-check-cache-dropped should default to false on reopen
The default value of x-check-cache-dropped is false. There's no reason
to use the previous value as a default in raw_reopen_prepare() because
bdrv_reopen_queue_child() already takes care of putting the old
options in the BDRVReopenState.options QDict.
If x-check-cache-dropped was previously set but is now missing from
the reopen QDict then it should be reset to false.
Signed-off-by: Alberto Garcia
Reviewed-by: Max Reitz
---
block/file-posix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index fe83cbf0eb..ddac0cc3e6 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -851,7 +851,7 @@ static int raw_reopen_prepare(BDRVReopenState *state,
}
rs->check_cache_dropped = qemu_opt_get_bool(opts, "x-check-cache-dropped",
-s->check_cache_dropped);
+false);
if (s->type == FTYPE_CD) {
rs->open_flags |= O_NONBLOCK;
--
2.11.0
Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/9] jobs: canonize Error object
Kevin Wolf writes:
> Am 01.09.2018 um 09:54 hat Markus Armbruster geschrieben:
>> John Snow writes:
>>
>> > On 08/31/2018 02:08 AM, Markus Armbruster wrote:
>> >> Eric Blake writes:
>> >>
>> >>> On 08/29/2018 08:57 PM, John Snow wrote:
>> Jobs presently use both an Error object in the case of the create job,
>> and char strings in the case of generic errors elsewhere.
>>
>> Unify the two paths as just j->err, and remove the extra argument from
>> job_completed. The integer error code for job_completed is kept for now,
>> to be removed shortly in a separate patch.
>>
>> Signed-off-by: John Snow
>> ---
>> >>>
>> +++ b/job.c
>> >>>
>> @@ -666,8 +666,8 @@ static void job_update_rc(Job *job)
>> job->ret = -ECANCELED;
>> }
>> if (job->ret) {
>> -if (!job->error) {
>> -job->error = g_strdup(strerror(-job->ret));
>> +if (!job->err) {
>> +error_setg(>err, "%s", g_strdup(strerror(-job->ret)));
>> >>>
>> >>> Memleak. Drop the g_strdup(), and just directly pass strerror()
>> >>> results to error_setg(). (I guess we can't quite use
>> >>> error_setg_errno() unless we add additional text beyond the strerror()
>> >>> results).
>> >>
>> >> Adding such text might well be an improvement. I'm not telling you to
>> >> do so (not having looked at the context myself), just to think about it.
>> >>
>> >
>> > In this case, and I agree with Kevin who suggested it; we ought to be
>> > moving away from the retcode in general and using first-class error
>> > objects for all of our jobs anyway.
>> >
>> > In this case, the job has failed with a retcode and we wish to give the
>> > user some hope of understanding why, but at this point in the code all
>> > we know is what the strerror can tell us, so a generic prefix like "The
>> > job failed" is not helpful because it will already be clear by events
>> > and other things that the job failed.
>> >
>> > The only text I can think of that would be useful is: "The job failed
>> > and didn't give a more specific error message. Please email
>> > qemu-de...@nongnu.org and harass the authors until they fix it. Anyway,
>> > nice chatting to you, the generic error message is: %s"
>>
>> That might well be an improvement ;)
>>
>> Since I don't have a realistic example ready, I'm making up a contrieved
>> one:
>>
>> --> {"execute": "job-frobnicate"}
>> <-- {"error": {"class": "GenericError", "desc": "Device or resource busy"}}
>>
>> Would a reply
>>
>> <-- {"error": {"class": "GenericError", "desc": "Job failed: Device or
>> resource busy"}}
>>
>> be better, worse, or a wash?
>>
>> If it's a wash, maintainability breaks the tie. So let's have a look at
>> the code. It's either
>>
>> error_setg(>err, "%s", strerror(-job->ret));
>>
>> or
>>
>> error_setg_errno(>err, -job->ret, "Job failed");
>>
>> I'd prefer the latter, because it's the common way to put an errno code
>> into an Error object, and it lets me grep for the message more easily.
>
> Basically, if I call "job-frobnicate", I don't want an error message to
> tell me "job-frobnicate failed: foo". I already know that I called
> "job-frobnicate", so the actually useful message is only "foo".
>
> A prefix like "job-frobnicate failed" should be added when it's one of
> multiple possible error sources. For example, if a higher level monitor
> command internally involves job-frobnicate, but also three other
> operations, this is the place where the prefix should be added to any
> error message returned by job-frobnicate so that we can distinguish
> which part of the operation failed.
John's point is that the error message is bad no matter what.
My point is that if it's bad no matter what, we can just as well emit it
the usual way, with error_setg_errno(), rather than playing games with
strerror().
Re: [Qemu-block] [Qemu-devel] [PATCH v3 2/9] jobs: canonize Error object
Am 01.09.2018 um 09:54 hat Markus Armbruster geschrieben:
> John Snow writes:
>
> > On 08/31/2018 02:08 AM, Markus Armbruster wrote:
> >> Eric Blake writes:
> >>
> >>> On 08/29/2018 08:57 PM, John Snow wrote:
> Jobs presently use both an Error object in the case of the create job,
> and char strings in the case of generic errors elsewhere.
>
> Unify the two paths as just j->err, and remove the extra argument from
> job_completed. The integer error code for job_completed is kept for now,
> to be removed shortly in a separate patch.
>
> Signed-off-by: John Snow
> ---
> >>>
> +++ b/job.c
> >>>
> @@ -666,8 +666,8 @@ static void job_update_rc(Job *job)
> job->ret = -ECANCELED;
> }
> if (job->ret) {
> -if (!job->error) {
> -job->error = g_strdup(strerror(-job->ret));
> +if (!job->err) {
> +error_setg(>err, "%s", g_strdup(strerror(-job->ret)));
> >>>
> >>> Memleak. Drop the g_strdup(), and just directly pass strerror()
> >>> results to error_setg(). (I guess we can't quite use
> >>> error_setg_errno() unless we add additional text beyond the strerror()
> >>> results).
> >>
> >> Adding such text might well be an improvement. I'm not telling you to
> >> do so (not having looked at the context myself), just to think about it.
> >>
> >
> > In this case, and I agree with Kevin who suggested it; we ought to be
> > moving away from the retcode in general and using first-class error
> > objects for all of our jobs anyway.
> >
> > In this case, the job has failed with a retcode and we wish to give the
> > user some hope of understanding why, but at this point in the code all
> > we know is what the strerror can tell us, so a generic prefix like "The
> > job failed" is not helpful because it will already be clear by events
> > and other things that the job failed.
> >
> > The only text I can think of that would be useful is: "The job failed
> > and didn't give a more specific error message. Please email
> > qemu-de...@nongnu.org and harass the authors until they fix it. Anyway,
> > nice chatting to you, the generic error message is: %s"
>
> That might well be an improvement ;)
>
> Since I don't have a realistic example ready, I'm making up a contrieved
> one:
>
> --> {"execute": "job-frobnicate"}
> <-- {"error": {"class": "GenericError", "desc": "Device or resource busy"}}
>
> Would a reply
>
> <-- {"error": {"class": "GenericError", "desc": "Job failed: Device or
> resource busy"}}
>
> be better, worse, or a wash?
>
> If it's a wash, maintainability breaks the tie. So let's have a look at
> the code. It's either
>
> error_setg(>err, "%s", strerror(-job->ret));
>
> or
>
> error_setg_errno(>err, -job->ret, "Job failed");
>
> I'd prefer the latter, because it's the common way to put an errno code
> into an Error object, and it lets me grep for the message more easily.
Basically, if I call "job-frobnicate", I don't want an error message to
tell me "job-frobnicate failed: foo". I already know that I called
"job-frobnicate", so the actually useful message is only "foo".
A prefix like "job-frobnicate failed" should be added when it's one of
multiple possible error sources. For example, if a higher level monitor
command internally involves job-frobnicate, but also three other
operations, this is the place where the prefix should be added to any
error message returned by job-frobnicate so that we can distinguish
which part of the operation failed.
Kevin
Re: [Qemu-block] [PATCH 1/3] util: add qemu_write_pidfile()
On Fri, Aug 31, 2018 at 04:53:12PM +0200, Marc-André Lureau wrote: > There are variants of qemu_create_pidfile() in qemu-pr-helper and > qemu-ga. Let's have a common implementation in libqemuutil. > > The code is initially based from pr-helper write_pidfile(), with > various improvements and suggestions from Daniel Berrangé: > > QEMU will leave the pidfile existing on disk when it exits which > initially made me think it avoids the deletion race. The app > managing QEMU, however, may well delete the pidfile after it has > seen QEMU exit, and even if the app locks the pidfile before > deleting it, there is still a race. > > eg consider the following sequence > > QEMU 1libvirtdQEMU 2 > > 1.lock(pidfile) > > 2.exit() > > 3. open(pidfile) > > 4. lock(pidfile) > > 5. open(pidfile) > > 6. unlink(pidfile) > > 7. close(pidfile) > > 8. lock(pidfile) > > IOW, at step 8 the new QEMU has successfully acquired the lock, but > the pidfile no longer exists on disk because it was deleted after > the original QEMU exited. > > While we could just say no external app should ever delete the > pidfile, I don't think that is satisfactory as people don't read > docs, and admins don't like stale pidfiles being left around on > disk. > > To make this robust, I think we might want to copy libvirt's > approach to pidfile acquisition which runs in a loop and checks that > the file on disk /after/ acquiring the lock matches the file that > was locked. Then we could in fact safely let QEMU delete its own > pidfiles on clean exit.. > > Signed-off-by: Marc-André Lureau > --- > include/qemu/osdep.h | 3 +- > os-posix.c| 24 --- > os-win32.c| 25 > qga/main.c| 54 +++--- > scsi/qemu-pr-helper.c | 40 - > util/oslib-posix.c| 68 +++ > util/oslib-win32.c| 27 + > vl.c | 4 +-- > 8 files changed, 114 insertions(+), 131 deletions(-) Reviewed-by: Daniel P. Berrangé Regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [Qemu-block] [PATCH v3 10/15] tests/test-blockjob-txn: move .exit to .clean
On 2018-09-01 00:29, John Snow wrote: > The exit callback in this test actually only performs cleanup. > > Signed-off-by: John Snow > --- > tests/test-blockjob-txn.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Max Reitz signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v3 07/15] block/commit: refactor stream to use job callbacks
On 2018-09-01 00:28, John Snow wrote: > Signed-off-by: John Snow > Reviewed-by: Max Reitz > --- > block/stream.c | 23 +++ > 1 file changed, 15 insertions(+), 8 deletions(-) You forgot to fix the commit title. ;-) Max signature.asc Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v3 06/15] block/mirror: conservative mirror_exit refactor
On 2018-09-01 00:28, John Snow wrote:
> For purposes of minimum code movement, refactor the mirror_exit
> callback to use the post-finalization callbacks in a trivial way.
>
> Signed-off-by: John Snow
> ---
> block/mirror.c | 31 +--
> 1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index c164fee883..5067f1764d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
[...]
> @@ -617,7 +618,13 @@ static void mirror_exit(Job *job)
> BlockDriverState *target_bs = blk_bs(s->target);
> BlockDriverState *mirror_top_bs = s->mirror_top_bs;
> Error *local_err = NULL;
> -int ret = job->ret;
> +bool abort = !!job->ret;
I think "job->ret < 0" could be read more easily.
> +int ret = 0;
> +
> +if (s->prepared) {
> +return 0;
> +}
> +s->prepared = true;
>
> bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
>
[...]
> @@ -712,7 +719,17 @@ static void mirror_exit(Job *job)
> bdrv_unref(mirror_top_bs);
> bdrv_unref(src);
>
> -job->ret = ret;
> +return ret;
> +}
> +
> +static int mirror_prepare(Job *job)
> +{
> +return mirror_exit_common(job);
> +}
> +
> +static void mirror_abort(Job *job)
> +{
> +assert(mirror_exit_common(job) == 0);
You shouldn't execute vital code in assert(), as NDEBUG would make that
code disappear. As far as I know, we have decided (at some point) not
to ever enable NDEBUG in qemu, but, you know. Doing it right only costs
one more line, and it would get you a
Reviewed-by: Max Reitz
> }
>
> static void mirror_throttle(MirrorBlockJob *s)
signature.asc
Description: OpenPGP digital signature
Re: [Qemu-block] [PATCH v3 05/15] block/mirror: don't install backing chain on abort
On 2018-09-01 00:28, John Snow wrote:
> In cases where we abort the block/mirror job, there's no point in
> installing the new backing chain before we finish aborting.
>
> Move this to the "success" portion of mirror_exit.
>
> Signed-off-by: John Snow
> ---
> block/mirror.c | 27 ++-
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index cba555b4ef..c164fee883 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -642,16 +642,6 @@ static void mirror_exit(Job *job)
> * required before it could become a backing file of target_bs. */
> bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
> _abort);
> -if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> -BlockDriverState *backing = s->is_none_mode ? src : s->base;
> -if (backing_bs(target_bs) != backing) {
> -bdrv_set_backing_hd(target_bs, backing, _err);
> -if (local_err) {
> -error_report_err(local_err);
> -ret = -EPERM;
> -}
> -}
> -}
>
> if (s->to_replace) {
> replace_aio_context = bdrv_get_aio_context(s->to_replace);
> @@ -659,9 +649,18 @@ static void mirror_exit(Job *job)
> }
>
> if (s->should_complete && ret == 0) {
> -BlockDriverState *to_replace = src;
> -if (s->to_replace) {
> -to_replace = s->to_replace;
> +BlockDriverState *to_replace = s->to_replace ? s->to_replace : src;
> +
> +if (s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
> +BlockDriverState *backing = s->is_none_mode ? src : s->base;
> +if (backing_bs(target_bs) != backing) {
> +bdrv_set_backing_hd(target_bs, backing, _err);
> +if (local_err) {
> +error_report_err(local_err);
> +ret = -EPERM;
> +goto clean;
> +}
> +}
> }
>
> if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
Testing shows that on post-READY cancel, s->should_complete is 0 (which
makes sense, because all the rest in this path is about replacing
to_replace by target_bs, which we don't want to do on cancel), so this
would not be executed.
However, I think we do want to give the target the correct backing chain
when the job is canceled this way (as I suppose there are ways to keep
the target around even with drive-mirror).
So we should attach the backing chain whenever ret == 0, not just when
s->should_complete is true.
Max
> @@ -678,6 +677,8 @@ static void mirror_exit(Job *job)
> ret = -EPERM;
> }
> }
> +
> + clean:
> if (s->to_replace) {
> bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
> error_free(s->replace_blocker);
>
signature.asc
Description: OpenPGP digital signature
