Don't pass flags to bdrv_reopen_queue()
v2 cover Don't pass flags to bdrv_reopen_queue()
Hi all,
when reopening a BlockDriverState using bdrv_reopen() and friends the new options can be specified either with a QDict or with flags. Both methods overlap and that makes the semantics and the implementation unnecessarily complicated.
This series removes the 'flags' parameter from these functions, so from now on all option changes must be specified using a QDict. Apart from simplifying the API, a few bugs are fixed along the way. See the individual patches for more details.
This was tested with the current master (09558375a634e17cea6cfbfec88).
Regards,
Berto
v2:
- Patches 4 and 9: Fixed trivial rebase conflict
- Patch 11: Update messages [Max]
- Patch 12: Add comment and use bdrv_is_read_only() instead of using the flags directly [Max]
- Patch 14: Remove inner block and move all variable declarations to the beginning of the function [Max]
v1: https://lists.gnu.org/archive/html/qemu-block/2018-09/msg00483.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/14:[----] [--] 'block: Don't call update_flags_from_options() if the options are wrong' 002/14:[----] [--] 'block: Add bdrv_reopen_set_read_only()'
003/14:[----] [--] 'block: Use bdrv_reopen_set_read_only() in bdrv_backing_update_filename()' 004/14:[0006] [FC] 'block: Use bdrv_reopen_set_read_only() in commit_start/complete()' 005/14:[----] [--] 'block: Use bdrv_reopen_set_read_only() in bdrv_commit()' 006/14:[----] [-C] 'block: Use bdrv_reopen_set_read_only() in stream_start/complete()' 007/14:[----] [--] 'block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file()' 008/14:[----] [--] 'block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()' 009/14:[0003] [FC] 'block: Use bdrv_reopen_set_read_only() in the mirror driver' 010/14:[----] [--] 'block: Drop bdrv_reopen()'
011/14:[0018] [FC] 'qemu-io: Put flag changes in the options QDict in reopen_f()' 012/14:[0011] [FC] 'block: Clean up reopen_backing_file() in block/replication.c' 013/14:[----] [--] 'block: Remove flags parameter from bdrv_reopen_queue()' 014/14:[0042] [FC] 'block: Stop passing flags to bdrv_reopen_queue_child()'
Alberto Garcia (14):
block: Don't call update_flags_from_options() if the options are wrong
block: Add bdrv_reopen_set_read_only()
block: Use bdrv_reopen_set_read_only() in
bdrv_backing_update_filename()
block: Use bdrv_reopen_set_read_only() in commit_start/complete()
block: Use bdrv_reopen_set_read_only() in bdrv_commit()
block: Use bdrv_reopen_set_read_only() in stream_start/complete()
block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file()
block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()
block: Use bdrv_reopen_set_read_only() in the mirror driver
block: Drop bdrv_reopen()
qemu-io: Put flag changes in the options QDict in reopen_f()
block: Clean up reopen_backing_file() in block/replication.c
block: Remove flags parameter from bdrv_reopen_queue()
block: Stop passing flags to bdrv_reopen_queue_child()
block.c | 86 +++++++++++++++++++++------------------------- block/commit.c | 23 +++++-------- block/mirror.c | 19 ++++++---- block/replication.c | 43 ++++++++++------------- block/stream.c | 20 +++++------ blockdev.c | 11 ++---- include/block/block.h | 6 ++-- qemu-io-cmds.c | 29 ++++++++++++++-- tests/qemu-iotests/133 | 15 ++++++++ tests/qemu-iotests/133.out | 12 +++++++ 10 files changed, 150 insertions(+), 114 deletions(-)
Signature · 1 line
2.11.0
PATCH v2 1/14 block: Don't call update_flags_from_options() if the options are wrong
If qemu_opts_absorb_qdict() fails and we carry on and call update_flags_from_options() then that can result on a failed assertion:
$ qemu-io -c 'reopen -o read-only=foo' hd.qcow2 block.c:1101: update_flags_from_options: Assertion `qemu_opt_find(opts, BDRV_OPT_CACHE_DIRECT)' failed. Aborted
This only happens in bdrv_reopen_queue_child(). Although this function doesn't return errors, we can simply keep the wrong options in the reopen queue and later bdrv_reopen_prepare() will fail.
--- block.c | 11 +++++++++-- tests/qemu-iotests/133 | 6 ++++++ tests/qemu-iotests/133.out | 4 ++++ 3 files changed, 19 insertions(+), 2 deletions(-)
block.c +9 -2
@@ -2942,12 +2942,19 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,if (parent_options) {QemuOpts *opts;QDict *options_copy; +Error *local_err =NULL ;assert (!flags); role->inherit_options (&flags, options, parent_flags, parent_options); options_copy =qdict_clone_shallow (options); opts =qemu_opts_create (&bdrv_runtime_opts,NULL ,0 , &error_abort); -qemu_opts_absorb_qdict (opts, options_copy,NULL ); -update_flags_from_options (&flags, opts); +qemu_opts_absorb_qdict (opts, options_copy, &local_err); +/* Don't call update_flags_from_options() if the options are wrong. +* bdrv_reopen_prepare() will later return an error. */ +if (!local_err) { +update_flags_from_options (&flags, opts); + }else { +error_free (local_err); + }qemu_opts_del (opts);qobject_unref (options_copy); }
tests/qemu-iotests/133 +6 -0
@@ -92,6 +92,12 @@ echo IMGOPTSSYNTAX=false $QEMU_IO -f null-co -c 'reopen' -c 'info' \ "json:{'driver': 'null-co', 'size': 65536}" +echo +echo "=== Check that invalid options are handled correctly ===" +echo + +$QEMU_IO -c 'reopen -o read-only=foo' $TEST_IMG + # success, all done echo "*** done" rm -f $seq.full
tests/qemu-iotests/133.out +4 -0
@@ -24,4 +24,8 @@ Cannot change the option 'driver' format name: null-co format name: null-co + +=== Check that invalid options are handled correctly === + +Parameter 'read-only' expects 'on' or 'off' *** done
Signature · 1 line
2.11.0
PATCH v2 2/14 block: Add bdrv_reopen_set_read_only()
In reply to Alberto GarciaMost callers of bdrv_reopen() only use it to switch a BlockDriverState between read-only and read-write, so this patch adds a new function that does just that.
We also want to get rid of the flags parameter in the bdrv_reopen() API, so this function sets the "read-only" option and passes the original flags (which will then be updated in bdrv_reopen_prepare()).
--- block.c | 17 +++++++++++++++++ include/block/block.h | 2 ++ 2 files changed, 19 insertions(+)
block.c +17 -0
@@ -3098,6 +3098,23 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)return ret; } +int bdrv_reopen_set_read_only (BlockDriverState *bs,bool read_only, +Error **errp) +{ +int ret; +BlockReopenQueue *queue; +QDict *opts =qdict_new (); + +qdict_put_bool (opts,BDRV_OPT_READ_ONLY , read_only); + +bdrv_subtree_drained_begin (bs); + queue =bdrv_reopen_queue (NULL , bs, opts,bdrv_get_flags (bs)); + ret =bdrv_reopen_multiple (bdrv_get_aio_context (bs), queue, errp); +bdrv_subtree_drained_end (bs); + +return ret; +} +static BlockReopenQueueEntry *find_parent_in_reopen_queue (BlockReopenQueue *q,BdrvChild *c) {
include/block/block.h +2 -0
@@ -301,6 +301,8 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, QDict *options, int flags);int bdrv_reopen_multiple (AioContext *ctx,BlockReopenQueue *bs_queue,Error **errp);int bdrv_reopen (BlockDriverState *bs,int bdrv_flags,Error **errp); +int bdrv_reopen_set_read_only (BlockDriverState *bs,bool read_only, +Error **errp);int bdrv_reopen_prepare (BDRVReopenState *reopen_state,BlockReopenQueue *queue,Error **errp);void bdrv_reopen_commit (BDRVReopenState *reopen_state);
Signature · 1 line
2.11.0
PATCH v2 3/14 block: Use bdrv_reopen_set_read_only() in bdrv_backing_update_filename()
In reply to Alberto GarciaThis patch replaces the bdrv_reopen() calls that set and remove the BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
--- block.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
block.c +5 -5
@@ -1058,11 +1058,11 @@ static int bdrv_backing_update_filename(BdrvChild *c, BlockDriverState *base, const char *filename, Error **errp) { BlockDriverState *parent = c->opaque; -int orig_flags =bdrv_get_flags (parent); +bool read_only =bdrv_is_read_only (parent);int ret; -if (!(orig_flags &BDRV_O_RDWR )) { - ret =bdrv_reopen (parent, orig_flags |BDRV_O_RDWR , errp); +if (read_only) { + ret =bdrv_reopen_set_read_only (parent, false, errp);if (ret <0 ) {return ret; }
@@ -1074,8 +1074,8 @@ static int bdrv_backing_update_filename(BdrvChild *c, BlockDriverState *base,error_setg_errno (errp, -ret,"Could not update backing file link" ); } -if (!(orig_flags &BDRV_O_RDWR )) { -bdrv_reopen (parent, orig_flags,NULL ); +if (read_only) { +bdrv_reopen_set_read_only (parent, true,NULL ); }return ret;
Signature · 1 line
2.11.0
PATCH v2 4/14 block: Use bdrv_reopen_set_read_only() in commit_start/complete()
In reply to Alberto GarciaThis patch replaces the bdrv_reopen() calls that set and remove the BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
--- block/commit.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
block/commit.c +6 -10
@@ -38,7 +38,7 @@ typedef struct CommitBlockJob {BlockBackend *base;BlockDriverState *base_bs;BlockdevOnError on_error; -int base_flags; +bool base_read_only;char *backing_file_str; } CommitBlockJob;
@@ -124,8 +124,8 @@ static void commit_clean(Job *job)/* restore base open flags here if appropriate (e.g., change the base back * to r/o). These reopens do not need to be atomic, since we won't abort * even on failure here */ -if (s->base_flags !=bdrv_get_flags (s->base_bs)) { -bdrv_reopen (s->base_bs, s->base_flags,NULL ); +if (s->base_read_only) { +bdrv_reopen_set_read_only (s->base_bs, true,NULL ); }g_free (s->backing_file_str);
@@ -264,7 +264,6 @@ void commit_start(const char *job_id, BlockDriverState *bs, const char *filter_node_name, Error **errp) { CommitBlockJob *s; -int orig_base_flags;BlockDriverState *iter;BlockDriverState *commit_top_bs =NULL ;Error *local_err =NULL ;
@@ -283,11 +282,9 @@ void commit_start(const char *job_id, BlockDriverState *bs, }/* convert base to r/w, if necessary */ - orig_base_flags =bdrv_get_flags (base); -if (!(orig_base_flags &BDRV_O_RDWR )) { -bdrv_reopen (base, orig_base_flags |BDRV_O_RDWR , &local_err); -if (local_err !=NULL ) { -error_propagate (errp, local_err); + s->base_read_only =bdrv_is_read_only (base); +if (s->base_read_only) { +if (bdrv_reopen_set_read_only (base, false, errp) !=0 ) { goto fail; } }
@@ -363,7 +360,6 @@ void commit_start(const char *job_id, BlockDriverState *bs, goto fail; } - s->base_flags = orig_base_flags; s->backing_file_str =g_strdup (backing_file_str); s->on_error = on_error;
Signature · 1 line
2.11.0
PATCH v2 5/14 block: Use bdrv_reopen_set_read_only() in bdrv_commit()
In reply to Alberto GarciaThis patch replaces the bdrv_reopen() calls that set and remove the BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
--- block/commit.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
block/commit.c +3 -4
@@ -391,7 +391,7 @@ int bdrv_commit(BlockDriverState *bs)BlockDriverState *commit_top_bs =NULL ;BlockDriver *drv = bs->drv;int64_t offset, length, backing_length; -int ro, open_flags; +int ro;int64_t n;int ret =0 ;uint8_t *buf =NULL ;
@@ -410,10 +410,9 @@ int bdrv_commit(BlockDriverState *bs) } ro = bs->backing->bs->read_only; - open_flags = bs->backing->bs->open_flags;if (ro) { -if (bdrv_reopen (bs->backing->bs, open_flags |BDRV_O_RDWR ,NULL )) { +if (bdrv_reopen_set_read_only (bs->backing->bs, false,NULL )) {return -EACCES ; } }
@@ -523,7 +522,7 @@ ro_cleanup:if (ro) {/* ignoring error return here */ -bdrv_reopen (bs->backing->bs, open_flags & ~BDRV_O_RDWR ,NULL ); +bdrv_reopen_set_read_only (bs->backing->bs, true,NULL ); }return ret;
Signature · 1 line
2.11.0
PATCH v2 6/14 block: Use bdrv_reopen_set_read_only() in stream_start/complete()
In reply to Alberto GarciaThis patch replaces the bdrv_reopen() calls that set and remove the BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
--- block/stream.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
block/stream.c +10 -10
@@ -34,7 +34,7 @@ typedef struct StreamBlockJob {BlockDriverState *base;BlockdevOnError on_error;char *backing_file_str; -int bs_flags; +bool bs_read_only; } StreamBlockJob; static int coroutine_fn stream_populate(BlockBackend *blk,
@@ -89,10 +89,10 @@ static void stream_clean(Job *job)BlockDriverState *bs =blk_bs (bjob->blk);/* Reopen the image back in read-only mode if necessary */ -if (s->bs_flags !=bdrv_get_flags (bs)) { +if (s->bs_read_only) {/* Give up write permissions before making it read-only */ blk_set_perm (bjob->blk,0 ,BLK_PERM_ALL , &error_abort); -bdrv_reopen (bs, s->bs_flags,NULL ); +bdrv_reopen_set_read_only (bs, true,NULL ); }g_free (s->backing_file_str);
@@ -226,12 +226,12 @@ void stream_start(const char *job_id, BlockDriverState *bs, {StreamBlockJob *s;BlockDriverState *iter; -int orig_bs_flags; +int bs_read_only;/* Make sure that the image is opened in read-write mode */ - orig_bs_flags =bdrv_get_flags (bs); -if (!(orig_bs_flags &BDRV_O_RDWR )) { -if (bdrv_reopen (bs, orig_bs_flags |BDRV_O_RDWR , errp) !=0 ) { + bs_read_only =bdrv_is_read_only (bs); +if (bs_read_only) { +if (bdrv_reopen_set_read_only (bs, false, errp) !=0 ) {return ; } }
@@ -261,7 +261,7 @@ void stream_start(const char *job_id, BlockDriverState *bs, s->base = base; s->backing_file_str =g_strdup (backing_file_str); - s->bs_flags = orig_bs_flags; + s->bs_read_only = bs_read_only; s->on_error = on_error;trace_stream_start (bs, base, s);
@@ -269,7 +269,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,return ; fail: -if (orig_bs_flags !=bdrv_get_flags (bs)) { -bdrv_reopen (bs, orig_bs_flags,NULL ); +if (bs_read_only) { +bdrv_reopen_set_read_only (bs, true,NULL ); } }
Signature · 1 line
2.11.0
PATCH v2 7/14 block: Use bdrv_reopen_set_read_only() in qmp_change_backing_file()
In reply to Alberto GarciaThis patch replaces the bdrv_reopen() calls that set and remove the BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
--- blockdev.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
blockdev.c +2 -6
@@ -4064,7 +4064,6 @@ void qmp_change_backing_file(const char *device,BlockDriverState *image_bs =NULL ;Error *local_err =NULL ;bool ro; -int open_flags;int ret; bs =qmp_get_root_bs (device, errp);
@@ -4106,13 +4105,10 @@ void qmp_change_backing_file(const char *device, }/* if not r/w, reopen to make r/w */ - open_flags = image_bs->open_flags; ro =bdrv_is_read_only (image_bs);if (ro) { -bdrv_reopen (image_bs, open_flags |BDRV_O_RDWR , &local_err); -if (local_err) { -error_propagate (errp, local_err); +if (bdrv_reopen_set_read_only (image_bs, false, errp) !=0 ) { goto out; } }
@@ -4128,7 +4124,7 @@ void qmp_change_backing_file(const char *device, }if (ro) { -bdrv_reopen (image_bs, open_flags, &local_err); +bdrv_reopen_set_read_only (image_bs, true, &local_err);error_propagate (errp, local_err); }
Signature · 1 line
2.11.0
PATCH v2 8/14 block: Use bdrv_reopen_set_read_only() in external_snapshot_commit()
In reply to Alberto GarciaThis patch replaces the bdrv_reopen() call that set and remove the BDRV_O_RDWR flag with the new bdrv_reopen_set_read_only() function.
--- blockdev.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
blockdev.c +1 -2
@@ -1701,8 +1701,7 @@ static void external_snapshot_commit(BlkActionState *common)* bdrv_reopen_multiple() across all the entries at once, because we * don't want to abort all of them if one of them fails the reopen */ if (!atomic_read (&state->old_bs->copy_on_read)) { -bdrv_reopen (state->old_bs, state->old_bs->open_flags & ~BDRV_O_RDWR , -NULL ); +bdrv_reopen_set_read_only (state->old_bs, true,NULL ); }aio_context_release (aio_context);
Signature · 1 line
2.11.0
PATCH v2 9/14 block: Use bdrv_reopen_set_read_only() in the mirror driver
In reply to Alberto GarciaThe 'block-commit' QMP command is implemented internally using two different drivers. If the source image is the active layer then the mirror driver is used (commit_active_start()), otherwise the commit driver is used (commit_start()).
In both cases the destination image must be put temporarily in read-write mode. This is done correctly in the latter case, but what commit_active_start() does is copy all flags instead.
This patch replaces the bdrv_reopen() calls in that function with bdrv_reopen_set_read_only() so that only the read-only status is changed.
A similar change is made in mirror_exit(), which is also used by the 'drive-mirror' and 'blockdev-mirror' commands.
--- block/mirror.c | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-)
block/mirror.c +12 -7
@@ -672,9 +672,10 @@ static int mirror_exit_common(Job *job)if (s->should_complete && !abort) {BlockDriverState *to_replace = s->to_replace ?: src; +bool ro =bdrv_is_read_only (to_replace); -if (bdrv_get_flags (target_bs) !=bdrv_get_flags (to_replace)) { -bdrv_reopen (target_bs,bdrv_get_flags (to_replace),NULL ); +if (ro !=bdrv_is_read_only (target_bs)) { +bdrv_reopen_set_read_only (target_bs, ro,NULL ); }/* The mirror job has no requests in flight any more, but we need to
@@ -1692,13 +1693,15 @@ void commit_active_start(const char *job_id, BlockDriverState *bs, BlockCompletionFunc *cb, void *opaque, bool auto_complete, Error **errp) { -int orig_base_flags; +bool base_read_only;Error *local_err =NULL ; - orig_base_flags =bdrv_get_flags (base); + base_read_only =bdrv_is_read_only (base); -if (bdrv_reopen (base, bs->open_flags, errp)) { -return ; +if (base_read_only) { +if (bdrv_reopen_set_read_only (base, false, errp) <0 ) { +return ; + } } mirror_start_job(job_id, bs, creation_flags, base, NULL, speed, 0, 0,
@@ -1717,6 +1720,8 @@ void commit_active_start(const char *job_id, BlockDriverState *bs, error_restore_flags:/* ignore error and errp for bdrv_reopen, because we want to propagate * the original error */ -bdrv_reopen (base, orig_base_flags,NULL ); +if (base_read_only) { +bdrv_reopen_set_read_only (base, true,NULL ); + }return ; }
Signature · 1 line
2.11.0
PATCH v2 10/14 block: Drop bdrv_reopen()
In reply to Alberto GarciaNo one is using this function anymore, so we can safely remove it.
--- block.c | 21 --------------------- include/block/block.h | 1 - 2 files changed, 22 deletions(-)
block.c +0 -21
@@ -3077,27 +3077,6 @@ cleanup:return ret; } - -/* Reopen a single BlockDriverState with the specified flags. */ -int bdrv_reopen (BlockDriverState *bs,int bdrv_flags,Error **errp) -{ -int ret =-1 ; -Error *local_err =NULL ; -BlockReopenQueue *queue; - -bdrv_subtree_drained_begin (bs); - - queue =bdrv_reopen_queue (NULL , bs,NULL , bdrv_flags); - ret =bdrv_reopen_multiple (bdrv_get_aio_context (bs), queue, &local_err); -if (local_err !=NULL ) { -error_propagate (errp, local_err); - } - -bdrv_subtree_drained_end (bs); - -return ret; -} -int bdrv_reopen_set_read_only (BlockDriverState *bs,bool read_only,Error **errp) {
include/block/block.h +0 -1
@@ -300,7 +300,6 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, BlockDriverState *bs, QDict *options, int flags);int bdrv_reopen_multiple (AioContext *ctx,BlockReopenQueue *bs_queue,Error **errp); -int bdrv_reopen (BlockDriverState *bs,int bdrv_flags,Error **errp);int bdrv_reopen_set_read_only (BlockDriverState *bs,bool read_only,Error **errp); int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
Signature · 1 line
2.11.0
PATCH v2 11/14 qemu-io: Put flag changes in the options QDict in reopen_f()
In reply to Alberto GarciaWhen reopen_f() puts a block device in the reopen queue, some of the new options are passed using a QDict, but others ("read-only" and the cache options) are passed as flags.
This patch puts those flags in the QDict. This way the flags parameter becomes redundant and we'll be able to get rid of it in a subsequent patch.
--- qemu-io-cmds.c | 27 ++++++++++++++++++++++++++- tests/qemu-iotests/133 | 9 +++++++++ tests/qemu-iotests/133.out | 8 ++++++++ 3 files changed, 43 insertions(+), 1 deletion(-)
qemu-io-cmds.c +26 -1
@@ -10,6 +10,7 @@#include "qemu/osdep.h" #include "qapi/error.h" +#include "qapi/qmp/qdict.h" #include "qemu-io.h" #include "sysemu/block-backend.h" #include "block/block.h"
@@ -1978,6 +1979,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)int flags = bs->open_flags;bool writethrough = !blk_enable_write_cache (blk);bool has_rw_option = false; +bool has_cache_option = false;BlockReopenQueue *brq;Error *local_err =NULL ;
@@ -1989,6 +1991,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv)error_report ("Invalid cache option: %s" , optarg);return -EINVAL ; } + has_cache_option = true;break ;case 'o' :if (!qemu_opts_parse_noisily (&reopen_opts, optarg,0 )) {
@@ -2046,9 +2049,31 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv) } qopts =qemu_opts_find (&reopen_opts,NULL ); - opts = qopts ?qemu_opts_to_qdict (qopts,NULL ) :NULL ; + opts = qopts ?qemu_opts_to_qdict (qopts,NULL ) :qdict_new ();qemu_opts_reset (&reopen_opts); +if (qdict_haskey (opts,BDRV_OPT_READ_ONLY )) { +if (has_rw_option) { +error_report ("Cannot set both -r/-w and '" BDRV_OPT_READ_ONLY "'" ); +qobject_unref (opts); +return -EINVAL ; + } + }else { +qdict_put_bool (opts,BDRV_OPT_READ_ONLY , !(flags &BDRV_O_RDWR )); + } + +if (qdict_haskey (opts,BDRV_OPT_CACHE_DIRECT ) || +qdict_haskey (opts,BDRV_OPT_CACHE_NO_FLUSH )) { +if (has_cache_option) { +error_report ("Cannot set both -c and the cache options" ); +qobject_unref (opts); +return -EINVAL ; + } + }else { +qdict_put_bool (opts,BDRV_OPT_CACHE_DIRECT , flags &BDRV_O_NOCACHE ); +qdict_put_bool (opts,BDRV_OPT_CACHE_NO_FLUSH , flags &BDRV_O_NO_FLUSH ); + } +bdrv_subtree_drained_begin (bs); brq =bdrv_reopen_queue (NULL , bs, opts, flags);bdrv_reopen_multiple (bdrv_get_aio_context (bs), brq, &local_err);
tests/qemu-iotests/133 +9 -0
@@ -98,6 +98,15 @@ echo $QEMU_IO -c 'reopen -o read-only=foo' $TEST_IMG +echo +echo "=== Check that mixing -c/-r/-w and their corresponding options is forbidden ===" +echo + +$QEMU_IO -c 'reopen -r -o read-only=on' $TEST_IMG +$QEMU_IO -c 'reopen -w -o read-only=on' $TEST_IMG +$QEMU_IO -c 'reopen -c none -o cache.direct=on' $TEST_IMG +$QEMU_IO -c 'reopen -c writeback -o cache.direct=on' $TEST_IMG +$QEMU_IO -c 'reopen -c directsync -o cache.no-flush=on' $TEST_IMG # success, all done echo "*** done" rm -f $seq.full
tests/qemu-iotests/133.out +8 -0
@@ -28,4 +28,12 @@ format name: null-co === Check that invalid options are handled correctly === Parameter 'read-only' expects 'on' or 'off' + +=== Check that mixing -c/-r/-w and their corresponding options is forbidden === + +Cannot set both -r/-w and 'read-only' +Cannot set both -r/-w and 'read-only' +Cannot set both -c and the cache options +Cannot set both -c and the cache options +Cannot set both -c and the cache options *** done
Signature · 1 line
2.11.0
PATCH v2 12/14 block: Clean up reopen_backing_file() in block/replication.c
In reply to Alberto GarciaThis function is used to put the hidden and secondary disks in read-write mode before launching the backup job, and back in read-only mode afterwards.
This patch does the following changes:
- Use an options QDict with the "read-only" option instead of passing the changes as flags only.
- Simplify the code (it was unnecessarily complicated and verbose).
- Fix a bug due to which the secondary disk was not being put back in read-only mode when writable=false (because in this case orig_secondary_flags always had the BDRV_O_RDWR flag set).
- Stop clearing the BDRV_O_INACTIVE flag.
The flags parameter to bdrv_reopen_queue() becomes redundant and we'll be able to get rid of it in a subsequent patch.
--- block/replication.c | 45 +++++++++++++++++++++------------------------ 1 file changed, 21 insertions(+), 24 deletions(-)
block/replication.c +21 -24
@@ -20,6 +20,7 @@#include "block/block_backup.h" #include "sysemu/block-backend.h" #include "qapi/error.h" +#include "qapi/qmp/qdict.h" #include "replication.h" typedef enum {
@@ -39,8 +40,8 @@ typedef struct BDRVReplicationState {char *top_id;ReplicationState *rs;Error *blocker; -int orig_hidden_flags; -int orig_secondary_flags; +bool orig_hidden_read_only; +bool orig_secondary_read_only;int error; } BDRVReplicationState;
@@ -371,44 +372,40 @@ static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp) } } +/* This function is supposed to be called twice: +* first with writable = true, then with writable = false. +* The first call puts s->hidden_disk and s->secondary_disk in +* r/w mode, and the second puts them back in their original state. +*/ static void reopen_backing_file (BlockDriverState *bs,bool writable,Error **errp) {BDRVReplicationState *s = bs->opaque;BlockReopenQueue *reopen_queue =NULL ; -int orig_hidden_flags, orig_secondary_flags; -int new_hidden_flags, new_secondary_flags;Error *local_err =NULL ;if (writable) { - orig_hidden_flags = s->orig_hidden_flags = -bdrv_get_flags (s->hidden_disk->bs); - new_hidden_flags = (orig_hidden_flags |BDRV_O_RDWR ) & - ~BDRV_O_INACTIVE ; - orig_secondary_flags = s->orig_secondary_flags = -bdrv_get_flags (s->secondary_disk->bs); - new_secondary_flags = (orig_secondary_flags |BDRV_O_RDWR ) & - ~BDRV_O_INACTIVE ; - }else { - orig_hidden_flags = (s->orig_hidden_flags |BDRV_O_RDWR ) & - ~BDRV_O_INACTIVE ; - new_hidden_flags = s->orig_hidden_flags; - orig_secondary_flags = (s->orig_secondary_flags |BDRV_O_RDWR ) & - ~BDRV_O_INACTIVE ; - new_secondary_flags = s->orig_secondary_flags; + s->orig_hidden_read_only =bdrv_is_read_only (s->hidden_disk->bs); + s->orig_secondary_read_only =bdrv_is_read_only (s->secondary_disk->bs); }bdrv_subtree_drained_begin (s->hidden_disk->bs);bdrv_subtree_drained_begin (s->secondary_disk->bs); -if (orig_hidden_flags != new_hidden_flags) { - reopen_queue =bdrv_reopen_queue (reopen_queue, s->hidden_disk->bs,NULL , - new_hidden_flags); +if (s->orig_hidden_read_only) { +int flags =bdrv_get_flags (s->hidden_disk->bs); +QDict *opts =qdict_new (); +qdict_put_bool (opts,BDRV_OPT_READ_ONLY , !writable); + reopen_queue =bdrv_reopen_queue (reopen_queue, s->hidden_disk->bs, + opts, flags); } -if (!(orig_secondary_flags &BDRV_O_RDWR )) { +if (s->orig_secondary_read_only) { +int flags =bdrv_get_flags (s->secondary_disk->bs); +QDict *opts =qdict_new (); +qdict_put_bool (opts,BDRV_OPT_READ_ONLY , !writable); reopen_queue =bdrv_reopen_queue (reopen_queue, s->secondary_disk->bs, -NULL , new_secondary_flags); + opts, flags); }if (reopen_queue) {
Signature · 1 line
2.11.0
PATCH v2 13/14 block: Remove flags parameter from bdrv_reopen_queue()
In reply to Alberto GarciaNow that all callers are passing all flag changes as QDict options, the flags parameter is no longer necessary, so we can get rid of it.
--- block.c | 5 +++-- block/replication.c | 6 ++---- include/block/block.h | 3 +-- qemu-io-cmds.c | 2 +- 4 files changed, 7 insertions(+), 9 deletions(-)
block.c +3 -2
@@ -3012,8 +3012,9 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,BlockReopenQueue *bdrv_reopen_queue (BlockReopenQueue *bs_queue,BlockDriverState *bs, -QDict *options,int flags) +QDict *options) { +int flags =bdrv_get_flags (bs);return bdrv_reopen_queue_child (bs_queue, bs, options, flags,NULL ,NULL ,0 ); }
@@ -3087,7 +3088,7 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only,qdict_put_bool (opts,BDRV_OPT_READ_ONLY , read_only);bdrv_subtree_drained_begin (bs); - queue =bdrv_reopen_queue (NULL , bs, opts,bdrv_get_flags (bs)); + queue =bdrv_reopen_queue (NULL , bs, opts); ret =bdrv_reopen_multiple (bdrv_get_aio_context (bs), queue, errp);bdrv_subtree_drained_end (bs);
block/replication.c +2 -4
@@ -393,19 +393,17 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable,bdrv_subtree_drained_begin (s->secondary_disk->bs);if (s->orig_hidden_read_only) { -int flags =bdrv_get_flags (s->hidden_disk->bs);QDict *opts =qdict_new ();qdict_put_bool (opts,BDRV_OPT_READ_ONLY , !writable); reopen_queue =bdrv_reopen_queue (reopen_queue, s->hidden_disk->bs, - opts, flags); + opts); }if (s->orig_secondary_read_only) { -int flags =bdrv_get_flags (s->secondary_disk->bs);QDict *opts =qdict_new ();qdict_put_bool (opts,BDRV_OPT_READ_ONLY , !writable); reopen_queue =bdrv_reopen_queue (reopen_queue, s->secondary_disk->bs, - opts, flags); + opts); }if (reopen_queue) {
include/block/block.h +1 -2
@@ -297,8 +297,7 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,BlockDriverState *bdrv_new_open_driver (BlockDriver *drv,const char *node_name,int flags,Error **errp);BlockReopenQueue *bdrv_reopen_queue (BlockReopenQueue *bs_queue, -BlockDriverState *bs, -QDict *options,int flags); +BlockDriverState *bs,QDict *options);int bdrv_reopen_multiple (AioContext *ctx,BlockReopenQueue *bs_queue,Error **errp);int bdrv_reopen_set_read_only (BlockDriverState *bs,bool read_only,Error **errp);
qemu-io-cmds.c +1 -1
@@ -2075,7 +2075,7 @@ static int reopen_f(BlockBackend *blk, int argc, char **argv) }bdrv_subtree_drained_begin (bs); - brq =bdrv_reopen_queue (NULL , bs, opts, flags); + brq =bdrv_reopen_queue (NULL , bs, opts);bdrv_reopen_multiple (bdrv_get_aio_context (bs), brq, &local_err);bdrv_subtree_drained_end (bs);
Signature · 1 line
2.11.0
PATCH v2 14/14 block: Stop passing flags to bdrv_reopen_queue_child()
In reply to Alberto GarciaNow that all callers are passing the new options using the QDict we no longer need the 'flags' parameter.
This patch makes the following changes:
1) The update_options_from_flags() call is no longer necessary so it can be removed.
2) The update_flags_from_options() call is now used in all cases, and is moved down a few lines so it happens after the options QDict contains the final set of values.
3) The flags parameter is removed. Now the flags are initialized using the current value (for the top-level node) or the parent flags (after inherit_options()). In both cases the initial values are updated to reflect the new options in the QDict. This happens in bdrv_reopen_queue_child() (as explained above) and in bdrv_reopen_prepare().
--- block.c | 62 ++++++++++++++++++++++++++------------------------------------ 1 file changed, 26 insertions(+), 36 deletions(-)
block.c +26 -36
@@ -2876,7 +2876,6 @@ BlockDriverState *bdrv_open(const char *filename, const char *reference,static BlockReopenQueue *bdrv_reopen_queue_child (BlockReopenQueue *bs_queue,BlockDriverState *bs,QDict *options, -int flags,const BdrvChildRole *role,QDict *parent_options,int parent_flags)
@@ -2885,7 +2884,10 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,BlockReopenQueueEntry *bs_entry;BdrvChild *child; -QDict *old_options, *explicit_options; +QDict *old_options, *explicit_options, *options_copy; +int flags; +QemuOpts *opts; +Error *local_err =NULL ;/* Make sure that the caller remembered to use a drained section. This is * important to avoid graph changes between the recursive queuing here and
@@ -2911,22 +2913,11 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,/* * Precedence of options: * 1. Explicitly passed in options (highest) -* 2. Set in flags (only for top level) -* 3. Retained from explicitly set options of bs -* 4. Inherited from parent node -* 5. Retained from effective options of bs +* 2. Retained from explicitly set options of bs +* 3. Inherited from parent node +* 4. Retained from effective options of bs */ -if (!parent_options) { -/* -* Any setting represented by flags is always updated. If the -* corresponding QDict option is set, it takes precedence. Otherwise -* the flag is translated into a QDict option. The old setting of bs is -* not considered. -*/ -update_options_from_flags (options, flags); - } -/* Old explicitly set values (don't overwrite by inherited value) */ if (bs_entry) { old_options =qdict_clone_shallow (bs_entry->state.explicit_options);
@@ -2940,23 +2931,10 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,/* Inherit from parent node */ if (parent_options) { -QemuOpts *opts; -QDict *options_copy; -Error *local_err =NULL ; -assert (!flags); + flags =0 ; role->inherit_options (&flags, options, parent_flags, parent_options); - options_copy =qdict_clone_shallow (options); - opts =qemu_opts_create (&bdrv_runtime_opts,NULL ,0 , &error_abort); -qemu_opts_absorb_qdict (opts, options_copy, &local_err); -/* Don't call update_flags_from_options() if the options are wrong. -* bdrv_reopen_prepare() will later return an error. */ -if (!local_err) { -update_flags_from_options (&flags, opts); - }else { -error_free (local_err); - } -qemu_opts_del (opts); -qobject_unref (options_copy); + }else { + flags =bdrv_get_flags (bs); }/* Old values are used for options that aren't set yet */
@@ -2964,6 +2942,20 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,bdrv_join_options (bs, options, old_options);qobject_unref (old_options); +/* We have the final set of options so let's update the flags */ + options_copy =qdict_clone_shallow (options); + opts =qemu_opts_create (&bdrv_runtime_opts,NULL ,0 , &error_abort); +qemu_opts_absorb_qdict (opts, options_copy, &local_err); +/* Don't call update_flags_from_options() if the options are wrong. +* bdrv_reopen_prepare() will later return an error. */ +if (!local_err) { +update_flags_from_options (&flags, opts); + }else { +error_free (local_err); + } +qemu_opts_del (opts); +qobject_unref (options_copy); +/* bdrv_open_inherit() sets and clears some additional flags internally */ flags &= ~BDRV_O_PROTOCOL ;if (flags &BDRV_O_RDWR ) {
@@ -3003,7 +2995,7 @@ static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,qdict_extract_subqdict (options, &new_child_options, child_key_dot);g_free (child_key_dot); -bdrv_reopen_queue_child (bs_queue, child->bs, new_child_options,0 , +bdrv_reopen_queue_child (bs_queue, child->bs, new_child_options, child->role, options, flags); }
@@ -3014,9 +3006,7 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue, BlockDriverState *bs, QDict *options) { -int flags =bdrv_get_flags (bs); -return bdrv_reopen_queue_child (bs_queue, bs, options, flags, -NULL ,NULL ,0 ); +return bdrv_reopen_queue_child (bs_queue, bs, options,NULL ,NULL ,0 ); }/*
Signature · 1 line
2.11.0