* [Qemu-devel] [PATCH v2 01/13] block: Allow specifying driver-specific options to reopen
2015-09-04 17:18 [Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options Kevin Wolf
@ 2015-09-04 17:18 ` Kevin Wolf
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 02/13] qemu-io: Remove duplicate 'open' error message Kevin Wolf
` (11 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Kevin Wolf @ 2015-09-04 17:18 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 42 +++++++++++++++++++++++++++++++++++++++---
block/commit.c | 4 ++--
include/block/block.h | 4 +++-
3 files changed, 44 insertions(+), 6 deletions(-)
diff --git a/block.c b/block.c
index 090923c..a2cd9b8 100644
--- a/block.c
+++ b/block.c
@@ -1660,6 +1660,9 @@ typedef struct BlockReopenQueueEntry {
*
* bs is the BlockDriverState to add to the reopen queue.
*
+ * options contains the changed options for the associated bs
+ * (the BlockReopenQueue takes ownership)
+ *
* flags contains the open flags for the associated bs
*
* returns a pointer to bs_queue, which is either the newly allocated
@@ -1667,18 +1670,28 @@ typedef struct BlockReopenQueueEntry {
*
*/
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
- BlockDriverState *bs, int flags)
+ BlockDriverState *bs,
+ QDict *options, int flags)
{
assert(bs != NULL);
BlockReopenQueueEntry *bs_entry;
BdrvChild *child;
+ QDict *old_options;
if (bs_queue == NULL) {
bs_queue = g_new0(BlockReopenQueue, 1);
QSIMPLEQ_INIT(bs_queue);
}
+ if (!options) {
+ options = qdict_new();
+ }
+
+ old_options = qdict_clone_shallow(bs->options);
+ qdict_join(options, old_options, false);
+ QDECREF(old_options);
+
/* bdrv_open() masks this flag out */
flags &= ~BDRV_O_PROTOCOL;
@@ -1690,13 +1703,15 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
}
child_flags = child->role->inherit_flags(flags);
- bdrv_reopen_queue(bs_queue, child->bs, child_flags);
+ /* TODO Pass down child flags (backing.*, extents.*, ...) */
+ bdrv_reopen_queue(bs_queue, child->bs, NULL, child_flags);
}
bs_entry = g_new0(BlockReopenQueueEntry, 1);
QSIMPLEQ_INSERT_TAIL(bs_queue, bs_entry, entry);
bs_entry->state.bs = bs;
+ bs_entry->state.options = options;
bs_entry->state.flags = flags;
return bs_queue;
@@ -1749,6 +1764,7 @@ cleanup:
if (ret && bs_entry->prepared) {
bdrv_reopen_abort(&bs_entry->state);
}
+ QDECREF(bs_entry->state.options);
g_free(bs_entry);
}
g_free(bs_queue);
@@ -1761,7 +1777,7 @@ int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp)
{
int ret = -1;
Error *local_err = NULL;
- BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, bdrv_flags);
+ BlockReopenQueue *queue = bdrv_reopen_queue(NULL, bs, NULL, bdrv_flags);
ret = bdrv_reopen_multiple(queue, &local_err);
if (local_err != NULL) {
@@ -1837,6 +1853,26 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue,
goto error;
}
+ /* Options that are not handled are only okay if they are unchanged
+ * compared to the old state. It is expected that some options are only
+ * used for the initial open, but not reopen (e.g. filename) */
+ if (qdict_size(reopen_state->options)) {
+ const QDictEntry *entry = qdict_first(reopen_state->options);
+
+ do {
+ QString *new_obj = qobject_to_qstring(entry->value);
+ const char *new = qstring_get_str(new_obj);
+ const char *old = qdict_get_try_str(reopen_state->bs->options,
+ entry->key);
+
+ if (!old || strcmp(new, old)) {
+ error_setg(errp, "Cannot change the option '%s'", entry->key);
+ ret = -EINVAL;
+ goto error;
+ }
+ } while ((entry = qdict_next(reopen_state->options, entry)));
+ }
+
ret = 0;
error:
diff --git a/block/commit.c b/block/commit.c
index 7312a5b..d12e26f 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -236,11 +236,11 @@ void commit_start(BlockDriverState *bs, BlockDriverState *base,
/* convert base & overlay_bs to r/w, if necessary */
if (!(orig_base_flags & BDRV_O_RDWR)) {
- reopen_queue = bdrv_reopen_queue(reopen_queue, base,
+ reopen_queue = bdrv_reopen_queue(reopen_queue, base, NULL,
orig_base_flags | BDRV_O_RDWR);
}
if (!(orig_overlay_flags & BDRV_O_RDWR)) {
- reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs,
+ reopen_queue = bdrv_reopen_queue(reopen_queue, overlay_bs, NULL,
orig_overlay_flags | BDRV_O_RDWR);
}
if (reopen_queue) {
diff --git a/include/block/block.h b/include/block/block.h
index 608cd4e..555a2ce 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -147,6 +147,7 @@ typedef QSIMPLEQ_HEAD(BlockReopenQueue, BlockReopenQueueEntry) BlockReopenQueue;
typedef struct BDRVReopenState {
BlockDriverState *bs;
int flags;
+ QDict *options;
void *opaque;
} BDRVReopenState;
@@ -221,7 +222,8 @@ int bdrv_open(BlockDriverState **pbs, const char *filename,
const char *reference, QDict *options, int flags,
BlockDriver *drv, Error **errp);
BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
- BlockDriverState *bs, int flags);
+ BlockDriverState *bs,
+ QDict *options, int flags);
int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
int bdrv_reopen(BlockDriverState *bs, int bdrv_flags, Error **errp);
int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v2 02/13] qemu-io: Remove duplicate 'open' error message
2015-09-04 17:18 [Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options Kevin Wolf
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 01/13] block: Allow specifying driver-specific options to reopen Kevin Wolf
@ 2015-09-04 17:18 ` Kevin Wolf
2015-09-04 18:10 ` Max Reitz
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 03/13] qemu-io: Add command 'reopen' Kevin Wolf
` (10 subsequent siblings)
12 siblings, 1 reply; 22+ messages in thread
From: Kevin Wolf @ 2015-09-04 17:18 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
qemu_opts_parse_noisily() already prints an error message with the exact
reason why the parsing failed. No need to add another less specific one.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qemu-io.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/qemu-io.c b/qemu-io.c
index f1e3a67..269f17c 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -156,7 +156,6 @@ static int open_f(BlockBackend *blk, int argc, char **argv)
break;
case 'o':
if (!qemu_opts_parse_noisily(&empty_opts, optarg, false)) {
- printf("could not parse option list -- %s\n", optarg);
qemu_opts_reset(&empty_opts);
return 0;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v2 03/13] qemu-io: Add command 'reopen'
2015-09-04 17:18 [Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options Kevin Wolf
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 01/13] block: Allow specifying driver-specific options to reopen Kevin Wolf
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 02/13] qemu-io: Remove duplicate 'open' error message Kevin Wolf
@ 2015-09-04 17:18 ` Kevin Wolf
2015-09-04 18:33 ` Max Reitz
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 04/13] qcow2: Improve error message Kevin Wolf
` (9 subsequent siblings)
12 siblings, 1 reply; 22+ messages in thread
From: Kevin Wolf @ 2015-09-04 17:18 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
qemu-io-cmds.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 90 insertions(+)
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 53477e1..d6572a8 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1979,6 +1979,95 @@ static const cmdinfo_t map_cmd = {
.oneline = "prints the allocated areas of a file",
};
+static void reopen_help(void)
+{
+ printf(
+"\n"
+" Changes the open options of an already opened image\n"
+"\n"
+" Example:\n"
+" 'reopen -o lazy-refcounts=on' - activates lazy refcount writeback on a qcow2 image\n"
+"\n"
+" -r, -- Reopen the image read-only\n"
+" -c, -- Change the cache mode to the given value\n"
+" -o, -- Changes block driver options (cf. 'open' command)\n"
+"\n");
+}
+
+static int reopen_f(BlockBackend *blk, int argc, char **argv);
+
+static QemuOptsList reopen_opts = {
+ .name = "reopen",
+ .merge_lists = true,
+ .head = QTAILQ_HEAD_INITIALIZER(reopen_opts.head),
+ .desc = {
+ /* no elements => accept any params */
+ { /* end of list */ }
+ },
+};
+
+static const cmdinfo_t reopen_cmd = {
+ .name = "reopen",
+ .argmin = 0,
+ .argmax = -1,
+ .cfunc = reopen_f,
+ .args = "[-r] [-c cache] [-o options]",
+ .oneline = "reopens an image with new options",
+ .help = reopen_help,
+};
+
+static int reopen_f(BlockBackend *blk, int argc, char **argv)
+{
+ BlockDriverState *bs = blk_bs(blk);
+ QemuOpts *qopts;
+ QDict *opts;
+ int c;
+ int flags = bs->open_flags;
+
+ BlockReopenQueue *brq;
+ Error *local_err = NULL;
+
+ while ((c = getopt(argc, argv, "c:o:r")) != -1) {
+ switch (c) {
+ case 'c':
+ if (bdrv_parse_cache_flags(optarg, &flags) < 0) {
+ error_report("Invalid cache option: %s", optarg);
+ return 0;
+ }
+ break;
+ case 'o':
+ if (!qemu_opts_parse_noisily(&reopen_opts, optarg, 0)) {
+ qemu_opts_reset(&reopen_opts);
+ return 0;
+ }
+ break;
+ case 'r':
+ flags &= ~BDRV_O_RDWR;
+ break;
+ default:
+ qemu_opts_reset(&reopen_opts);
+ return qemuio_command_usage(&reopen_cmd);
+ }
+ }
+
+ if (optind != argc) {
+ qemu_opts_reset(&reopen_opts);
+ return qemuio_command_usage(&reopen_cmd);
+ }
+
+ qopts = qemu_opts_find(&reopen_opts, NULL);
+ opts = qopts ? qemu_opts_to_qdict(qopts, NULL) : NULL;
+ qemu_opts_reset(&reopen_opts);
+
+ brq = bdrv_reopen_queue(NULL, bs, opts, flags);
+ bdrv_reopen_multiple(brq, &local_err);
+ if (local_err) {
+ error_report_err(local_err);
+ }
+
+ return 0;
+}
+
static int break_f(BlockBackend *blk, int argc, char **argv)
{
int ret;
@@ -2266,6 +2355,7 @@ static void __attribute((constructor)) init_qemuio_commands(void)
qemuio_add_command(&discard_cmd);
qemuio_add_command(&alloc_cmd);
qemuio_add_command(&map_cmd);
+ qemuio_add_command(&reopen_cmd);
qemuio_add_command(&break_cmd);
qemuio_add_command(&remove_break_cmd);
qemuio_add_command(&resume_cmd);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v2 04/13] qcow2: Improve error message
2015-09-04 17:18 [Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options Kevin Wolf
` (2 preceding siblings ...)
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 03/13] qemu-io: Add command 'reopen' Kevin Wolf
@ 2015-09-04 17:18 ` Kevin Wolf
2015-09-04 18:34 ` Max Reitz
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 05/13] qcow2: Factor out qcow2_update_options() Kevin Wolf
` (8 subsequent siblings)
12 siblings, 1 reply; 22+ messages in thread
From: Kevin Wolf @ 2015-09-04 17:18 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Eric says that "any" sounds better than "either", and my non-native
feeling says the same, so let's change it.
Suggested-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 76c331b..e120ed3 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -971,7 +971,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
overlap_check_template = QCOW2_OL_ALL;
} else {
error_setg(errp, "Unsupported value '%s' for qcow2 option "
- "'overlap-check'. Allowed are either of the following: "
+ "'overlap-check'. Allowed are any of the following: "
"none, constant, cached, all", opt_overlap_check);
ret = -EINVAL;
goto fail;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v2 05/13] qcow2: Factor out qcow2_update_options()
2015-09-04 17:18 [Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options Kevin Wolf
` (3 preceding siblings ...)
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 04/13] qcow2: Improve error message Kevin Wolf
@ 2015-09-04 17:18 ` Kevin Wolf
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 06/13] qcow2: Move qcow2_update_options() call up Kevin Wolf
` (7 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Kevin Wolf @ 2015-09-04 17:18 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Eventually we want to be able to change options at runtime. As a first
step towards that goal, separate some option handling code from the
general initialisation code in qcow2_open().
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 135 +++++++++++++++++++++++++++++++++-------------------------
1 file changed, 76 insertions(+), 59 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index e120ed3..dc07d35 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -541,6 +541,80 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
}
}
+static int qcow2_update_options(BlockDriverState *bs, QemuOpts *opts,
+ int flags, Error **errp)
+{
+ BDRVQcowState *s = bs->opaque;
+ const char *opt_overlap_check, *opt_overlap_check_template;
+ int overlap_check_template = 0;
+ int i;
+ int ret;
+
+ s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
+ (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
+
+ s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
+ s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
+ s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
+ qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
+ flags & BDRV_O_UNMAP);
+ s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
+ qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
+ s->discard_passthrough[QCOW2_DISCARD_OTHER] =
+ qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
+
+ opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
+ opt_overlap_check_template = qemu_opt_get(opts, QCOW2_OPT_OVERLAP_TEMPLATE);
+ if (opt_overlap_check_template && opt_overlap_check &&
+ strcmp(opt_overlap_check_template, opt_overlap_check))
+ {
+ error_setg(errp, "Conflicting values for qcow2 options '"
+ QCOW2_OPT_OVERLAP "' ('%s') and '" QCOW2_OPT_OVERLAP_TEMPLATE
+ "' ('%s')", opt_overlap_check, opt_overlap_check_template);
+ ret = -EINVAL;
+ goto fail;
+ }
+ if (!opt_overlap_check) {
+ opt_overlap_check = opt_overlap_check_template ?: "cached";
+ }
+
+ if (!strcmp(opt_overlap_check, "none")) {
+ overlap_check_template = 0;
+ } else if (!strcmp(opt_overlap_check, "constant")) {
+ overlap_check_template = QCOW2_OL_CONSTANT;
+ } else if (!strcmp(opt_overlap_check, "cached")) {
+ overlap_check_template = QCOW2_OL_CACHED;
+ } else if (!strcmp(opt_overlap_check, "all")) {
+ overlap_check_template = QCOW2_OL_ALL;
+ } else {
+ error_setg(errp, "Unsupported value '%s' for qcow2 option "
+ "'overlap-check'. Allowed are any of the following: "
+ "none, constant, cached, all", opt_overlap_check);
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ s->overlap_check = 0;
+ for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
+ /* overlap-check defines a template bitmask, but every flag may be
+ * overwritten through the associated boolean option */
+ s->overlap_check |=
+ qemu_opt_get_bool(opts, overlap_bool_option_names[i],
+ overlap_check_template & (1 << i)) << i;
+ }
+
+ if (s->use_lazy_refcounts && s->qcow_version < 3) {
+ error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
+ "qemu 1.1 compatibility level");
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ ret = 0;
+fail:
+ return ret;
+}
+
static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
@@ -552,8 +626,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
Error *local_err = NULL;
uint64_t ext_end;
uint64_t l1_vm_state_index;
- const char *opt_overlap_check, *opt_overlap_check_template;
- int overlap_check_template = 0;
uint64_t l2_cache_size, refcount_cache_size;
ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
@@ -933,69 +1005,14 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
}
/* Enable lazy_refcounts according to image and command line options */
- s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
- (s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
-
- s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
- s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
- s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
- qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
- flags & BDRV_O_UNMAP);
- s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
- qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
- s->discard_passthrough[QCOW2_DISCARD_OTHER] =
- qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
-
- opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
- opt_overlap_check_template = qemu_opt_get(opts, QCOW2_OPT_OVERLAP_TEMPLATE);
- if (opt_overlap_check_template && opt_overlap_check &&
- strcmp(opt_overlap_check_template, opt_overlap_check))
- {
- error_setg(errp, "Conflicting values for qcow2 options '"
- QCOW2_OPT_OVERLAP "' ('%s') and '" QCOW2_OPT_OVERLAP_TEMPLATE
- "' ('%s')", opt_overlap_check, opt_overlap_check_template);
- ret = -EINVAL;
- goto fail;
- }
- if (!opt_overlap_check) {
- opt_overlap_check = opt_overlap_check_template ?: "cached";
- }
-
- if (!strcmp(opt_overlap_check, "none")) {
- overlap_check_template = 0;
- } else if (!strcmp(opt_overlap_check, "constant")) {
- overlap_check_template = QCOW2_OL_CONSTANT;
- } else if (!strcmp(opt_overlap_check, "cached")) {
- overlap_check_template = QCOW2_OL_CACHED;
- } else if (!strcmp(opt_overlap_check, "all")) {
- overlap_check_template = QCOW2_OL_ALL;
- } else {
- error_setg(errp, "Unsupported value '%s' for qcow2 option "
- "'overlap-check'. Allowed are any of the following: "
- "none, constant, cached, all", opt_overlap_check);
- ret = -EINVAL;
+ ret = qcow2_update_options(bs, opts, flags, errp);
+ if (ret < 0) {
goto fail;
}
- s->overlap_check = 0;
- for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
- /* overlap-check defines a template bitmask, but every flag may be
- * overwritten through the associated boolean option */
- s->overlap_check |=
- qemu_opt_get_bool(opts, overlap_bool_option_names[i],
- overlap_check_template & (1 << i)) << i;
- }
-
qemu_opts_del(opts);
opts = NULL;
- if (s->use_lazy_refcounts && s->qcow_version < 3) {
- error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
- "qemu 1.1 compatibility level");
- ret = -EINVAL;
- goto fail;
- }
-
#ifdef DEBUG_ALLOC
{
BdrvCheckResult result = {0};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v2 06/13] qcow2: Move qcow2_update_options() call up
2015-09-04 17:18 [Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options Kevin Wolf
` (4 preceding siblings ...)
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 05/13] qcow2: Factor out qcow2_update_options() Kevin Wolf
@ 2015-09-04 17:18 ` Kevin Wolf
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 07/13] qcow2: Move rest of option handling to qcow2_update_options() Kevin Wolf
` (6 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Kevin Wolf @ 2015-09-04 17:18 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
qcow2_update_options() only updates some variables in BDRVQcowState and
doesn't really depend on other parts of it being initialised yet, so it
can be moved so that it immediately follows the other half of option
handling code in qcow2_open().
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index dc07d35..2903481 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -920,6 +920,15 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
goto fail;
}
+ /* Enable lazy_refcounts according to image and command line options */
+ ret = qcow2_update_options(bs, opts, flags, errp);
+ if (ret < 0) {
+ goto fail;
+ }
+
+ qemu_opts_del(opts);
+ opts = NULL;
+
s->cluster_cache = g_malloc(s->cluster_size);
/* one more sector for decompressed data alignment */
s->cluster_data = qemu_try_blockalign(bs->file, QCOW_MAX_CRYPT_CLUSTERS
@@ -1004,15 +1013,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
}
}
- /* Enable lazy_refcounts according to image and command line options */
- ret = qcow2_update_options(bs, opts, flags, errp);
- if (ret < 0) {
- goto fail;
- }
-
- qemu_opts_del(opts);
- opts = NULL;
-
#ifdef DEBUG_ALLOC
{
BdrvCheckResult result = {0};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v2 07/13] qcow2: Move rest of option handling to qcow2_update_options()
2015-09-04 17:18 [Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options Kevin Wolf
` (5 preceding siblings ...)
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 06/13] qcow2: Move qcow2_update_options() call up Kevin Wolf
@ 2015-09-04 17:18 ` Kevin Wolf
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 08/13] qcow2: Leave s unchanged on qcow2_update_options() failure Kevin Wolf
` (5 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Kevin Wolf @ 2015-09-04 17:18 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
With this commit, the handling of driver-specific options in
qcow2_open() is completely separated out into qcow2_update_options().
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 111 +++++++++++++++++++++++++++++-----------------------------
1 file changed, 56 insertions(+), 55 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 2903481..bad377d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -541,15 +541,65 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
}
}
-static int qcow2_update_options(BlockDriverState *bs, QemuOpts *opts,
+static int qcow2_update_options(BlockDriverState *bs, QDict *options,
int flags, Error **errp)
{
BDRVQcowState *s = bs->opaque;
+ QemuOpts *opts = NULL;
const char *opt_overlap_check, *opt_overlap_check_template;
int overlap_check_template = 0;
+ uint64_t l2_cache_size, refcount_cache_size;
int i;
+ Error *local_err = NULL;
int ret;
+ opts = qemu_opts_create(&qcow2_runtime_opts, NULL, 0, &error_abort);
+ qemu_opts_absorb_qdict(opts, options, &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* get L2 table/refcount block cache size from command line options */
+ read_cache_sizes(bs, opts, &l2_cache_size, &refcount_cache_size,
+ &local_err);
+ if (local_err) {
+ error_propagate(errp, local_err);
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ l2_cache_size /= s->cluster_size;
+ if (l2_cache_size < MIN_L2_CACHE_SIZE) {
+ l2_cache_size = MIN_L2_CACHE_SIZE;
+ }
+ if (l2_cache_size > INT_MAX) {
+ error_setg(errp, "L2 cache size too big");
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ refcount_cache_size /= s->cluster_size;
+ if (refcount_cache_size < MIN_REFCOUNT_CACHE_SIZE) {
+ refcount_cache_size = MIN_REFCOUNT_CACHE_SIZE;
+ }
+ if (refcount_cache_size > INT_MAX) {
+ error_setg(errp, "Refcount cache size too big");
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* alloc L2 table/refcount block cache */
+ s->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
+ s->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
+ if (s->l2_table_cache == NULL || s->refcount_block_cache == NULL) {
+ error_setg(errp, "Could not allocate metadata caches");
+ ret = -ENOMEM;
+ goto fail;
+ }
+
+ /* Enable lazy_refcounts according to image and command line options */
s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
(s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
@@ -612,6 +662,9 @@ static int qcow2_update_options(BlockDriverState *bs, QemuOpts *opts,
ret = 0;
fail:
+ qemu_opts_del(opts);
+ opts = NULL;
+
return ret;
}
@@ -622,11 +675,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
unsigned int len, i;
int ret = 0;
QCowHeader header;
- QemuOpts *opts = NULL;
Error *local_err = NULL;
uint64_t ext_end;
uint64_t l1_vm_state_index;
- uint64_t l2_cache_size, refcount_cache_size;
ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
if (ret < 0) {
@@ -874,61 +925,12 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
}
}
- /* get L2 table/refcount block cache size from command line options */
- opts = qemu_opts_create(&qcow2_runtime_opts, NULL, 0, &error_abort);
- qemu_opts_absorb_qdict(opts, options, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- ret = -EINVAL;
- goto fail;
- }
-
- read_cache_sizes(bs, opts, &l2_cache_size, &refcount_cache_size,
- &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
- ret = -EINVAL;
- goto fail;
- }
-
- l2_cache_size /= s->cluster_size;
- if (l2_cache_size < MIN_L2_CACHE_SIZE) {
- l2_cache_size = MIN_L2_CACHE_SIZE;
- }
- if (l2_cache_size > INT_MAX) {
- error_setg(errp, "L2 cache size too big");
- ret = -EINVAL;
- goto fail;
- }
-
- refcount_cache_size /= s->cluster_size;
- if (refcount_cache_size < MIN_REFCOUNT_CACHE_SIZE) {
- refcount_cache_size = MIN_REFCOUNT_CACHE_SIZE;
- }
- if (refcount_cache_size > INT_MAX) {
- error_setg(errp, "Refcount cache size too big");
- ret = -EINVAL;
- goto fail;
- }
-
- /* alloc L2 table/refcount block cache */
- s->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
- s->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
- if (s->l2_table_cache == NULL || s->refcount_block_cache == NULL) {
- error_setg(errp, "Could not allocate metadata caches");
- ret = -ENOMEM;
- goto fail;
- }
-
- /* Enable lazy_refcounts according to image and command line options */
- ret = qcow2_update_options(bs, opts, flags, errp);
+ /* Parse driver-specific options */
+ ret = qcow2_update_options(bs, options, flags, errp);
if (ret < 0) {
goto fail;
}
- qemu_opts_del(opts);
- opts = NULL;
-
s->cluster_cache = g_malloc(s->cluster_size);
/* one more sector for decompressed data alignment */
s->cluster_data = qemu_try_blockalign(bs->file, QCOW_MAX_CRYPT_CLUSTERS
@@ -1022,7 +1024,6 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
return ret;
fail:
- qemu_opts_del(opts);
g_free(s->unknown_header_fields);
cleanup_unknown_header_ext(bs);
qcow2_free_snapshots(bs);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v2 08/13] qcow2: Leave s unchanged on qcow2_update_options() failure
2015-09-04 17:18 [Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options Kevin Wolf
` (6 preceding siblings ...)
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 07/13] qcow2: Move rest of option handling to qcow2_update_options() Kevin Wolf
@ 2015-09-04 17:18 ` Kevin Wolf
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 09/13] qcow2: Fix memory leak in qcow2_update_options() error path Kevin Wolf
` (4 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Kevin Wolf @ 2015-09-04 17:18 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
On return, either all new options should be applied to BDRVQcowState (on
success), or all of the old settings should be preserved (on failure).
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 52 ++++++++++++++++++++++++++++++++--------------------
1 file changed, 32 insertions(+), 20 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index bad377d..9b67334 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -549,6 +549,9 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
const char *opt_overlap_check, *opt_overlap_check_template;
int overlap_check_template = 0;
uint64_t l2_cache_size, refcount_cache_size;
+ Qcow2Cache *l2_table_cache;
+ Qcow2Cache *refcount_block_cache;
+ bool use_lazy_refcounts;
int i;
Error *local_err = NULL;
int ret;
@@ -591,28 +594,25 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
}
/* alloc L2 table/refcount block cache */
- s->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
- s->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
- if (s->l2_table_cache == NULL || s->refcount_block_cache == NULL) {
+ l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
+ refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
+ if (l2_table_cache == NULL || refcount_block_cache == NULL) {
error_setg(errp, "Could not allocate metadata caches");
ret = -ENOMEM;
goto fail;
}
/* Enable lazy_refcounts according to image and command line options */
- s->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
+ use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
(s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
+ if (use_lazy_refcounts && s->qcow_version < 3) {
+ error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
+ "qemu 1.1 compatibility level");
+ ret = -EINVAL;
+ goto fail;
+ }
- s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
- s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
- s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
- qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
- flags & BDRV_O_UNMAP);
- s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
- qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
- s->discard_passthrough[QCOW2_DISCARD_OTHER] =
- qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
-
+ /* Overlap check options */
opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
opt_overlap_check_template = qemu_opt_get(opts, QCOW2_OPT_OVERLAP_TEMPLATE);
if (opt_overlap_check_template && opt_overlap_check &&
@@ -644,6 +644,10 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
goto fail;
}
+ /*
+ * Start updating fields in BDRVQcowState.
+ * After this point no failure is allowed any more.
+ */
s->overlap_check = 0;
for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
/* overlap-check defines a template bitmask, but every flag may be
@@ -653,12 +657,20 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
overlap_check_template & (1 << i)) << i;
}
- if (s->use_lazy_refcounts && s->qcow_version < 3) {
- error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
- "qemu 1.1 compatibility level");
- ret = -EINVAL;
- goto fail;
- }
+ s->l2_table_cache = l2_table_cache;
+ s->refcount_block_cache = refcount_block_cache;
+
+ s->use_lazy_refcounts = use_lazy_refcounts;
+
+ s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
+ s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
+ s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
+ qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
+ flags & BDRV_O_UNMAP);
+ s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
+ qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
+ s->discard_passthrough[QCOW2_DISCARD_OTHER] =
+ qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
ret = 0;
fail:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v2 09/13] qcow2: Fix memory leak in qcow2_update_options() error path
2015-09-04 17:18 [Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options Kevin Wolf
` (7 preceding siblings ...)
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 08/13] qcow2: Leave s unchanged on qcow2_update_options() failure Kevin Wolf
@ 2015-09-04 17:18 ` Kevin Wolf
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 10/13] qcow2: Make qcow2_update_options() suitable for transactions Kevin Wolf
` (3 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Kevin Wolf @ 2015-09-04 17:18 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 9b67334..24acefd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -549,8 +549,8 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
const char *opt_overlap_check, *opt_overlap_check_template;
int overlap_check_template = 0;
uint64_t l2_cache_size, refcount_cache_size;
- Qcow2Cache *l2_table_cache;
- Qcow2Cache *refcount_block_cache;
+ Qcow2Cache *l2_table_cache = NULL;
+ Qcow2Cache *refcount_block_cache = NULL;
bool use_lazy_refcounts;
int i;
Error *local_err = NULL;
@@ -674,6 +674,14 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
ret = 0;
fail:
+ if (ret < 0) {
+ if (l2_table_cache) {
+ qcow2_cache_destroy(bs, l2_table_cache);
+ }
+ if (refcount_block_cache) {
+ qcow2_cache_destroy(bs, refcount_block_cache);
+ }
+ }
qemu_opts_del(opts);
opts = NULL;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v2 10/13] qcow2: Make qcow2_update_options() suitable for transactions
2015-09-04 17:18 [Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options Kevin Wolf
` (8 preceding siblings ...)
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 09/13] qcow2: Fix memory leak in qcow2_update_options() error path Kevin Wolf
@ 2015-09-04 17:18 ` Kevin Wolf
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 11/13] qcow2: Support updating driver-specific options in reopen Kevin Wolf
` (2 subsequent siblings)
12 siblings, 0 replies; 22+ messages in thread
From: Kevin Wolf @ 2015-09-04 17:18 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Before we can allow updating options at runtime with bdrv_reopen(), we
need to split the function into prepare/commit/abort parts.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block/qcow2.c | 101 ++++++++++++++++++++++++++++++++++++++--------------------
1 file changed, 67 insertions(+), 34 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 24acefd..b498cf1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -541,17 +541,24 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
}
}
-static int qcow2_update_options(BlockDriverState *bs, QDict *options,
- int flags, Error **errp)
+typedef struct Qcow2ReopenState {
+ Qcow2Cache *l2_table_cache;
+ Qcow2Cache *refcount_block_cache;
+ bool use_lazy_refcounts;
+ int overlap_check;
+ bool discard_passthrough[QCOW2_DISCARD_MAX];
+} Qcow2ReopenState;
+
+static int qcow2_update_options_prepare(BlockDriverState *bs,
+ Qcow2ReopenState *r,
+ QDict *options, int flags,
+ Error **errp)
{
BDRVQcowState *s = bs->opaque;
QemuOpts *opts = NULL;
const char *opt_overlap_check, *opt_overlap_check_template;
int overlap_check_template = 0;
uint64_t l2_cache_size, refcount_cache_size;
- Qcow2Cache *l2_table_cache = NULL;
- Qcow2Cache *refcount_block_cache = NULL;
- bool use_lazy_refcounts;
int i;
Error *local_err = NULL;
int ret;
@@ -594,18 +601,18 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
}
/* alloc L2 table/refcount block cache */
- l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
- refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
- if (l2_table_cache == NULL || refcount_block_cache == NULL) {
+ r->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
+ r->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
+ if (r->l2_table_cache == NULL || r->refcount_block_cache == NULL) {
error_setg(errp, "Could not allocate metadata caches");
ret = -ENOMEM;
goto fail;
}
/* Enable lazy_refcounts according to image and command line options */
- use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
+ r->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
(s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
- if (use_lazy_refcounts && s->qcow_version < 3) {
+ if (r->use_lazy_refcounts && s->qcow_version < 3) {
error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
"qemu 1.1 compatibility level");
ret = -EINVAL;
@@ -644,46 +651,72 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
goto fail;
}
- /*
- * Start updating fields in BDRVQcowState.
- * After this point no failure is allowed any more.
- */
- s->overlap_check = 0;
+ r->overlap_check = 0;
for (i = 0; i < QCOW2_OL_MAX_BITNR; i++) {
/* overlap-check defines a template bitmask, but every flag may be
* overwritten through the associated boolean option */
- s->overlap_check |=
+ r->overlap_check |=
qemu_opt_get_bool(opts, overlap_bool_option_names[i],
overlap_check_template & (1 << i)) << i;
}
- s->l2_table_cache = l2_table_cache;
- s->refcount_block_cache = refcount_block_cache;
-
- s->use_lazy_refcounts = use_lazy_refcounts;
-
- s->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
- s->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
- s->discard_passthrough[QCOW2_DISCARD_REQUEST] =
+ r->discard_passthrough[QCOW2_DISCARD_NEVER] = false;
+ r->discard_passthrough[QCOW2_DISCARD_ALWAYS] = true;
+ r->discard_passthrough[QCOW2_DISCARD_REQUEST] =
qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_REQUEST,
flags & BDRV_O_UNMAP);
- s->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
+ r->discard_passthrough[QCOW2_DISCARD_SNAPSHOT] =
qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_SNAPSHOT, true);
- s->discard_passthrough[QCOW2_DISCARD_OTHER] =
+ r->discard_passthrough[QCOW2_DISCARD_OTHER] =
qemu_opt_get_bool(opts, QCOW2_OPT_DISCARD_OTHER, false);
ret = 0;
fail:
- if (ret < 0) {
- if (l2_table_cache) {
- qcow2_cache_destroy(bs, l2_table_cache);
- }
- if (refcount_block_cache) {
- qcow2_cache_destroy(bs, refcount_block_cache);
- }
- }
qemu_opts_del(opts);
opts = NULL;
+ return ret;
+}
+
+static void qcow2_update_options_commit(BlockDriverState *bs,
+ Qcow2ReopenState *r)
+{
+ BDRVQcowState *s = bs->opaque;
+ int i;
+
+ s->l2_table_cache = r->l2_table_cache;
+ s->refcount_block_cache = r->refcount_block_cache;
+
+ s->overlap_check = r->overlap_check;
+ s->use_lazy_refcounts = r->use_lazy_refcounts;
+
+ for (i = 0; i < QCOW2_DISCARD_MAX; i++) {
+ s->discard_passthrough[i] = r->discard_passthrough[i];
+ }
+}
+
+static void qcow2_update_options_abort(BlockDriverState *bs,
+ Qcow2ReopenState *r)
+{
+ if (r->l2_table_cache) {
+ qcow2_cache_destroy(bs, r->l2_table_cache);
+ }
+ if (r->refcount_block_cache) {
+ qcow2_cache_destroy(bs, r->refcount_block_cache);
+ }
+}
+
+static int qcow2_update_options(BlockDriverState *bs, QDict *options,
+ int flags, Error **errp)
+{
+ Qcow2ReopenState r = {};
+ int ret;
+
+ ret = qcow2_update_options_prepare(bs, &r, options, flags, errp);
+ if (ret >= 0) {
+ qcow2_update_options_commit(bs, &r);
+ } else {
+ qcow2_update_options_abort(bs, &r);
+ }
return ret;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v2 11/13] qcow2: Support updating driver-specific options in reopen
2015-09-04 17:18 [Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options Kevin Wolf
` (9 preceding siblings ...)
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 10/13] qcow2: Make qcow2_update_options() suitable for transactions Kevin Wolf
@ 2015-09-04 17:18 ` Kevin Wolf
2015-09-04 19:16 ` Max Reitz
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 12/13] qemu-iotests: Reopen qcow2 with lazy-refcounts change Kevin Wolf
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 13/13] qemu-iotests: More qcow2 reopen tests Kevin Wolf
12 siblings, 1 reply; 22+ messages in thread
From: Kevin Wolf @ 2015-09-04 17:18 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
For updating the cache sizes or disabling lazy refcounts there is a bit
more to do than just changing the variables, but otherwise we're all set
for changing options during bdrv_reopen().
Just implement the missing pieces and hook the functions up in
bdrv_reopen().
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 65 insertions(+), 6 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index b498cf1..8d52766 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -600,7 +600,24 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
goto fail;
}
- /* alloc L2 table/refcount block cache */
+ /* alloc new L2 table/refcount block cache, flush old one */
+ if (s->l2_table_cache) {
+ ret = qcow2_cache_flush(bs, s->l2_table_cache);
+ if (ret) {
+ error_setg_errno(errp, -ret, "Failed to flush the L2 table cache");
+ goto fail;
+ }
+ }
+
+ if (s->refcount_block_cache) {
+ ret = qcow2_cache_flush(bs, s->refcount_block_cache);
+ if (ret) {
+ error_setg_errno(errp, -ret,
+ "Failed to flush the refcount block cache");
+ goto fail;
+ }
+ }
+
r->l2_table_cache = qcow2_cache_create(bs, l2_cache_size);
r->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size);
if (r->l2_table_cache == NULL || r->refcount_block_cache == NULL) {
@@ -609,7 +626,7 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
goto fail;
}
- /* Enable lazy_refcounts according to image and command line options */
+ /* lazy-refcounts; flush if going from enabled to disabled */
r->use_lazy_refcounts = qemu_opt_get_bool(opts, QCOW2_OPT_LAZY_REFCOUNTS,
(s->compatible_features & QCOW2_COMPAT_LAZY_REFCOUNTS));
if (r->use_lazy_refcounts && s->qcow_version < 3) {
@@ -619,6 +636,14 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
goto fail;
}
+ if (s->use_lazy_refcounts && !r->use_lazy_refcounts) {
+ ret = qcow2_mark_clean(bs);
+ if (ret < 0) {
+ error_setg_errno(errp, -ret, "Failed to disable lazy refcounts");
+ goto fail;
+ }
+ }
+
/* Overlap check options */
opt_overlap_check = qemu_opt_get(opts, QCOW2_OPT_OVERLAP);
opt_overlap_check_template = qemu_opt_get(opts, QCOW2_OPT_OVERLAP_TEMPLATE);
@@ -683,6 +708,12 @@ static void qcow2_update_options_commit(BlockDriverState *bs,
BDRVQcowState *s = bs->opaque;
int i;
+ if (s->l2_table_cache) {
+ qcow2_cache_destroy(bs, s->l2_table_cache);
+ }
+ if (s->refcount_block_cache) {
+ qcow2_cache_destroy(bs, s->refcount_block_cache);
+ }
s->l2_table_cache = r->l2_table_cache;
s->refcount_block_cache = r->refcount_block_cache;
@@ -1137,26 +1168,52 @@ static int qcow2_set_key(BlockDriverState *bs, const char *key)
return 0;
}
-/* We have no actual commit/abort logic for qcow2, but we need to write out any
- * unwritten data if we reopen read-only. */
static int qcow2_reopen_prepare(BDRVReopenState *state,
BlockReopenQueue *queue, Error **errp)
{
+ Qcow2ReopenState *r;
int ret;
+ r = g_new0(Qcow2ReopenState, 1);
+ state->opaque = r;
+
+ ret = qcow2_update_options_prepare(state->bs, r, state->options,
+ state->flags, errp);
+ if (ret < 0) {
+ goto fail;
+ }
+
+ /* We need to write out any unwritten data if we reopen read-only. */
if ((state->flags & BDRV_O_RDWR) == 0) {
ret = bdrv_flush(state->bs);
if (ret < 0) {
- return ret;
+ goto fail;
}
ret = qcow2_mark_clean(state->bs);
if (ret < 0) {
- return ret;
+ goto fail;
}
}
return 0;
+
+fail:
+ qcow2_update_options_abort(state->bs, r);
+ g_free(r);
+ return ret;
+}
+
+static void qcow2_reopen_commit(BDRVReopenState *state)
+{
+ qcow2_update_options_commit(state->bs, state->opaque);
+ g_free(state->opaque);
+}
+
+static void qcow2_reopen_abort(BDRVReopenState *state)
+{
+ qcow2_update_options_abort(state->bs, state->opaque);
+ g_free(state->opaque);
}
static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
@@ -3013,6 +3070,8 @@ BlockDriver bdrv_qcow2 = {
.bdrv_open = qcow2_open,
.bdrv_close = qcow2_close,
.bdrv_reopen_prepare = qcow2_reopen_prepare,
+ .bdrv_reopen_commit = qcow2_reopen_commit,
+ .bdrv_reopen_abort = qcow2_reopen_abort,
.bdrv_create = qcow2_create,
.bdrv_has_zero_init = bdrv_has_zero_init_1,
.bdrv_co_get_block_status = qcow2_co_get_block_status,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v2 12/13] qemu-iotests: Reopen qcow2 with lazy-refcounts change
2015-09-04 17:18 [Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options Kevin Wolf
` (10 preceding siblings ...)
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 11/13] qcow2: Support updating driver-specific options in reopen Kevin Wolf
@ 2015-09-04 17:18 ` Kevin Wolf
2015-09-04 19:22 ` Max Reitz
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 13/13] qemu-iotests: More qcow2 reopen tests Kevin Wolf
12 siblings, 1 reply; 22+ messages in thread
From: Kevin Wolf @ 2015-09-04 17:18 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/qemu-iotests/039 | 27 +++++++++++++++++++++++++++
tests/qemu-iotests/039.out | 18 ++++++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index 617f397..9e9b379 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -147,6 +147,33 @@ $PYTHON qcow2.py "$TEST_IMG".base dump-header | grep incompatible_features
_check_test_img
TEST_IMG="$TEST_IMG".base _check_test_img
+echo
+echo "== Changing lazy_refcounts setting at runtime =="
+
+IMGOPTS="compat=1.1,lazy_refcounts=off"
+_make_test_img $size
+
+$QEMU_IO -c "reopen -o lazy-refcounts=on" \
+ -c "write -P 0x5a 0 512" \
+ -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
+ | _filter_qemu_io
+
+# The dirty bit must be set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+_check_test_img
+
+IMGOPTS="compat=1.1,lazy_refcounts=on"
+_make_test_img $size
+
+$QEMU_IO -c "reopen -o lazy-refcounts=off" \
+ -c "write -P 0x5a 0 512" \
+ -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
+ | _filter_qemu_io
+
+# The dirty bit must not be set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+_check_test_img
+
# success, all done
echo "*** done"
diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
index b055670..39859b2 100644
--- a/tests/qemu-iotests/039.out
+++ b/tests/qemu-iotests/039.out
@@ -74,4 +74,22 @@ incompatible_features 0x0
incompatible_features 0x0
No errors were found on the image.
No errors were found on the image.
+
+== Changing lazy_refcounts setting at runtime ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+./common.config: Killed ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
+incompatible_features 0x1
+ERROR cluster 5 refcount=0 reference=1
+ERROR OFLAG_COPIED data cluster: l2_entry=8000000000050000 refcount=0
+
+2 errors were found on the image.
+Data may be corrupted, or further writes to the image may corrupt it.
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+./common.config: Killed ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
+incompatible_features 0x0
+No errors were found on the image.
*** done
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v2 12/13] qemu-iotests: Reopen qcow2 with lazy-refcounts change
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 12/13] qemu-iotests: Reopen qcow2 with lazy-refcounts change Kevin Wolf
@ 2015-09-04 19:22 ` Max Reitz
0 siblings, 0 replies; 22+ messages in thread
From: Max Reitz @ 2015-09-04 19:22 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 3003 bytes --]
On 04.09.2015 19:18, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> tests/qemu-iotests/039 | 27 +++++++++++++++++++++++++++
> tests/qemu-iotests/039.out | 18 ++++++++++++++++++
> 2 files changed, 45 insertions(+)
>
> diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
> index 617f397..9e9b379 100755
> --- a/tests/qemu-iotests/039
> +++ b/tests/qemu-iotests/039
> @@ -147,6 +147,33 @@ $PYTHON qcow2.py "$TEST_IMG".base dump-header | grep incompatible_features
> _check_test_img
> TEST_IMG="$TEST_IMG".base _check_test_img
>
> +echo
> +echo "== Changing lazy_refcounts setting at runtime =="
> +
> +IMGOPTS="compat=1.1,lazy_refcounts=off"
> +_make_test_img $size
> +
> +$QEMU_IO -c "reopen -o lazy-refcounts=on" \
> + -c "write -P 0x5a 0 512" \
> + -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> + | _filter_qemu_io
> +
> +# The dirty bit must be set
> +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
> +_check_test_img
> +
> +IMGOPTS="compat=1.1,lazy_refcounts=on"
> +_make_test_img $size
> +
> +$QEMU_IO -c "reopen -o lazy-refcounts=off" \
> + -c "write -P 0x5a 0 512" \
Maybe it would be nicer to test this the other way around, write first
and then disable lazy refcounts in order to test whether the refcounts
are actually flushed during the reopen.
Either way:
Reviewed-by: Max Reitz <mreitz@redhat.com>
> + -c "sigraise $(kill -l KILL)" "$TEST_IMG" 2>&1 \
> + | _filter_qemu_io
> +
> +# The dirty bit must not be set
> +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
> +_check_test_img
> +
>
> # success, all done
> echo "*** done"
> diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
> index b055670..39859b2 100644
> --- a/tests/qemu-iotests/039.out
> +++ b/tests/qemu-iotests/039.out
> @@ -74,4 +74,22 @@ incompatible_features 0x0
> incompatible_features 0x0
> No errors were found on the image.
> No errors were found on the image.
> +
> +== Changing lazy_refcounts setting at runtime ==
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> +wrote 512/512 bytes at offset 0
> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +./common.config: Killed ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
> +incompatible_features 0x1
> +ERROR cluster 5 refcount=0 reference=1
> +ERROR OFLAG_COPIED data cluster: l2_entry=8000000000050000 refcount=0
> +
> +2 errors were found on the image.
> +Data may be corrupted, or further writes to the image may corrupt it.
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
> +wrote 512/512 bytes at offset 0
> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +./common.config: Killed ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
> +incompatible_features 0x0
> +No errors were found on the image.
> *** done
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v2 13/13] qemu-iotests: More qcow2 reopen tests
2015-09-04 17:18 [Qemu-devel] [PATCH v2 00/13] qcow2: reopen: Change driver-specific runtime options Kevin Wolf
` (11 preceding siblings ...)
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 12/13] qemu-iotests: Reopen qcow2 with lazy-refcounts change Kevin Wolf
@ 2015-09-04 17:18 ` Kevin Wolf
2015-09-04 19:52 ` Max Reitz
12 siblings, 1 reply; 22+ messages in thread
From: Kevin Wolf @ 2015-09-04 17:18 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel, mreitz
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
tests/qemu-iotests/137 | 143 +++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/137.out | 43 ++++++++++++++
tests/qemu-iotests/group | 1 +
3 files changed, 187 insertions(+)
create mode 100755 tests/qemu-iotests/137
create mode 100644 tests/qemu-iotests/137.out
diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
new file mode 100755
index 0000000..ad52f71
--- /dev/null
+++ b/tests/qemu-iotests/137
@@ -0,0 +1,143 @@
+#!/bin/bash
+#
+# Test qcow2 reopen
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program. If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+here="$PWD"
+tmp=/tmp/$$
+status=1 # failure is the default!
+
+_cleanup()
+{
+ _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2
+_supported_proto generic
+_supported_os Linux
+
+
+_make_test_img 64M
+
+echo === Try setting valid values for all options ===
+echo
+
+# Try all options and then check that all of the basic I/O operations still
+# work on this image.
+$QEMU_IO \
+ -c "reopen -o lazy-refcounts=on,pass-discard-request=on" \
+ -c "reopen -o lazy-refcounts=off,pass-discard-request=off" \
+ -c "reopen -o pass-discard-snapshot=on,pass-discard-other=on" \
+ -c "reopen -o pass-discard-snapshot=off,pass-discard-other=off" \
+ -c "reopen -o overlap-check=all" \
+ -c "reopen -o overlap-check=none" \
+ -c "reopen -o overlap-check=cached" \
+ -c "reopen -o overlap-check=constant" \
+ -c "reopen -o overlap-check.template=all" \
+ -c "reopen -o overlap-check.template=none" \
+ -c "reopen -o overlap-check.template=cached" \
+ -c "reopen -o overlap-check.template=constant" \
+ -c "reopen -o overlap-check.main-header=on" \
+ -c "reopen -o overlap-check.main-header=off" \
+ -c "reopen -o overlap-check.active-l1=on" \
+ -c "reopen -o overlap-check.active-l1=off" \
+ -c "reopen -o overlap-check.active-l2=on" \
+ -c "reopen -o overlap-check.active-l2=off" \
+ -c "reopen -o overlap-check.refcount-table=on" \
+ -c "reopen -o overlap-check.refcount-table=off" \
+ -c "reopen -o overlap-check.refcount-block=on" \
+ -c "reopen -o overlap-check.refcount-block=off" \
+ -c "reopen -o overlap-check.snapshot-table=on" \
+ -c "reopen -o overlap-check.snapshot-table=off" \
+ -c "reopen -o overlap-check.inactive-l1=on" \
+ -c "reopen -o overlap-check.inactive-l1=off" \
+ -c "reopen -o overlap-check.inactive-l2=on" \
+ -c "reopen -o overlap-check.inactive-l2=off" \
+ -c "reopen -o cache-size=1M" \
+ -c "reopen -o l2-cache-size=512k" \
+ -c "reopen -o refcount-cache-size=128k" \
+ \
+ -c "write -P 55 0 32M" \
+ -c "read -P 55 0 32M" \
+ -c "discard 0 32M" \
+ -c "write -z 0 32M" \
+ -c "read -P 0 0 32M" \
+ \
+ "$TEST_IMG" | _filter_qemu_io
+
+
+echo
+echo === Try setting some invalid values ===
+echo
+
+$QEMU_IO \
+ -c "reopen -o lazy-refcounts=42" \
+ -c "reopen -o cache-size=1M,l2-cache-size=64k,refcount-cache-size=64k" \
+ -c "reopen -o cache-size=1M,l2-cache-size=2M" \
+ -c "reopen -o cache-size=1M,refcount-cache-size=2M" \
+ -c "reopen -o l2-cache-size=256T" \
+ -c "reopen -o l2-cache-size=64G" \
+ -c "reopen -o refcount-cache-size=256T" \
+ -c "reopen -o refcount-cache-size=64G" \
+ -c "reopen -o overlap-check=constant,overlap-check.template=all" \
+ -c "reopen -o overlap-check=blubb" \
+ -c "reopen -o overlap-check.template=blubb" \
+ "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo === Test transaction semantics ===
+echo
+
+# Whether lazy-refcounts was actually enabled can easily be tested: Check if
+# the dirty bit is set after a crash
+$QEMU_IO \
+ -c "reopen -o lazy-refcounts=on,overlap-check=blubb" \
+ -c "write -P 0x5a 0 512" \
+ -c "sigraise $(kill -l KILL)" \
+ "$TEST_IMG" 2>&1 | _filter_qemu_io
+
+# The dirty bit must not be set
+$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+
+# Similarly we can test whether corruption detection has been enabled:
+# Create L1/L2, overwrite first entry in refcount block, allocate something.
+# Disabling the checks should fail, so the corruption must be detected.
+_make_test_img 64M
+$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io
+poke_file "$TEST_IMG" "$((0x20000))" "\x00\x00"
+$QEMU_IO \
+ -c "reopen -o overlap-check=none,lazy-refcounts=42" \
+ -c "write 64k 64k" \
+ "$TEST_IMG" 2>&1 | _filter_qemu_io
+
+# success, all done
+echo '*** done'
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out
new file mode 100644
index 0000000..b1a1565
--- /dev/null
+++ b/tests/qemu-iotests/137.out
@@ -0,0 +1,43 @@
+QA output created by 137
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+=== Try setting valid values for all options ===
+
+wrote 33554432/33554432 bytes at offset 0
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 33554432/33554432 bytes at offset 0
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 33554432/33554432 bytes at offset 0
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 33554432/33554432 bytes at offset 0
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 33554432/33554432 bytes at offset 0
+32 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Try setting some invalid values ===
+
+Parameter 'lazy-refcounts' expects 'on' or 'off'
+cache-size, l2-cache-size and refcount-cache-size may not be set the same time
+l2-cache-size may not exceed cache-size
+refcount-cache-size may not exceed cache-size
+L2 cache size too big
+Could not allocate metadata caches
+L2 cache size too big
+Could not allocate metadata caches
+Conflicting values for qcow2 options 'overlap-check' ('constant') and 'overlap-check.template' ('all')
+Unsupported value 'blubb' for qcow2 option 'overlap-check'. Allowed are any of the following: none, constant, cached, all
+Unsupported value 'blubb' for qcow2 option 'overlap-check'. Allowed are any of the following: none, constant, cached, all
+
+=== Test transaction semantics ===
+
+Unsupported value 'blubb' for qcow2 option 'overlap-check'. Allowed are any of the following: none, constant, cached, all
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+./common.config: Killed ( exec "$QEMU_IO_PROG" $QEMU_IO_OPTIONS "$@" )
+incompatible_features 0x0
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Parameter 'lazy-refcounts' expects 'on' or 'off'
+qcow2: Marking image as corrupt: Preventing invalid write on metadata (overlaps with qcow2_header); further corruption events will be suppressed
+write failed: Input/output error
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index c430b6c..3a6a8f0 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -134,3 +134,4 @@
132 rw auto quick
134 rw auto quick
135 rw auto
+137 rw auto
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v2 13/13] qemu-iotests: More qcow2 reopen tests
2015-09-04 17:18 ` [Qemu-devel] [PATCH v2 13/13] qemu-iotests: More qcow2 reopen tests Kevin Wolf
@ 2015-09-04 19:52 ` Max Reitz
2015-09-07 12:08 ` Kevin Wolf
0 siblings, 1 reply; 22+ messages in thread
From: Max Reitz @ 2015-09-04 19:52 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1950 bytes --]
On 04.09.2015 19:18, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> tests/qemu-iotests/137 | 143 +++++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/137.out | 43 ++++++++++++++
> tests/qemu-iotests/group | 1 +
> 3 files changed, 187 insertions(+)
> create mode 100755 tests/qemu-iotests/137
> create mode 100644 tests/qemu-iotests/137.out
>
> diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
> new file mode 100755
> index 0000000..ad52f71
> --- /dev/null
> +++ b/tests/qemu-iotests/137
[snip]
> +echo
> +echo === Try setting some invalid values ===
> +echo
> +
> +$QEMU_IO \
> + -c "reopen -o lazy-refcounts=42" \
> + -c "reopen -o cache-size=1M,l2-cache-size=64k,refcount-cache-size=64k" \
> + -c "reopen -o cache-size=1M,l2-cache-size=2M" \
> + -c "reopen -o cache-size=1M,refcount-cache-size=2M" \
> + -c "reopen -o l2-cache-size=256T" \
> + -c "reopen -o l2-cache-size=64G" \
Testing this seems risky. The rest looks fine, but I'm not sure whether
this is really needed so much that we should risk the test failing on
some machines, especially in the future.
Or actually even on my machine:
$ touch ~/tmp/swap
$ truncate ~/tmp/swap --size 128G
$ fallocate -l 128G ~/tmp/swap
# mkswap ~maxx/tmp/swap
# swapon ~max/tmp/swap
$ cd /to/qemu/build
$ ./qemu-io -c 'reopen -o l2-cache-size=64G' test.qcow2
$
(Running the test itself is apparently a bad idea, because then for some
reason Linux thinks I actually want to use the swap and everything slows
down to a crawl)
Max
> + -c "reopen -o refcount-cache-size=256T" \
> + -c "reopen -o refcount-cache-size=64G" \
> + -c "reopen -o overlap-check=constant,overlap-check.template=all" \
> + -c "reopen -o overlap-check=blubb" \
> + -c "reopen -o overlap-check.template=blubb" \
> + "$TEST_IMG" | _filter_qemu_io
> +
[snip]
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v2 13/13] qemu-iotests: More qcow2 reopen tests
2015-09-04 19:52 ` Max Reitz
@ 2015-09-07 12:08 ` Kevin Wolf
2015-09-07 12:23 ` [Qemu-devel] [Qemu-block] " Kevin Wolf
0 siblings, 1 reply; 22+ messages in thread
From: Kevin Wolf @ 2015-09-07 12:08 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 2180 bytes --]
Am 04.09.2015 um 21:52 hat Max Reitz geschrieben:
> On 04.09.2015 19:18, Kevin Wolf wrote:
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > tests/qemu-iotests/137 | 143 +++++++++++++++++++++++++++++++++++++++++++++
> > tests/qemu-iotests/137.out | 43 ++++++++++++++
> > tests/qemu-iotests/group | 1 +
> > 3 files changed, 187 insertions(+)
> > create mode 100755 tests/qemu-iotests/137
> > create mode 100644 tests/qemu-iotests/137.out
> >
> > diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
> > new file mode 100755
> > index 0000000..ad52f71
> > --- /dev/null
> > +++ b/tests/qemu-iotests/137
>
>
> [snip]
>
> > +echo
> > +echo === Try setting some invalid values ===
> > +echo
> > +
> > +$QEMU_IO \
> > + -c "reopen -o lazy-refcounts=42" \
> > + -c "reopen -o cache-size=1M,l2-cache-size=64k,refcount-cache-size=64k" \
> > + -c "reopen -o cache-size=1M,l2-cache-size=2M" \
> > + -c "reopen -o cache-size=1M,refcount-cache-size=2M" \
> > + -c "reopen -o l2-cache-size=256T" \
> > + -c "reopen -o l2-cache-size=64G" \
>
> Testing this seems risky. The rest looks fine, but I'm not sure whether
> this is really needed so much that we should risk the test failing on
> some machines, especially in the future.
I think we should be testing the "too large, but not much too large"
case. A risk of false positives is better than a risk to break things
without noticing. I could make it something like 64T, though, that
should last a bit longer. The "much too large" case starts at 128T
(INT_MAX * s->cluster_size == 2G * 64k).
> Or actually even on my machine:
>
> $ touch ~/tmp/swap
> $ truncate ~/tmp/swap --size 128G
> $ fallocate -l 128G ~/tmp/swap
> # mkswap ~maxx/tmp/swap
> # swapon ~max/tmp/swap
This looks faked. :-P
> $ cd /to/qemu/build
> $ ./qemu-io -c 'reopen -o l2-cache-size=64G' test.qcow2
> $
>
> (Running the test itself is apparently a bad idea, because then for some
> reason Linux thinks I actually want to use the swap and everything slows
> down to a crawl)
Well, if you do that intentionally, I guess you deserve it...
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 13/13] qemu-iotests: More qcow2 reopen tests
2015-09-07 12:08 ` Kevin Wolf
@ 2015-09-07 12:23 ` Kevin Wolf
0 siblings, 0 replies; 22+ messages in thread
From: Kevin Wolf @ 2015-09-07 12:23 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 2116 bytes --]
Am 07.09.2015 um 14:08 hat Kevin Wolf geschrieben:
> Am 04.09.2015 um 21:52 hat Max Reitz geschrieben:
> > On 04.09.2015 19:18, Kevin Wolf wrote:
> > > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > > ---
> > > tests/qemu-iotests/137 | 143 +++++++++++++++++++++++++++++++++++++++++++++
> > > tests/qemu-iotests/137.out | 43 ++++++++++++++
> > > tests/qemu-iotests/group | 1 +
> > > 3 files changed, 187 insertions(+)
> > > create mode 100755 tests/qemu-iotests/137
> > > create mode 100644 tests/qemu-iotests/137.out
> > >
> > > diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137
> > > new file mode 100755
> > > index 0000000..ad52f71
> > > --- /dev/null
> > > +++ b/tests/qemu-iotests/137
> >
> >
> > [snip]
> >
> > > +echo
> > > +echo === Try setting some invalid values ===
> > > +echo
> > > +
> > > +$QEMU_IO \
> > > + -c "reopen -o lazy-refcounts=42" \
> > > + -c "reopen -o cache-size=1M,l2-cache-size=64k,refcount-cache-size=64k" \
> > > + -c "reopen -o cache-size=1M,l2-cache-size=2M" \
> > > + -c "reopen -o cache-size=1M,refcount-cache-size=2M" \
> > > + -c "reopen -o l2-cache-size=256T" \
> > > + -c "reopen -o l2-cache-size=64G" \
> >
> > Testing this seems risky. The rest looks fine, but I'm not sure whether
> > this is really needed so much that we should risk the test failing on
> > some machines, especially in the future.
>
> I think we should be testing the "too large, but not much too large"
> case. A risk of false positives is better than a risk to break things
> without noticing. I could make it something like 64T, though, that
> should last a bit longer. The "much too large" case starts at 128T
> (INT_MAX * s->cluster_size == 2G * 64k).
...except that qcow2_cache_create() also allocates a Qcow2CachedTable
array and that one is small enough in comparison that we'd end up again
in the same range as before, where allocations might not fail right
away, but fill up the swap.
Fair enough, I'll remove the lines. Do you want a v3 on the list or can
I fix it while merging?
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 22+ messages in thread