[Qemu-block] [PATCH v3 01/21] qcow2: Add .bdrv_join_options callback

2015-12-04 Thread Kevin Wolf
qcow2 accepts a few driver-specific options that overlap semantically
(e.g. "overlap-check" is an alias of "overlap-check.template", and any
missing cache size option is derived from the given ones).

When bdrv_reopen() merges the set of updated options with left out
options that should be kept at their old value, we need to consider this
and filter out any duplicates (which would generally cause errors
because new and old value would contradict each other).

This patch adds a .bdrv_join_options callback to BlockDriver and
implements it for qcow2.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block/qcow2.c | 47 +++
 include/block/block_int.h |  1 +
 2 files changed, 48 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 88f56c8..9baaf4d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1282,6 +1282,52 @@ static void qcow2_reopen_abort(BDRVReopenState *state)
 g_free(state->opaque);
 }
 
+static void qcow2_join_options(QDict *options, QDict *old_options)
+{
+bool has_new_overlap_template =
+qdict_haskey(options, QCOW2_OPT_OVERLAP) ||
+qdict_haskey(options, QCOW2_OPT_OVERLAP_TEMPLATE);
+bool has_new_total_cache_size =
+qdict_haskey(options, QCOW2_OPT_CACHE_SIZE);
+bool has_all_cache_options;
+
+/* New overlap template overrides all old overlap options */
+if (has_new_overlap_template) {
+qdict_del(old_options, QCOW2_OPT_OVERLAP);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_TEMPLATE);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_MAIN_HEADER);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_ACTIVE_L1);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_ACTIVE_L2);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_REFCOUNT_TABLE);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_REFCOUNT_BLOCK);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_SNAPSHOT_TABLE);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_INACTIVE_L1);
+qdict_del(old_options, QCOW2_OPT_OVERLAP_INACTIVE_L2);
+}
+
+/* New total cache size overrides all old options */
+if (qdict_haskey(options, QCOW2_OPT_CACHE_SIZE)) {
+qdict_del(old_options, QCOW2_OPT_L2_CACHE_SIZE);
+qdict_del(old_options, QCOW2_OPT_REFCOUNT_CACHE_SIZE);
+}
+
+qdict_join(options, old_options, false);
+
+/*
+ * If after merging all cache size options are set, an old total size is
+ * overwritten. Do keep all options, however, if all three are new. The
+ * resulting error message is what we want to happen.
+ */
+has_all_cache_options =
+qdict_haskey(options, QCOW2_OPT_CACHE_SIZE) ||
+qdict_haskey(options, QCOW2_OPT_L2_CACHE_SIZE) ||
+qdict_haskey(options, QCOW2_OPT_REFCOUNT_CACHE_SIZE);
+
+if (has_all_cache_options && !has_new_total_cache_size) {
+qdict_del(options, QCOW2_OPT_CACHE_SIZE);
+}
+}
+
 static int64_t coroutine_fn qcow2_co_get_block_status(BlockDriverState *bs,
 int64_t sector_num, int nb_sectors, int *pnum)
 {
@@ -3145,6 +3191,7 @@ BlockDriver bdrv_qcow2 = {
 .bdrv_reopen_prepare  = qcow2_reopen_prepare,
 .bdrv_reopen_commit   = qcow2_reopen_commit,
 .bdrv_reopen_abort= qcow2_reopen_abort,
+.bdrv_join_options= qcow2_join_options,
 .bdrv_create= qcow2_create,
 .bdrv_has_zero_init = bdrv_has_zero_init_1,
 .bdrv_co_get_block_status = qcow2_co_get_block_status,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 4012e36..77dc165 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -121,6 +121,7 @@ struct BlockDriver {
BlockReopenQueue *queue, Error **errp);
 void (*bdrv_reopen_commit)(BDRVReopenState *reopen_state);
 void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
+void (*bdrv_join_options)(QDict *options, QDict *old_options);
 
 int (*bdrv_open)(BlockDriverState *bs, QDict *options, int flags,
  Error **errp);
-- 
1.8.3.1




[Qemu-block] [PATCH v3 08/21] block: Keep "driver" in bs->options

2015-12-04 Thread Kevin Wolf
Instead of passing a separate drv argument to bdrv_open_common(), just
make sure that a "driver" option is set in the QDict. This also means
that a "driver" entry is consistently present in bs->options now.

This is another step towards keeping all options in the QDict (which is
the represenation of the blockdev-add QMP command).

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block.c | 57 -
 1 file changed, 40 insertions(+), 17 deletions(-)

diff --git a/block.c b/block.c
index 5ca7c71..2728bc5 100644
--- a/block.c
+++ b/block.c
@@ -817,6 +817,11 @@ static QemuOptsList bdrv_runtime_opts = {
 .type = QEMU_OPT_STRING,
 .help = "Node name of the block device node",
 },
+{
+.name = "driver",
+.type = QEMU_OPT_STRING,
+.help = "Block driver to use for the node",
+},
 { /* end of list */ }
 },
 };
@@ -827,18 +832,31 @@ static QemuOptsList bdrv_runtime_opts = {
  * Removes all processed options from *options.
  */
 static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file,
-QDict *options, int flags, BlockDriver *drv, Error **errp)
+QDict *options, int flags, Error **errp)
 {
 int ret, open_flags;
 const char *filename;
+const char *driver_name = NULL;
 const char *node_name = NULL;
 QemuOpts *opts;
+BlockDriver *drv;
 Error *local_err = NULL;
 
-assert(drv != NULL);
 assert(bs->file == NULL);
 assert(options != NULL && bs->options != options);
 
+opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
+qemu_opts_absorb_qdict(opts, options, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto fail_opts;
+}
+
+driver_name = qemu_opt_get(opts, "driver");
+drv = bdrv_find_format(driver_name);
+assert(drv != NULL);
+
 if (file != NULL) {
 filename = file->bs->filename;
 } else {
@@ -848,19 +866,12 @@ static int bdrv_open_common(BlockDriverState *bs, 
BdrvChild *file,
 if (drv->bdrv_needs_filename && !filename) {
 error_setg(errp, "The '%s' block driver requires a file name",
drv->format_name);
-return -EINVAL;
-}
-
-trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
-
-opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
-qemu_opts_absorb_qdict(opts, options, _err);
-if (local_err) {
-error_propagate(errp, local_err);
 ret = -EINVAL;
 goto fail_opts;
 }
 
+trace_bdrv_open_common(bs, filename ?: "", flags, drv->format_name);
+
 node_name = qemu_opt_get(opts, "node-name");
 bdrv_assign_node_name(bs, node_name, _err);
 if (local_err) {
@@ -1477,11 +1488,14 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 goto fail;
 }
 
+bs->open_flags = flags;
+bs->options = options;
+options = qdict_clone_shallow(options);
+
 /* Find the right image format driver */
 drvname = qdict_get_try_str(options, "driver");
 if (drvname) {
 drv = bdrv_find_format(drvname);
-qdict_del(options, "driver");
 if (!drv) {
 error_setg(errp, "Unknown driver: '%s'", drvname);
 ret = -EINVAL;
@@ -1497,10 +1511,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 qdict_del(options, "backing");
 }
 
-bs->open_flags = flags;
-bs->options = options;
-options = qdict_clone_shallow(options);
-
 /* Open image file without format layer */
 if ((flags & BDRV_O_PROTOCOL) == 0) {
 if (flags & BDRV_O_RDWR) {
@@ -1528,6 +1538,19 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 if (ret < 0) {
 goto fail;
 }
+/*
+ * This option update would logically belong in bdrv_fill_options(),
+ * but we first need to open bs->file for the probing to work, while
+ * opening bs->file already requires the (mostly) final set of options
+ * so that cache mode etc. can be inherited.
+ *
+ * Adding the driver later is somewhat ugly, but it's not an option
+ * that would ever be inherited, so it's correct. We just need to make
+ * sure to update both bs->options (which has the full effective
+ * options for bs) and options (which has file.* already removed).
+ */
+qdict_put(bs->options, "driver", qstring_from_str(drv->format_name));
+qdict_put(options, "driver", qstring_from_str(drv->format_name));
 } else if (!drv) {
 error_setg(errp, "Must specify either driver or file");
 ret = -EINVAL;
@@ -1541,7 +1564,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 assert(!(flags & BDRV_O_PROTOCOL) || !file);

[Qemu-block] [PATCH v3 02/21] block: Fix reopen with semantically overlapping options

2015-12-04 Thread Kevin Wolf
This fixes bdrv_reopen() calls like the following one:

qemu-io -c 'open -o overlap-check.template=all /tmp/test.qcow2' \
-c 'reopen -o overlap-check=none'

The approach taken so far would result in an options QDict that has both
"overlap-check.template=all" and "overlap-check=none", which obviously
conflicts. In this case, the old option should be overridden by the
newly specified option.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 3a7324b..675e5a8 100644
--- a/block.c
+++ b/block.c
@@ -624,6 +624,20 @@ static int refresh_total_sectors(BlockDriverState *bs, 
int64_t hint)
 }
 
 /**
+ * Combines a QDict of new block driver @options with any missing options taken
+ * from @old_options, so that leaving out an option defaults to its old value.
+ */
+static void bdrv_join_options(BlockDriverState *bs, QDict *options,
+  QDict *old_options)
+{
+if (bs->drv && bs->drv->bdrv_join_options) {
+bs->drv->bdrv_join_options(options, old_options);
+} else {
+qdict_join(options, old_options, false);
+}
+}
+
+/**
  * Set open flags for a given discard mode
  *
  * Return 0 on success, -1 if the discard mode was invalid.
@@ -1663,7 +1677,7 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue 
*bs_queue,
 }
 
 old_options = qdict_clone_shallow(bs->options);
-qdict_join(options, old_options, false);
+bdrv_join_options(bs, options, old_options);
 QDECREF(old_options);
 
 /* bdrv_open() masks this flag out */
-- 
1.8.3.1




[Qemu-block] [PATCH v3 00/21] block: Cache mode for children etc.

2015-12-04 Thread Kevin Wolf
This is part three (or four, depending on whether you count the bdrv_swap
removal) of what I had sent earlier as "[PATCH 00/34] block: Cache mode for
children, reopen overhaul and more". Most of the patches were actually already
reviewed in v1.

This part contains the remaining functional changes that the cover letter for
v1 advertised, and a bit more:

- You can now use node name references for backing files
- bdrv_reopen() works now properly for inherited options (don't exist before
  this series; after the series the cache options)
- bdrv_reopen() works now properly with semantically overlapping options
- bdrv_reopen() can change child node options
- And finally you can set cache mode options for backing files and other
  children now (and the reopen behaviour even makes sense

v3:
- Patch 3 ('mirror: Error out when a BDS would get two BBs')
  Fixed typo in a comment [Max]

- Patch 4 ('block: Allow references for backing files')
  Removed meanwhile redundant assertion [Max]
  s/backing_hd/backing/ in the commit message [Max]

- Patch 6 ('block: Exclude nested options only for children in 
append_open_options()')
  g_strdup() the child name to avoid use after free [Wen Congyang]

- Patch 14 ('blockdev: Set 'format' indicates non-empty drive')
  Fix qemu commandline in a qtest case [Wen Congyang]

- Patch 20 ('qemu-iotests: Test cache mode option inheritance')
  s/backing_hd/backing/ in some comments [Max]
  Enabled commented out 'grep Cache' filter for saner output [Max]


Kevin Wolf (21):
  qcow2: Add .bdrv_join_options callback
  block: Fix reopen with semantically overlapping options
  mirror: Error out when a BDS would get two BBs
  block: Allow references for backing files
  block: Consider all block layer options in append_open_options
  block: Exclude nested options only for children in
append_open_options()
  block: Pass driver-specific options to .bdrv_refresh_filename()
  block: Keep "driver" in bs->options
  block: Allow specifying child options in reopen
  block: reopen: Document option precedence and refactor accordingly
  block: Add infrastructure for option inheritance
  block: Split out parse_json_protocol()
  block: Introduce bs->explicit_options
  blockdev: Set 'format' indicates non-empty drive
  qemu-iotests: Remove cache mode test without medium
  block: reopen: Extract QemuOpts for generic block layer options
  block: Move cache options into options QDict
  blkdebug: Enable reopen
  qemu-iotests: Try setting cache mode for children
  qemu-iotests: Test cache mode option inheritance
  qemu-iotests: Test reopen with node-name/driver options

 block.c   | 459 +++--
 block/blkdebug.c  |  24 +-
 block/blkverify.c |   2 +-
 block/mirror.c|  30 +-
 block/nbd.c   |  10 +-
 block/qcow2.c |  47 +++
 block/quorum.c|   2 +-
 blockdev.c|  57 +---
 include/block/block.h |   4 +-
 include/block/block_int.h |   8 +-
 tests/hd-geo-test.c   |   4 +-
 tests/qemu-iotests/051|  22 +-
 tests/qemu-iotests/051.out|  74 +++-
 tests/qemu-iotests/133|  90 +
 tests/qemu-iotests/133.out|  22 ++
 tests/qemu-iotests/142| 354 +++
 tests/qemu-iotests/142.out| 773 ++
 tests/qemu-iotests/group  |   2 +
 tests/qemu-iotests/iotests.py |   4 +-
 19 files changed, 1811 insertions(+), 177 deletions(-)
 create mode 100755 tests/qemu-iotests/133
 create mode 100644 tests/qemu-iotests/133.out
 create mode 100755 tests/qemu-iotests/142
 create mode 100644 tests/qemu-iotests/142.out

-- 
1.8.3.1




[Qemu-block] [PATCH v3 03/21] mirror: Error out when a BDS would get two BBs

2015-12-04 Thread Kevin Wolf
bdrv_replace_in_backing_chain() asserts that not both old and new
BlockDdriverState have a BlockBackend attached to them because both
would have to end up pointing to the new BDS and we don't support more
than one BB per BDS yet.

Before we can safely allow references to existing nodes as backing
files, we need to make sure that even if a backing file has a BB on it,
this doesn't crash qemu.

There are probably also some cases with the 'replaces' option set where
drive-mirror could fail this assertion today. They are fixed with this
error check as well.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block/mirror.c | 28 
 1 file changed, 28 insertions(+)

diff --git a/block/mirror.c b/block/mirror.c
index 0e8f556..8e3f524 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -18,6 +18,7 @@
 #include "qapi/qmp/qerror.h"
 #include "qemu/ratelimit.h"
 #include "qemu/bitmap.h"
+#include "qemu/error-report.h"
 
 #define SLICE_TIME1ULL /* ns */
 #define MAX_IN_FLIGHT 16
@@ -370,11 +371,22 @@ static void mirror_exit(BlockJob *job, void *opaque)
 if (s->to_replace) {
 to_replace = s->to_replace;
 }
+
+/* This was checked in mirror_start_job(), but meanwhile one of the
+ * nodes could have been newly attached to a BlockBackend. */
+if (to_replace->blk && s->target->blk) {
+error_report("block job: Can't create node with two 
BlockBackends");
+data->ret = -EINVAL;
+goto out;
+}
+
 if (bdrv_get_flags(s->target) != bdrv_get_flags(to_replace)) {
 bdrv_reopen(s->target, bdrv_get_flags(to_replace), NULL);
 }
 bdrv_replace_in_backing_chain(to_replace, s->target);
 }
+
+out:
 if (s->to_replace) {
 bdrv_op_unblock_all(s->to_replace, s->replace_blocker);
 error_free(s->replace_blocker);
@@ -705,6 +717,7 @@ static void mirror_start_job(BlockDriverState *bs, 
BlockDriverState *target,
  bool is_none_mode, BlockDriverState *base)
 {
 MirrorBlockJob *s;
+BlockDriverState *replaced_bs;
 
 if (granularity == 0) {
 granularity = bdrv_get_default_bitmap_granularity(target);
@@ -728,6 +741,21 @@ static void mirror_start_job(BlockDriverState *bs, 
BlockDriverState *target,
 buf_size = DEFAULT_MIRROR_BUF_SIZE;
 }
 
+/* We can't support this case as long as the block layer can't handle
+ * multiple BlockBackends per BlockDriverState. */
+if (replaces) {
+replaced_bs = bdrv_lookup_bs(replaces, replaces, errp);
+if (replaced_bs == NULL) {
+return;
+}
+} else {
+replaced_bs = bs;
+}
+if (replaced_bs->blk && target->blk) {
+error_setg(errp, "Can't create node with two BlockBackends");
+return;
+}
+
 s = block_job_create(driver, bs, speed, cb, opaque, errp);
 if (!s) {
 return;
-- 
1.8.3.1




[Qemu-block] [PATCH v3 04/21] block: Allow references for backing files

2015-12-04 Thread Kevin Wolf
For bs->file, using references to existing BDSes has been possible for a
while already. This patch enables the same for bs->backing.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block.c   | 48 +---
 block/mirror.c|  2 +-
 include/block/block.h |  3 ++-
 3 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/block.c b/block.c
index 675e5a8..0241acd 100644
--- a/block.c
+++ b/block.c
@@ -1182,30 +1182,43 @@ out:
 /*
  * Opens the backing file for a BlockDriverState if not yet open
  *
- * options is a QDict of options to pass to the block drivers, or NULL for an
- * empty set of options. The reference to the QDict is transferred to this
- * function (even on failure), so if the caller intends to reuse the 
dictionary,
- * it needs to use QINCREF() before calling bdrv_file_open.
+ * bdref_key specifies the key for the image's BlockdevRef in the options 
QDict.
+ * That QDict has to be flattened; therefore, if the BlockdevRef is a QDict
+ * itself, all options starting with "${bdref_key}." are considered part of the
+ * BlockdevRef.
+ *
+ * TODO Can this be unified with bdrv_open_image()?
  */
-int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
+int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
+   const char *bdref_key, Error **errp)
 {
 char *backing_filename = g_malloc0(PATH_MAX);
+char *bdref_key_dot;
+const char *reference = NULL;
 int ret = 0;
 BlockDriverState *backing_hd;
+QDict *options;
+QDict *tmp_parent_options = NULL;
 Error *local_err = NULL;
 
 if (bs->backing != NULL) {
-QDECREF(options);
 goto free_exit;
 }
 
 /* NULL means an empty set of options */
-if (options == NULL) {
-options = qdict_new();
+if (parent_options == NULL) {
+tmp_parent_options = qdict_new();
+parent_options = tmp_parent_options;
 }
 
 bs->open_flags &= ~BDRV_O_NO_BACKING;
-if (qdict_haskey(options, "file.filename")) {
+
+bdref_key_dot = g_strdup_printf("%s.", bdref_key);
+qdict_extract_subqdict(parent_options, , bdref_key_dot);
+g_free(bdref_key_dot);
+
+reference = qdict_get_try_str(parent_options, bdref_key);
+if (reference || qdict_haskey(options, "file.filename")) {
 backing_filename[0] = '\0';
 } else if (bs->backing_file[0] == '\0' && qdict_size(options) == 0) {
 QDECREF(options);
@@ -1228,19 +1241,16 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*options, Error **errp)
 goto free_exit;
 }
 
-backing_hd = bdrv_new();
-
 if (bs->backing_format[0] != '\0' && !qdict_haskey(options, "driver")) {
 qdict_put(options, "driver", qstring_from_str(bs->backing_format));
 }
 
-assert(bs->backing == NULL);
+backing_hd = NULL;
 ret = bdrv_open_inherit(_hd,
 *backing_filename ? backing_filename : NULL,
-NULL, options, 0, bs, _backing, _err);
+reference, options, 0, bs, _backing,
+_err);
 if (ret < 0) {
-bdrv_unref(backing_hd);
-backing_hd = NULL;
 bs->open_flags |= BDRV_O_NO_BACKING;
 error_setg(errp, "Could not open backing file: %s",
error_get_pretty(local_err));
@@ -1253,8 +1263,11 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict 
*options, Error **errp)
 bdrv_set_backing_hd(bs, backing_hd);
 bdrv_unref(backing_hd);
 
+qdict_del(parent_options, bdref_key);
+
 free_exit:
 g_free(backing_filename);
+QDECREF(tmp_parent_options);
 return ret;
 }
 
@@ -1537,10 +1550,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 
 /* If there is a backing file, use it */
 if ((flags & BDRV_O_NO_BACKING) == 0) {
-QDict *backing_options;
-
-qdict_extract_subqdict(options, _options, "backing.");
-ret = bdrv_open_backing_file(bs, backing_options, _err);
+ret = bdrv_open_backing_file(bs, options, "backing", _err);
 if (ret < 0) {
 goto close_and_fail;
 }
diff --git a/block/mirror.c b/block/mirror.c
index 8e3f524..fc34a9c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -652,7 +652,7 @@ static void mirror_complete(BlockJob *job, Error **errp)
 Error *local_err = NULL;
 int ret;
 
-ret = bdrv_open_backing_file(s->target, NULL, _err);
+ret = bdrv_open_backing_file(s->target, NULL, "backing", _err);
 if (ret < 0) {
 error_propagate(errp, local_err);
 return;
diff --git a/include/block/block.h b/include/block/block.h
index 3477328..589be29 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -210,7 +210,8 @@ BdrvChild *bdrv_open_child(const char *filename,
const BdrvChildRole *child_role,

[Qemu-block] [PATCH v3 10/21] block: reopen: Document option precedence and refactor accordingly

2015-12-04 Thread Kevin Wolf
The interesting part of reopening an image is from which sources the
effective options should be taken, i.e. which options take precedence
over which other options. This patch documents the precedence that will
be implemented in the following patches.

It also refactors bdrv_reopen_queue(), so that the top-level reopened
node is handled the same way as children are. Option/flag inheritance
from the parent becomes just one item in the list and is done at the
beginning of the function, similar to how the other items are/will be
handled.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block.c | 39 +--
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/block.c b/block.c
index a800d9e..235aa72 100644
--- a/block.c
+++ b/block.c
@@ -1693,9 +1693,13 @@ typedef struct BlockReopenQueueEntry {
  * bs_queue, or the existing bs_queue being used.
  *
  */
-BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
-BlockDriverState *bs,
-QDict *options, int flags)
+static BlockReopenQueue *bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
+ BlockDriverState *bs,
+ QDict *options,
+ int flags,
+ const BdrvChildRole *role,
+ QDict *parent_options,
+ int parent_flags)
 {
 assert(bs != NULL);
 
@@ -1712,6 +1716,22 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue 
*bs_queue,
 options = qdict_new();
 }
 
+/*
+ * Precedence of options:
+ * 1. Explicitly passed in options (highest)
+ * 2. TODO Set in flags (only for top level)
+ * 3. TODO Retained from explicitly set options of bs
+ * 4. TODO Inherited from parent node
+ * 5. Retained from effective options of bs
+ */
+
+/* Inherit from parent node */
+if (parent_options) {
+assert(!flags);
+flags = role->inherit_flags(parent_flags);
+}
+
+/* Old values are used for options that aren't set yet */
 old_options = qdict_clone_shallow(bs->options);
 bdrv_join_options(bs, options, old_options);
 QDECREF(old_options);
@@ -1722,7 +1742,6 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue 
*bs_queue,
 QLIST_FOREACH(child, >children, next) {
 QDict *new_child_options;
 char *child_key_dot;
-int child_flags;
 
 /* reopen can only change the options of block devices that were
  * implicitly created and inherited options. For other (referenced)
@@ -1735,8 +1754,8 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue 
*bs_queue,
 qdict_extract_subqdict(options, _child_options, child_key_dot);
 g_free(child_key_dot);
 
-child_flags = child->role->inherit_flags(flags);
-bdrv_reopen_queue(bs_queue, child->bs, new_child_options, child_flags);
+bdrv_reopen_queue_child(bs_queue, child->bs, new_child_options, 0,
+child->role, options, flags);
 }
 
 bs_entry = g_new0(BlockReopenQueueEntry, 1);
@@ -1749,6 +1768,14 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue 
*bs_queue,
 return bs_queue;
 }
 
+BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
+BlockDriverState *bs,
+QDict *options, int flags)
+{
+return bdrv_reopen_queue_child(bs_queue, bs, options, flags,
+   NULL, NULL, 0);
+}
+
 /*
  * Reopen multiple BlockDriverStates atomically & transactionally.
  *
-- 
1.8.3.1




Re: [Qemu-block] [PATCH v2 03/21] mirror: Error out when a BDS would get two BBs

2015-12-04 Thread Kevin Wolf
Am 30.11.2015 um 15:51 hat Alberto Garcia geschrieben:
> On Mon 23 Nov 2015 04:59:42 PM CET, Kevin Wolf wrote:
> 
> > @@ -370,11 +371,22 @@ static void mirror_exit(BlockJob *job, void *opaque)
> >  if (s->to_replace) {
> >  to_replace = s->to_replace;
> >  }
> > +
> > +/* This was checked in mirror_start_job(), but meanwhile one of the
> > + * nodes could have been newly attached to a BlockBackend. */
> > +if (to_replace->blk && s->target->blk) {
> > +error_report("block job: Can't create node with two 
> > BlockBackends");
> > +data->ret = -EINVAL;
> > +goto out;
> > +}
> 
> Does it make sense to even allow attaching a BDS to a Block Backend
> during this block job? Is there any use case for that?

Well, is there a good reason for forbidding it? I can imagine that
someone could have good reasons to start an NBD server on any node,
including nodes that are going to be replaced at some point.

The only reason for not allowing this is that a BDS can only have a
single BB, which is a limitation of the implementation rather than a
fundamental problem.

Kevin



[Qemu-block] [PATCH v3 11/21] block: Add infrastructure for option inheritance

2015-12-04 Thread Kevin Wolf
Options are not actually inherited from the parent node yet, but this
commit lays the grounds for doing so.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block.c   | 52 ---
 include/block/block_int.h |  3 ++-
 2 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/block.c b/block.c
index 235aa72..5b0183e 100644
--- a/block.c
+++ b/block.c
@@ -695,11 +695,14 @@ static int bdrv_temp_snapshot_flags(int flags)
 }
 
 /*
- * Returns the flags that bs->file should get if a protocol driver is expected,
- * based on the given flags for the parent BDS
+ * Returns the options and flags that bs->file should get if a protocol driver
+ * is expected, based on the given options and flags for the parent BDS
  */
-static int bdrv_inherited_flags(int flags)
+static void bdrv_inherited_options(int *child_flags, QDict *child_options,
+   int parent_flags, QDict *parent_options)
 {
+int flags = parent_flags;
+
 /* Enable protocol handling, disable format probing for bs->file */
 flags |= BDRV_O_PROTOCOL;
 
@@ -710,45 +713,51 @@ static int bdrv_inherited_flags(int flags)
 /* Clear flags that only apply to the top layer */
 flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_NO_BACKING | BDRV_O_COPY_ON_READ);
 
-return flags;
+*child_flags = flags;
 }
 
 const BdrvChildRole child_file = {
-.inherit_flags = bdrv_inherited_flags,
+.inherit_options = bdrv_inherited_options,
 };
 
 /*
- * Returns the flags that bs->file should get if the use of formats (and not
- * only protocols) is permitted for it, based on the given flags for the parent
- * BDS
+ * Returns the options and flags that bs->file should get if the use of formats
+ * (and not only protocols) is permitted for it, based on the given options and
+ * flags for the parent BDS
  */
-static int bdrv_inherited_fmt_flags(int parent_flags)
+static void bdrv_inherited_fmt_options(int *child_flags, QDict *child_options,
+   int parent_flags, QDict *parent_options)
 {
-int flags = child_file.inherit_flags(parent_flags);
-return flags & ~BDRV_O_PROTOCOL;
+child_file.inherit_options(child_flags, child_options,
+   parent_flags, parent_options);
+
+*child_flags &= ~BDRV_O_PROTOCOL;
 }
 
 const BdrvChildRole child_format = {
-.inherit_flags = bdrv_inherited_fmt_flags,
+.inherit_options = bdrv_inherited_fmt_options,
 };
 
 /*
- * Returns the flags that bs->backing should get, based on the given flags
- * for the parent BDS
+ * Returns the options and flags that bs->backing should get, based on the
+ * given options and flags for the parent BDS
  */
-static int bdrv_backing_flags(int flags)
+static void bdrv_backing_options(int *child_flags, QDict *child_options,
+ int parent_flags, QDict *parent_options)
 {
+int flags = parent_flags;
+
 /* backing files always opened read-only */
 flags &= ~(BDRV_O_RDWR | BDRV_O_COPY_ON_READ);
 
 /* snapshot=on is handled on the top layer */
 flags &= ~(BDRV_O_SNAPSHOT | BDRV_O_TEMPORARY);
 
-return flags;
+*child_flags = flags;
 }
 
 static const BdrvChildRole child_backing = {
-.inherit_flags = bdrv_backing_flags,
+.inherit_options = bdrv_backing_options,
 };
 
 static int bdrv_open_flags(BlockDriverState *bs, int flags)
@@ -1480,7 +1489,8 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 
 if (child_role) {
 bs->inherits_from = parent;
-flags = child_role->inherit_flags(parent->open_flags);
+child_role->inherit_options(, options,
+parent->open_flags, parent->options);
 }
 
 ret = bdrv_fill_options(, , , _err);
@@ -1518,7 +1528,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 }
 if (flags & BDRV_O_SNAPSHOT) {
 snapshot_flags = bdrv_temp_snapshot_flags(flags);
-flags = bdrv_backing_flags(flags);
+bdrv_backing_options(, options, flags, options);
 }
 
 bs->open_flags = flags;
@@ -1721,14 +1731,14 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
  * 1. Explicitly passed in options (highest)
  * 2. TODO Set in flags (only for top level)
  * 3. TODO Retained from explicitly set options of bs
- * 4. TODO Inherited from parent node
+ * 4. Inherited from parent node
  * 5. Retained from effective options of bs
  */
 
 /* Inherit from parent node */
 if (parent_options) {
 assert(!flags);
-flags = role->inherit_flags(parent_flags);
+role->inherit_options(, options, parent_flags, parent_options);
 }
 
 /* Old values are used for options that aren't set yet */
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 

[Qemu-block] [PATCH v3 19/21] qemu-iotests: Try setting cache mode for children

2015-12-04 Thread Kevin Wolf
This is a basic test for specifying cache modes for child nodes on the
command line. It doesn't take much time and works without O_DIRECT
support.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/051 | 10 +++-
 tests/qemu-iotests/051.out | 60 ++
 2 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index f6f0f4d..da90f59 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -61,7 +61,7 @@ function do_run_qemu()
 
 function run_qemu()
 {
-do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu | 
_filter_generated_node_ids
 }
 
 size=128M
@@ -190,6 +190,14 @@ run_qemu -drive driver=null-co,cache=writethrough
 run_qemu -drive driver=null-co,cache=unsafe
 run_qemu -drive driver=null-co,cache=invalid_value
 
+# Can't test direct=on here because O_DIRECT might not be supported on this FS
+# Test 142 checks the direct=on cases
+
+for cache in writeback writethrough unsafe invalid_value; do
+echo -e "info block\ninfo block file\ninfo block backing\ninfo block 
backing-file" | \
+run_qemu -drive 
file="$TEST_IMG",cache=$cache,backing.file.filename="$TEST_IMG.base",backing.cache.no-flush=on,backing.cache.writeback=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file
 -nodefaults
+done
+
 echo
 echo === Specifying the protocol layer ===
 echo
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 7a459a3..070d318 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -238,6 +238,66 @@ QEMU X.Y.Z monitor - type 'help' for more information
 Testing: -drive driver=null-co,cache=invalid_value
 QEMU_PROG: -drive driver=null-co,cache=invalid_value: invalid cache option
 
+Testing: -drive 
file=TEST_DIR/t.qcow2,cache=writeback,backing.file.filename=TEST_DIR/t.qcow2.base,backing.cache.no-flush=on,backing.cache.writeback=on,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file
 -nodefaults
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) iininfinfoinfo 
info binfo 
blinfo bloinfo 
blocinfo block
+ide0-hd0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
+Cache mode:   writeback
+Backing file: TEST_DIR/t.qcow2.base (chain depth: 1)
+(qemu) iininfinfoinfo 
info binfo 
blinfo bloinfo 
blocinfo 
blockinfo block 
info block 
finfo block 
fiinfo block 
filinfo block file
+
+file: TEST_DIR/t.qcow2 (file)
+Cache mode:   writeback
+(qemu) iininfinfoinfo 
info binfo 
blinfo bloinfo 
blocinfo 
blockinfo block 
info block 
binfo block 
bainfo block 
bacinfo block 
backinfo block 
backiinfo block 
backininfo block 
backing
+backing: TEST_DIR/t.qcow2.base (qcow2, read-only)
+Cache mode:   writeback, ignore flushes
+(qemu) iininfinfoinfo 
info binfo 
blinfo bloinfo 
blocinfo 
blockinfo block 
info block 
binfo block 
bainfo block 
bacinfo block 
backinfo block 
backiinfo block 
backininfo block 
backinginfo block 
backing-info block 
backing-finfo 
block backing-fi!
 info block 

[Qemu-block] [PATCH v3 07/21] block: Pass driver-specific options to .bdrv_refresh_filename()

2015-12-04 Thread Kevin Wolf
In order to decide whether a blkdebug: filename can be produced or a
json: one is necessary, blkdebug checked whether bs->options had more
options than just "config", "x-image" or "image" (the latter including
nested options). That doesn't work well when generic block layer options
are present.

This patch passes an option QDict to the driver that contains only
driver-specific options, i.e. the options for the general block layer as
well as child nodes are already filtered out. Works much better this
way.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block.c   |  5 -
 block/blkdebug.c  | 17 ++---
 block/blkverify.c |  2 +-
 block/nbd.c   | 10 +-
 block/quorum.c|  2 +-
 include/block/block_int.h |  2 +-
 6 files changed, 18 insertions(+), 20 deletions(-)

diff --git a/block.c b/block.c
index 0dfff7a..5ca7c71 100644
--- a/block.c
+++ b/block.c
@@ -4027,7 +4027,10 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 bs->full_open_options = NULL;
 }
 
-drv->bdrv_refresh_filename(bs);
+opts = qdict_new();
+append_open_options(opts, bs);
+drv->bdrv_refresh_filename(bs, opts);
+QDECREF(opts);
 } else if (bs->file) {
 /* Try to reconstruct valid information from the underlying file */
 bool has_open_options;
diff --git a/block/blkdebug.c b/block/blkdebug.c
index dee3a0e..48b0812 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -731,17 +731,15 @@ static int blkdebug_truncate(BlockDriverState *bs, 
int64_t offset)
 return bdrv_truncate(bs->file->bs, offset);
 }
 
-static void blkdebug_refresh_filename(BlockDriverState *bs)
+static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options)
 {
 QDict *opts;
 const QDictEntry *e;
 bool force_json = false;
 
-for (e = qdict_first(bs->options); e; e = qdict_next(bs->options, e)) {
+for (e = qdict_first(options); e; e = qdict_next(options, e)) {
 if (strcmp(qdict_entry_key(e), "config") &&
-strcmp(qdict_entry_key(e), "x-image") &&
-strcmp(qdict_entry_key(e), "image") &&
-strncmp(qdict_entry_key(e), "image.", strlen("image.")))
+strcmp(qdict_entry_key(e), "x-image"))
 {
 force_json = true;
 break;
@@ -757,7 +755,7 @@ static void blkdebug_refresh_filename(BlockDriverState *bs)
 if (!force_json && bs->file->bs->exact_filename[0]) {
 snprintf(bs->exact_filename, sizeof(bs->exact_filename),
  "blkdebug:%s:%s",
- qdict_get_try_str(bs->options, "config") ?: "",
+ qdict_get_try_str(options, "config") ?: "",
  bs->file->bs->exact_filename);
 }
 
@@ -767,11 +765,8 @@ static void blkdebug_refresh_filename(BlockDriverState *bs)
 QINCREF(bs->file->bs->full_open_options);
 qdict_put_obj(opts, "image", QOBJECT(bs->file->bs->full_open_options));
 
-for (e = qdict_first(bs->options); e; e = qdict_next(bs->options, e)) {
-if (strcmp(qdict_entry_key(e), "x-image") &&
-strcmp(qdict_entry_key(e), "image") &&
-strncmp(qdict_entry_key(e), "image.", strlen("image.")))
-{
+for (e = qdict_first(options); e; e = qdict_next(options, e)) {
+if (strcmp(qdict_entry_key(e), "x-image")) {
 qobject_incref(qdict_entry_value(e));
 qdict_put_obj(opts, qdict_entry_key(e), qdict_entry_value(e));
 }
diff --git a/block/blkverify.c b/block/blkverify.c
index c5f8e8d..1d75449 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -307,7 +307,7 @@ static void blkverify_attach_aio_context(BlockDriverState 
*bs,
 bdrv_attach_aio_context(s->test_file->bs, new_context);
 }
 
-static void blkverify_refresh_filename(BlockDriverState *bs)
+static void blkverify_refresh_filename(BlockDriverState *bs, QDict *options)
 {
 BDRVBlkverifyState *s = bs->opaque;
 
diff --git a/block/nbd.c b/block/nbd.c
index cd6a587..416f42b 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -342,13 +342,13 @@ static void nbd_attach_aio_context(BlockDriverState *bs,
 nbd_client_attach_aio_context(bs, new_context);
 }
 
-static void nbd_refresh_filename(BlockDriverState *bs)
+static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
 {
 QDict *opts = qdict_new();
-const char *path   = qdict_get_try_str(bs->options, "path");
-const char *host   = qdict_get_try_str(bs->options, "host");
-const char *port   = qdict_get_try_str(bs->options, "port");
-const char *export = qdict_get_try_str(bs->options, "export");
+const char *path   = qdict_get_try_str(options, "path");
+const char *host   = qdict_get_try_str(options, "host");
+const char *port   = qdict_get_try_str(options, "port");
+const char *export = qdict_get_try_str(options, "export");
 
 

[Qemu-block] [PATCH v3 14/21] blockdev: Set 'format' indicates non-empty drive

2015-12-04 Thread Kevin Wolf
Creating an empty drive while specifying 'format' doesn't make sense.
The specified format driver would simply be ignored.

Make a set 'format' option an indication that a non-empty drive should
be created. This makes 'format' consistent with 'driver' and allows
using it with a block driver that doesn't need any other options (like
null-co/null-aio).

Signed-off-by: Kevin Wolf 
---
 blockdev.c| 5 +
 tests/hd-geo-test.c   | 4 ++--
 tests/qemu-iotests/iotests.py | 2 +-
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 313841b..afaeef9 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -490,7 +490,6 @@ static BlockBackend *blockdev_init(const char *file, QDict 
*bs_opts,
 QDict *interval_dict = NULL;
 QList *interval_list = NULL;
 const char *id;
-bool has_driver_specific_opts;
 BlockdevDetectZeroesOptions detect_zeroes =
 BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF;
 const char *throttling_group = NULL;
@@ -514,8 +513,6 @@ static BlockBackend *blockdev_init(const char *file, QDict 
*bs_opts,
 qdict_del(bs_opts, "id");
 }
 
-has_driver_specific_opts = !!qdict_size(bs_opts);
-
 /* extract parameters */
 snapshot = qemu_opt_get_bool(opts, "snapshot", 0);
 
@@ -578,7 +575,7 @@ static BlockBackend *blockdev_init(const char *file, QDict 
*bs_opts,
 }
 
 /* init */
-if ((!file || !*file) && !has_driver_specific_opts) {
+if ((!file || !*file) && !qdict_size(bs_opts)) {
 BlockBackendRootState *blk_rs;
 
 blk = blk_new(qemu_opts_id(opts), errp);
diff --git a/tests/hd-geo-test.c b/tests/hd-geo-test.c
index 00afc20..13b763d 100644
--- a/tests/hd-geo-test.c
+++ b/tests/hd-geo-test.c
@@ -206,13 +206,13 @@ static int setup_ide(int argc, char *argv[], int argv_sz,
 {
 char *s1, *s2, *s3;
 
-s1 = g_strdup_printf("-drive id=drive%d,if=%s,format=raw",
+s1 = g_strdup_printf("-drive id=drive%d,if=%s",
  ide_idx, dev ? "none" : "ide");
 s2 = dev ? g_strdup("") : g_strdup_printf(",index=%d", ide_idx);
 
 if (img_secs[img_idx] >= 0) {
 setup_mbr(img_idx, mbr);
-s3 = g_strdup_printf(",file=%s", img_file_name[img_idx]);
+s3 = g_strdup_printf(",format=raw,file=%s", img_file_name[img_idx]);
 } else {
 s3 = g_strdup(",media=cdrom");
 }
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index e02245e..27eddec 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -148,12 +148,12 @@ class VM(object):
 def add_drive(self, path, opts='', interface='virtio'):
 '''Add a virtio-blk drive to the VM'''
 options = ['if=%s' % interface,
-   'format=%s' % imgfmt,
'cache=%s' % cachemode,
'id=drive%d' % self._num_drives]
 
 if path is not None:
 options.append('file=%s' % path)
+options.append('format=%s' % imgfmt)
 
 if opts:
 options.append(opts)
-- 
1.8.3.1




[Qemu-block] [PATCH v3 16/21] block: reopen: Extract QemuOpts for generic block layer options

2015-12-04 Thread Kevin Wolf
This patch adds a QemuOpts for generic block layer options to
bdrv_reopen_prepare(). The only two options that currently exist
(node-name and driver) cannot be changed, so the only thing we do is
putting them right back into the QDict so that we check at the end that
they are indeed unchanged.

We will add new options soon that can actually be changed.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/block.c b/block.c
index 2dd2830..07763ff 100644
--- a/block.c
+++ b/block.c
@@ -1907,11 +1907,34 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 int ret = -1;
 Error *local_err = NULL;
 BlockDriver *drv;
+QemuOpts *opts;
+const char *value;
 
 assert(reopen_state != NULL);
 assert(reopen_state->bs->drv != NULL);
 drv = reopen_state->bs->drv;
 
+/* Process generic block layer options */
+opts = qemu_opts_create(_runtime_opts, NULL, 0, _abort);
+qemu_opts_absorb_qdict(opts, reopen_state->options, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+ret = -EINVAL;
+goto error;
+}
+
+/* node-name and driver must be unchanged. Put them back into the QDict, so
+ * that they are checked at the end of this function. */
+value = qemu_opt_get(opts, "node-name");
+if (value) {
+qdict_put(reopen_state->options, "node-name", qstring_from_str(value));
+}
+
+value = qemu_opt_get(opts, "driver");
+if (value) {
+qdict_put(reopen_state->options, "driver", qstring_from_str(value));
+}
+
 /* if we are to stay read-only, do not allow permission change
  * to r/w */
 if (!(reopen_state->bs->open_flags & BDRV_O_ALLOW_RDWR) &&
@@ -1972,6 +1995,7 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
BlockReopenQueue *queue,
 ret = 0;
 
 error:
+qemu_opts_del(opts);
 return ret;
 }
 
-- 
1.8.3.1




[Qemu-block] [PATCH v3 15/21] qemu-iotests: Remove cache mode test without medium

2015-12-04 Thread Kevin Wolf
Specifying the cache mode for a driver without a medium is not a useful
thing to do: As long as there is no medium, the cache mode doesn't make
a difference, and once the 'change' command is used to insert a medium,
it ignores the old cache mode and makes the new medium use
cache=writethrough.

Later patches will make it an error to specify the cache mode for an
empty drive. Remove the corresponding test case.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/051| 12 ++--
 tests/qemu-iotests/051.out| 14 +++---
 tests/qemu-iotests/iotests.py |  2 +-
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/tests/qemu-iotests/051 b/tests/qemu-iotests/051
index 17dbf04..f6f0f4d 100755
--- a/tests/qemu-iotests/051
+++ b/tests/qemu-iotests/051
@@ -183,12 +183,12 @@ echo
 # Cannot use the test image because cache=none might not work on the host FS
 # Use cdrom so that we won't get errors about missing media
 
-run_qemu -drive media=cdrom,cache=none
-run_qemu -drive media=cdrom,cache=directsync
-run_qemu -drive media=cdrom,cache=writeback
-run_qemu -drive media=cdrom,cache=writethrough
-run_qemu -drive media=cdrom,cache=unsafe
-run_qemu -drive media=cdrom,cache=invalid_value
+run_qemu -drive driver=null-co,cache=none
+run_qemu -drive driver=null-co,cache=directsync
+run_qemu -drive driver=null-co,cache=writeback
+run_qemu -drive driver=null-co,cache=writethrough
+run_qemu -drive driver=null-co,cache=unsafe
+run_qemu -drive driver=null-co,cache=invalid_value
 
 echo
 echo === Specifying the protocol layer ===
diff --git a/tests/qemu-iotests/051.out b/tests/qemu-iotests/051.out
index 7765aa0..7a459a3 100644
--- a/tests/qemu-iotests/051.out
+++ b/tests/qemu-iotests/051.out
@@ -215,28 +215,28 @@ QEMU X.Y.Z monitor - type 'help' for more information
 
 === Cache modes ===
 
-Testing: -drive media=cdrom,cache=none
+Testing: -drive driver=null-co,cache=none
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) qququiquit
 
-Testing: -drive media=cdrom,cache=directsync
+Testing: -drive driver=null-co,cache=directsync
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) qququiquit
 
-Testing: -drive media=cdrom,cache=writeback
+Testing: -drive driver=null-co,cache=writeback
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) qququiquit
 
-Testing: -drive media=cdrom,cache=writethrough
+Testing: -drive driver=null-co,cache=writethrough
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) qququiquit
 
-Testing: -drive media=cdrom,cache=unsafe
+Testing: -drive driver=null-co,cache=unsafe
 QEMU X.Y.Z monitor - type 'help' for more information
 (qemu) qququiquit
 
-Testing: -drive media=cdrom,cache=invalid_value
-QEMU_PROG: -drive media=cdrom,cache=invalid_value: invalid cache option
+Testing: -drive driver=null-co,cache=invalid_value
+QEMU_PROG: -drive driver=null-co,cache=invalid_value: invalid cache option
 
 
 === Specifying the protocol layer ===
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 27eddec..0a238ec 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -148,12 +148,12 @@ class VM(object):
 def add_drive(self, path, opts='', interface='virtio'):
 '''Add a virtio-blk drive to the VM'''
 options = ['if=%s' % interface,
-   'cache=%s' % cachemode,
'id=drive%d' % self._num_drives]
 
 if path is not None:
 options.append('file=%s' % path)
 options.append('format=%s' % imgfmt)
+options.append('cache=%s' % cachemode)
 
 if opts:
 options.append(opts)
-- 
1.8.3.1




[Qemu-block] [PATCH v3 12/21] block: Split out parse_json_protocol()

2015-12-04 Thread Kevin Wolf
The next patch distinguishes options that were explicitly set and
options that were derived. bdrv_fill_option() added options of both
types: Options given by json: syntax should be counted as explicit, but
the rest is derived.

In preparation for the distinction, move json: parse to a separate
function.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block.c | 50 --
 1 file changed, 32 insertions(+), 18 deletions(-)

diff --git a/block.c b/block.c
index 5b0183e..8ac1669 100644
--- a/block.c
+++ b/block.c
@@ -1018,37 +1018,45 @@ static QDict *parse_json_filename(const char *filename, 
Error **errp)
 return options;
 }
 
+static void parse_json_protocol(QDict *options, const char **pfilename,
+Error **errp)
+{
+QDict *json_options;
+Error *local_err = NULL;
+
+/* Parse json: pseudo-protocol */
+if (!*pfilename || !g_str_has_prefix(*pfilename, "json:")) {
+return;
+}
+
+json_options = parse_json_filename(*pfilename, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+return;
+}
+
+/* Options given in the filename have lower priority than options
+ * specified directly */
+qdict_join(options, json_options, false);
+QDECREF(json_options);
+*pfilename = NULL;
+}
+
 /*
  * Fills in default options for opening images and converts the legacy
  * filename/flags pair to option QDict entries.
  * The BDRV_O_PROTOCOL flag in *flags will be set or cleared accordingly if a
  * block driver has been specified explicitly.
  */
-static int bdrv_fill_options(QDict **options, const char **pfilename,
+static int bdrv_fill_options(QDict **options, const char *filename,
  int *flags, Error **errp)
 {
-const char *filename = *pfilename;
 const char *drvname;
 bool protocol = *flags & BDRV_O_PROTOCOL;
 bool parse_filename = false;
 BlockDriver *drv = NULL;
 Error *local_err = NULL;
 
-/* Parse json: pseudo-protocol */
-if (filename && g_str_has_prefix(filename, "json:")) {
-QDict *json_options = parse_json_filename(filename, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-return -EINVAL;
-}
-
-/* Options given in the filename have lower priority than options
- * specified directly */
-qdict_join(*options, json_options, false);
-QDECREF(json_options);
-*pfilename = filename = NULL;
-}
-
 drvname = qdict_get_try_str(*options, "driver");
 if (drvname) {
 drv = bdrv_find_format(drvname);
@@ -1487,13 +1495,19 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 options = qdict_new();
 }
 
+parse_json_protocol(options, , _err);
+if (local_err) {
+ret = -EINVAL;
+goto fail;
+}
+
 if (child_role) {
 bs->inherits_from = parent;
 child_role->inherit_options(, options,
 parent->open_flags, parent->options);
 }
 
-ret = bdrv_fill_options(, , , _err);
+ret = bdrv_fill_options(, filename, , _err);
 if (local_err) {
 goto fail;
 }
-- 
1.8.3.1




[Qemu-block] [PATCH v3 20/21] qemu-iotests: Test cache mode option inheritance

2015-12-04 Thread Kevin Wolf
This is doing a more complete test on setting cache modes both while
opening an image (i.e. in a -drive command line) and in reopen
situations. It checks that reopen can specify options for child nodes
and that cache modes are correctly inherited from parent nodes where
they are not specified.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/142 | 354 +
 tests/qemu-iotests/142.out | 773 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 1128 insertions(+)
 create mode 100755 tests/qemu-iotests/142
 create mode 100644 tests/qemu-iotests/142.out

diff --git a/tests/qemu-iotests/142 b/tests/qemu-iotests/142
new file mode 100755
index 000..8aa50f8
--- /dev/null
+++ b/tests/qemu-iotests/142
@@ -0,0 +1,354 @@
+#!/bin/bash
+#
+# Test for configuring cache modes of arbitrary nodes (requires O_DIRECT)
+#
+# 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 .
+#
+
+# creator
+owner=kw...@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
+rm -f $TEST_IMG.snap
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+# We test all cache modes anyway, but O_DIRECT needs to be supported
+_default_cache_mode none
+_supported_cache_modes none directsync
+
+function do_run_qemu()
+{
+echo Testing: "$@"
+(
+if ! test -t 0; then
+while read cmd; do
+echo $cmd
+done
+fi
+echo quit
+) | $QEMU -nographic -monitor stdio -nodefaults "$@"
+echo
+}
+
+function run_qemu()
+{
+do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qemu
+}
+
+size=128M
+
+TEST_IMG="$TEST_IMG.base" _make_test_img $size
+TEST_IMG="$TEST_IMG.snap" _make_test_img $size
+_make_test_img -b "$TEST_IMG.base" $size
+
+echo
+echo === Simple test for all cache modes ===
+echo
+
+run_qemu -drive file="$TEST_IMG",cache=none
+run_qemu -drive file="$TEST_IMG",cache=directsync
+run_qemu -drive file="$TEST_IMG",cache=writeback
+run_qemu -drive file="$TEST_IMG",cache=writethrough
+run_qemu -drive file="$TEST_IMG",cache=unsafe
+run_qemu -drive file="$TEST_IMG",cache=invalid_value
+
+echo
+echo === Check inheritance of cache modes ===
+echo
+
+files="if=none,file=$TEST_IMG,backing.file.filename=$TEST_IMG.base"
+ids="node-name=image,backing.node-name=backing,backing.file.node-name=backing-file,file.node-name=file"
+
+function check_cache_all()
+{
+# cache.direct is supposed to be inherited by both bs->file and
+# bs->backing
+
+echo -e "cache.direct=on on none0"
+echo "$hmp_cmds" | run_qemu -drive "$files","$ids",cache.direct=on | grep 
"Cache"
+echo -e "\ncache.direct=on on file"
+echo "$hmp_cmds" | run_qemu -drive "$files","$ids",file.cache.direct=on | 
grep "Cache"
+echo -e "\ncache.direct=on on backing"
+echo "$hmp_cmds" | run_qemu -drive "$files","$ids",backing.cache.direct=on 
| grep "Cache"
+echo -e "\ncache.direct=on on backing-file"
+echo "$hmp_cmds" | run_qemu -drive 
"$files","$ids",backing.file.cache.direct=on | grep "Cache"
+
+# cache.writeback is supposed to be inherited by bs->backing; bs->file
+# always gets cache.writeback=on
+
+echo -e "\n\ncache.writeback=off on none0"
+echo "$hmp_cmds" | run_qemu -drive "$files","$ids",cache.writeback=off | 
grep "Cache"
+echo -e "\ncache.writeback=off on file"
+echo "$hmp_cmds" | run_qemu -drive 
"$files","$ids",file.cache.writeback=off | grep "Cache"
+echo -e "\ncache.writeback=off on backing"
+echo "$hmp_cmds" | run_qemu -drive 
"$files","$ids",backing.cache.writeback=off | grep "Cache"
+echo -e "\ncache.writeback=off on backing-file"
+echo "$hmp_cmds" | run_qemu -drive 
"$files","$ids",backing.file.cache.writeback=off | grep "Cache"
+
+# cache.no-flush is supposed to be inherited by both bs->file and 
bs->backing
+
+echo -e "\n\ncache.no-flush=on on none0"
+echo "$hmp_cmds" | run_qemu -drive "$files","$ids",cache.no-flush=on | 
grep "Cache"
+echo -e "\ncache.no-flush=on on file"
+echo "$hmp_cmds" | run_qemu -drive 

[Qemu-block] [PATCH v3 21/21] qemu-iotests: Test reopen with node-name/driver options

2015-12-04 Thread Kevin Wolf
'node-name' and 'driver' should not be changed during a reopen
operation. It is, however, valid to specify them with the same value as
they already had.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 tests/qemu-iotests/133 | 90 ++
 tests/qemu-iotests/133.out | 22 
 tests/qemu-iotests/group   |  1 +
 3 files changed, 113 insertions(+)
 create mode 100755 tests/qemu-iotests/133
 create mode 100644 tests/qemu-iotests/133.out

diff --git a/tests/qemu-iotests/133 b/tests/qemu-iotests/133
new file mode 100755
index 000..8587102
--- /dev/null
+++ b/tests/qemu-iotests/133
@@ -0,0 +1,90 @@
+#!/bin/bash
+#
+# Test for 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 .
+#
+
+# creator
+owner=kw...@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
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+TEST_IMG="$TEST_IMG.base" _make_test_img 64M
+_make_test_img -b "$TEST_IMG.base"
+
+echo
+echo "=== Check that node-name can't be changed ==="
+echo
+
+$QEMU_IO -c 'reopen -o node-name=foo' $TEST_IMG
+$QEMU_IO -c 'reopen -o file.node-name=foo' $TEST_IMG
+$QEMU_IO -c 'reopen -o backing.node-name=foo' $TEST_IMG
+
+echo
+echo "=== Check that unchanged node-name is okay ==="
+echo
+
+# Explicitly repeated
+$QEMU_IO -c "open -o node-name=foo $TEST_IMG" -c 'reopen -o node-name=foo'
+$QEMU_IO -c "open -o file.node-name=foo $TEST_IMG" -c 'reopen -o 
file.node-name=foo'
+$QEMU_IO -c "open -o backing.node-name=foo $TEST_IMG" -c 'reopen -o 
backing.node-name=foo'
+
+# Implicitly retained
+$QEMU_IO -c "open -o node-name=foo $TEST_IMG" -c 'reopen'
+$QEMU_IO -c "open -o file.node-name=foo $TEST_IMG" -c 'reopen'
+$QEMU_IO -c "open -o backing.node-name=foo $TEST_IMG" -c 'reopen'
+
+echo
+echo "=== Check that driver can't be changed ==="
+echo
+
+$QEMU_IO -c 'reopen -o driver=raw' $TEST_IMG
+$QEMU_IO -c 'reopen -o file.driver=qcow2' $TEST_IMG
+$QEMU_IO -c 'reopen -o backing.driver=file' $TEST_IMG
+
+echo
+echo "=== Check that unchanged driver is okay ==="
+echo
+
+# Explicitly repeated (implicit case is covered in node-name test)
+$QEMU_IO -c 'reopen -o driver=qcow2' $TEST_IMG
+$QEMU_IO -c 'reopen -o file.driver=file' $TEST_IMG
+$QEMU_IO -c 'reopen -o backing.driver=qcow2' $TEST_IMG
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/133.out b/tests/qemu-iotests/133.out
new file mode 100644
index 000..cc86b94
--- /dev/null
+++ b/tests/qemu-iotests/133.out
@@ -0,0 +1,22 @@
+QA output created by 133
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 
backing_file=TEST_DIR/t.IMGFMT.base
+
+=== Check that node-name can't be changed ===
+
+Cannot change the option 'node-name'
+Cannot change the option 'node-name'
+Cannot change the option 'node-name'
+
+=== Check that unchanged node-name is okay ===
+
+
+=== Check that driver can't be changed ===
+
+Cannot change the option 'driver'
+Cannot change the option 'driver'
+Cannot change the option 'driver'
+
+=== Check that unchanged driver is okay ===
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index ed6ce09..d6e9219 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -134,6 +134,7 @@
 130 rw auto quick
 131 rw auto quick
 132 rw auto quick
+133 auto quick
 134 rw auto quick
 135 rw auto
 136 rw auto
-- 
1.8.3.1




[Qemu-block] [PATCH] xen_disk: treat "vhd" as "vpc"

2015-12-04 Thread Stefano Stabellini
The Xen toolstack uses "vhd" to specify a disk in VHD format, however
the name of the driver in QEMU is "vpc". Replace "vhd" with "vpc", so
that QEMU can find the right driver to use for it.

Signed-off-by: Stefano Stabellini 

diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 267d8a8..37e14d1 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -811,6 +811,9 @@ static int blk_init(struct XenDevice *xendev)
 if (!strcmp("aio", blkdev->fileproto)) {
 blkdev->fileproto = "raw";
 }
+if (!strcmp("vhd", blkdev->fileproto)) {
+blkdev->fileproto = "vpc";
+}
 if (blkdev->mode == NULL) {
 blkdev->mode = xenstore_read_be_str(>xendev, "mode");
 }



[Qemu-block] [PATCH v3 06/21] block: Exclude nested options only for children in append_open_options()

2015-12-04 Thread Kevin Wolf
Some drivers have nested options (e.g. blkdebug rule arrays), which
don't belong to a child node and shouldn't be removed. Don't remove all
options with "." in their name, but check for the complete prefixes of
actually existing child nodes.

Signed-off-by: Kevin Wolf 
---
 block.c   | 20 
 include/block/block_int.h |  1 +
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 73f0816..0dfff7a 100644
--- a/block.c
+++ b/block.c
@@ -1101,11 +1101,13 @@ static int bdrv_fill_options(QDict **options, const 
char **pfilename,
 
 static BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
 BlockDriverState *child_bs,
+const char *child_name,
 const BdrvChildRole *child_role)
 {
 BdrvChild *child = g_new(BdrvChild, 1);
 *child = (BdrvChild) {
 .bs = child_bs,
+.name   = g_strdup(child_name),
 .role   = child_role,
 };
 
@@ -1119,6 +1121,7 @@ static void bdrv_detach_child(BdrvChild *child)
 {
 QLIST_REMOVE(child, next);
 QLIST_REMOVE(child, next_parent);
+g_free(child->name);
 g_free(child);
 }
 
@@ -1165,7 +1168,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd)
 bs->backing = NULL;
 goto out;
 }
-bs->backing = bdrv_attach_child(bs, backing_hd, _backing);
+bs->backing = bdrv_attach_child(bs, backing_hd, "backing", _backing);
 bs->open_flags &= ~BDRV_O_NO_BACKING;
 pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename);
 pstrcpy(bs->backing_format, sizeof(bs->backing_format),
@@ -1321,7 +1324,7 @@ BdrvChild *bdrv_open_child(const char *filename,
 goto done;
 }
 
-c = bdrv_attach_child(parent, bs, child_role);
+c = bdrv_attach_child(parent, bs, bdref_key, child_role);
 
 done:
 qdict_del(options, bdref_key);
@@ -3951,13 +3954,22 @@ static bool append_open_options(QDict *d, 
BlockDriverState *bs)
 {
 const QDictEntry *entry;
 QemuOptDesc *desc;
+BdrvChild *child;
 bool found_any = false;
+const char *p;
 
 for (entry = qdict_first(bs->options); entry;
  entry = qdict_next(bs->options, entry))
 {
-/* Only take options for this level */
-if (strchr(qdict_entry_key(entry), '.')) {
+/* Exclude options for children */
+QLIST_FOREACH(child, >children, next) {
+if (strstart(qdict_entry_key(entry), child->name, )
+&& (!*p || *p == '.'))
+{
+break;
+}
+}
+if (child) {
 continue;
 }
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 77dc165..7265247 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -351,6 +351,7 @@ extern const BdrvChildRole child_format;
 
 struct BdrvChild {
 BlockDriverState *bs;
+char *name;
 const BdrvChildRole *role;
 QLIST_ENTRY(BdrvChild) next;
 QLIST_ENTRY(BdrvChild) next_parent;
-- 
1.8.3.1




[Qemu-block] [PATCH v3 05/21] block: Consider all block layer options in append_open_options

2015-12-04 Thread Kevin Wolf
The code already special-cased "node-name", which is currently the only
option passed in the QDict that isn't driver-specific. Generalise the
code to take all general block layer options into consideration.

Signed-off-by: Kevin Wolf 
Reviewed-by: Eric Blake 
Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block.c | 26 ++
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index 0241acd..73f0816 100644
--- a/block.c
+++ b/block.c
@@ -3950,20 +3950,30 @@ out:
 static bool append_open_options(QDict *d, BlockDriverState *bs)
 {
 const QDictEntry *entry;
+QemuOptDesc *desc;
 bool found_any = false;
 
 for (entry = qdict_first(bs->options); entry;
  entry = qdict_next(bs->options, entry))
 {
-/* Only take options for this level and exclude all non-driver-specific
- * options */
-if (!strchr(qdict_entry_key(entry), '.') &&
-strcmp(qdict_entry_key(entry), "node-name"))
-{
-qobject_incref(qdict_entry_value(entry));
-qdict_put_obj(d, qdict_entry_key(entry), qdict_entry_value(entry));
-found_any = true;
+/* Only take options for this level */
+if (strchr(qdict_entry_key(entry), '.')) {
+continue;
 }
+
+/* And exclude all non-driver-specific options */
+for (desc = bdrv_runtime_opts.desc; desc->name; desc++) {
+if (!strcmp(qdict_entry_key(entry), desc->name)) {
+break;
+}
+}
+if (desc->name) {
+continue;
+}
+
+qobject_incref(qdict_entry_value(entry));
+qdict_put_obj(d, qdict_entry_key(entry), qdict_entry_value(entry));
+found_any = true;
 }
 
 return found_any;
-- 
1.8.3.1




[Qemu-block] [PATCH v3 09/21] block: Allow specifying child options in reopen

2015-12-04 Thread Kevin Wolf
If the child was defined in the same context (-drive argument or
blockdev-add QMP command) as its parent, a reopen of the parent should
work the same and allow changing options of the child.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
Reviewed-by: Alberto Garcia 
---
 block.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 2728bc5..a800d9e 100644
--- a/block.c
+++ b/block.c
@@ -1720,15 +1720,23 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue 
*bs_queue,
 flags &= ~BDRV_O_PROTOCOL;
 
 QLIST_FOREACH(child, >children, next) {
+QDict *new_child_options;
+char *child_key_dot;
 int child_flags;
 
+/* reopen can only change the options of block devices that were
+ * implicitly created and inherited options. For other (referenced)
+ * block devices, a syntax like "backing.foo" results in an error. */
 if (child->bs->inherits_from != bs) {
 continue;
 }
 
+child_key_dot = g_strdup_printf("%s.", child->name);
+qdict_extract_subqdict(options, _child_options, child_key_dot);
+g_free(child_key_dot);
+
 child_flags = child->role->inherit_flags(flags);
-/* TODO Pass down child flags (backing.*, extents.*, ...) */
-bdrv_reopen_queue(bs_queue, child->bs, NULL, child_flags);
+bdrv_reopen_queue(bs_queue, child->bs, new_child_options, child_flags);
 }
 
 bs_entry = g_new0(BlockReopenQueueEntry, 1);
-- 
1.8.3.1




[Qemu-block] [PATCH v3 13/21] block: Introduce bs->explicit_options

2015-12-04 Thread Kevin Wolf
bs->options doesn't only contain options that the user explicitly
requested, but also option that were derived from flags, the filename or
inherited from the parent node.

For reopen, it is important to know the difference because reopening the
parent can change inherited values in child nodes, but it shouldn't
change any options that were explicitly specified for the child.

Signed-off-by: Kevin Wolf 
Reviewed-by: Max Reitz 
---
 block.c   | 24 ++--
 include/block/block.h |  1 +
 include/block/block_int.h |  1 +
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 8ac1669..2dd2830 100644
--- a/block.c
+++ b/block.c
@@ -1495,12 +1495,15 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
const char *filename,
 options = qdict_new();
 }
 
+/* json: syntax counts as explicit options, as if in the QDict */
 parse_json_protocol(options, , _err);
 if (local_err) {
 ret = -EINVAL;
 goto fail;
 }
 
+bs->explicit_options = qdict_clone_shallow(options);
+
 if (child_role) {
 bs->inherits_from = parent;
 child_role->inherit_options(, options,
@@ -1655,6 +1658,7 @@ fail:
 if (file != NULL) {
 bdrv_unref_child(bs, file);
 }
+QDECREF(bs->explicit_options);
 QDECREF(bs->options);
 QDECREF(options);
 bs->options = NULL;
@@ -1729,7 +1733,7 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
 BlockReopenQueueEntry *bs_entry;
 BdrvChild *child;
-QDict *old_options;
+QDict *old_options, *explicit_options;
 
 if (bs_queue == NULL) {
 bs_queue = g_new0(BlockReopenQueue, 1);
@@ -1744,11 +1748,18 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
  * Precedence of options:
  * 1. Explicitly passed in options (highest)
  * 2. TODO Set in flags (only for top level)
- * 3. TODO Retained from explicitly set options of bs
+ * 3. Retained from explicitly set options of bs
  * 4. Inherited from parent node
  * 5. Retained from effective options of bs
  */
 
+/* Old explicitly set values (don't overwrite by inherited value) */
+old_options = qdict_clone_shallow(bs->explicit_options);
+bdrv_join_options(bs, options, old_options);
+QDECREF(old_options);
+
+explicit_options = qdict_clone_shallow(options);
+
 /* Inherit from parent node */
 if (parent_options) {
 assert(!flags);
@@ -1787,6 +1798,7 @@ static BlockReopenQueue 
*bdrv_reopen_queue_child(BlockReopenQueue *bs_queue,
 
 bs_entry->state.bs = bs;
 bs_entry->state.options = options;
+bs_entry->state.explicit_options = explicit_options;
 bs_entry->state.flags = flags;
 
 return bs_queue;
@@ -1846,6 +1858,8 @@ cleanup:
 QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
 if (ret && bs_entry->prepared) {
 bdrv_reopen_abort(_entry->state);
+} else if (ret) {
+QDECREF(bs_entry->state.explicit_options);
 }
 QDECREF(bs_entry->state.options);
 g_free(bs_entry);
@@ -1980,6 +1994,9 @@ void bdrv_reopen_commit(BDRVReopenState *reopen_state)
 }
 
 /* set BDS specific flags now */
+QDECREF(reopen_state->bs->explicit_options);
+
+reopen_state->bs->explicit_options   = reopen_state->explicit_options;
 reopen_state->bs->open_flags = reopen_state->flags;
 reopen_state->bs->enable_write_cache = !!(reopen_state->flags &
   BDRV_O_CACHE_WB);
@@ -2003,6 +2020,8 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
 if (drv->bdrv_reopen_abort) {
 drv->bdrv_reopen_abort(reopen_state);
 }
+
+QDECREF(reopen_state->explicit_options);
 }
 
 
@@ -2061,6 +2080,7 @@ void bdrv_close(BlockDriverState *bs)
 bs->sg = 0;
 bs->zero_beyond_eof = false;
 QDECREF(bs->options);
+QDECREF(bs->explicit_options);
 bs->options = NULL;
 QDECREF(bs->full_open_options);
 bs->full_open_options = NULL;
diff --git a/include/block/block.h b/include/block/block.h
index 589be29..17557c3 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -150,6 +150,7 @@ typedef struct BDRVReopenState {
 BlockDriverState *bs;
 int flags;
 QDict *options;
+QDict *explicit_options;
 void *opaque;
 } BDRVReopenState;
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 04daa55..6d7bd3b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -459,6 +459,7 @@ struct BlockDriverState {
 QLIST_HEAD(, BdrvChild) parents;
 
 QDict *options;
+QDict *explicit_options;
 BlockdevDetectZeroesOptions detect_zeroes;
 
 /* The error object in use for blocking operations on backing_hd */
-- 
1.8.3.1




Re: [Qemu-block] [Qemu-devel] [PATCH for-2.6 0/2] Preparation for PCI devices convert to realize()

2015-12-04 Thread John Snow


On 12/04/2015 02:47 AM, Cao jin wrote:
> Hi,
> As you know, there are many PCI devices still using .init() as its
> initialization function, I am planning to do the "convert to realize()"
> work, and PCI bridge devices are chosen first.
> The supporting functions should be modified first. msi_init() a supporting
> function for PCI devices.
> 
> Maybe it should be put in 2.6, as title indicated
> 
> Cao jin (2):
>   Add param Error** to msi_init()
>   Modify callers of msi_init()
> 
>  hw/audio/intel-hda.c   |  7 ++-
>  hw/ide/ich.c   |  2 +-
>  hw/net/vmxnet3.c   |  3 ++-
>  hw/pci-bridge/ioh3420.c|  6 +-
>  hw/pci-bridge/pci_bridge_dev.c |  6 +-
>  hw/pci-bridge/xio3130_downstream.c |  7 ++-
>  hw/pci-bridge/xio3130_upstream.c   |  7 ++-
>  hw/pci/msi.c   | 17 +
>  hw/scsi/megasas.c  |  2 +-
>  hw/scsi/vmw_pvscsi.c   |  3 ++-
>  hw/usb/hcd-xhci.c  |  5 -
>  hw/vfio/pci.c  |  3 ++-
>  include/hw/pci/msi.h   |  4 ++--
>  13 files changed, 55 insertions(+), 17 deletions(-)
> 

You'll need to squash these patches as the first patch will break git
bisect.

--js



Re: [Qemu-block] [PATCH v2 7/8] block: Move some bdrv_*_all() functions to BB

2015-12-04 Thread Max Reitz
On 01.12.2015 17:01, Kevin Wolf wrote:
> Am 10.11.2015 um 04:27 hat Max Reitz geschrieben:
>> Move bdrv_drain_all(), bdrv_commit_all(), bdrv_flush_all() and
>> bdrv_invalidate_cache_all() to BB.
>>
>> The only operation left is bdrv_close_all(), which cannot be moved to
>> the BB because it should not only close all BBs, but also all
>> monitor-owned BDSs.
>>
>> Signed-off-by: Max Reitz 
> 
> bdrv_commit_all() and bdrv_flush_all() are relatively obvious. They are
> meant to commit/flush changes made by a guest, so moving to the BB level
> seems right.

OK, I wanted to just follow this comment, but since monitor_bdrv_states
is local to blockdev.c (and I consider that correct) that would mean
either making it global (which I wouldn't like) or keeping bdrv_states
(which I wouldn't like either, not least because dropping bdrv_states
allows us to drop bdrv_new_root(), which may or may not be crucial to
the follow-up series to this one).

So, I'll be trying to defend what this patch does for now.

> I'm not so sure about bdrv_invalidate_cache_all(). Even if a BB isn't
> attached to a BDS, we still need to invalidate the caches of any images
> that have been opened before migration has completed, including those
> images that are only owned by the monitor.

I consider BB-less BDSs to be in a temporary state between blockdev-add
and binding them to a device, or between unbinding them from a device
and blockdev-del. So I don't even know if we should allow them to be
migrated.

Anyway, in my opinion, a BB-less BDS should be totally static.
Invalidating its cache shouldn't do anything. Instead, its cache should
be invalidated when it is detached from a BB (just like this patch adds
a bdrv_drain() call to blk_remove_bs()).

> Similarly I'm concerned about bdrv_drain_all(). Anything outside the
> block layer should certainly call blk_drain_all() because it can only be
> interested in those images that it can see. The calls in block.c,
> however, look like they should consider BDSes without a BB as well. (The
> monitor, which can hold direct BDS references, may be another valid user
> of a bdrv_drain_all that also covers BDSes without a BB.)

Similarly, in my opinion, bdrv_drain() shouldn't do anything on a
BB-less BDS. That is why this patch adds a bdrv_drain() call to
blk_remove_bs() (a bdrv_invalidate_cache() call is missing yet, though).

But I just remembered that block jobs don't use BBs... Did I mention
before how wrong I think that is?

Now I'm torn between trying to make block jobs use BBs once more
(because I really think BB-less BDSs should not have any active I/O) and
doing some ugly things with bdrv_states and/or bdrv_monitor_states. I'll
have to think about it.

> And block.c calling blk_*() functions smells a bit fishy anyway.

I don't find it any more fishy than single-BDS functions calling
bdrv_*_all() functions. And that is, not fishy at all. If some function
wants to drain all I/O and we define I/O to always originate from a BB,
then I find the only reasonable approach to call blk_drain_all().

Max



signature.asc
Description: OpenPGP digital signature


Re: [Qemu-block] [PATCH v3 06/21] block: Exclude nested options only for children in append_open_options()

2015-12-04 Thread Max Reitz
On 04.12.2015 14:35, Kevin Wolf wrote:
> Some drivers have nested options (e.g. blkdebug rule arrays), which
> don't belong to a child node and shouldn't be removed. Don't remove all
> options with "." in their name, but check for the complete prefixes of
> actually existing child nodes.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 20 
>  include/block/block_int.h |  1 +
>  2 files changed, 17 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature


Re: [Qemu-block] [PATCH v3 17/21] block: Move cache options into options QDict

2015-12-04 Thread Max Reitz
On 04.12.2015 14:35, Kevin Wolf wrote:
> This adds the cache mode options to the QDict, so that they can be
> specified for child nodes (e.g. backing.cache.direct=off).
> 
> The cache modes are not removed from the flags at this point; instead,
> options and flags are kept in sync. If the user specifies both flags and
> options, the options take precedence.
> 
> Child node inherit cache modes as options now, they don't use flags any
> more.
> 
> Note that this forbids specifying the cache mode for empty drives. It
> didn't make sense anyway to specify it there, because it didn't have any
> effect. blockdev_init() considers the cache options now bdrv_open()
> options and therefore doesn't create an empty drive any more but calls
> into bdrv_open(). This in turn will fail with no driver and filename
> specified.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c| 100 
> ++---
>  blockdev.c |  52 ++--
>  2 files changed, 111 insertions(+), 41 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature


Re: [Qemu-block] [PATCH v3 14/21] blockdev: Set 'format' indicates non-empty drive

2015-12-04 Thread Max Reitz
On 04.12.2015 14:35, Kevin Wolf wrote:
> Creating an empty drive while specifying 'format' doesn't make sense.
> The specified format driver would simply be ignored.
> 
> Make a set 'format' option an indication that a non-empty drive should
> be created. This makes 'format' consistent with 'driver' and allows
> using it with a block driver that doesn't need any other options (like
> null-co/null-aio).
> 
> Signed-off-by: Kevin Wolf 
> ---
>  blockdev.c| 5 +
>  tests/hd-geo-test.c   | 4 ++--
>  tests/qemu-iotests/iotests.py | 2 +-
>  3 files changed, 4 insertions(+), 7 deletions(-)

Reviewed-by: Max Reitz 



signature.asc
Description: OpenPGP digital signature


[Qemu-block] [PULL v2 3/4] tests: Use proper functions types instead of void (*fn)

2015-12-04 Thread Andreas Färber
From: Markus Armbruster 

We have several function parameters declared as void (*fn).  This is
just a stupid way to write void *, and the only purpose writing it
like that could serve is obscuring the sin of bypassing the type
system without need.

The original sin is commit 49ee359: its qtest_add_func() is a wrapper
for g_test_add_func().  Fix the parameter type to match
g_test_add_func()'s.  This uncovers type errors in ide-test.c; fix
them.

Commit 7949c0e faithfully repeated the sin for qtest_add_data_func().
Fix it the same way, along with a harmless type error uncovered in
vhost-user-test.c.

Commit 063c23d repeated it for qtest_add_abrt_handler().  The screwy
parameter gets assigned to GHook member func, so change its type to
match.  Requires wrapping kill_qemu() to keep the type checker happy.

Signed-off-by: Markus Armbruster 
Reviewed-by: Eric Blake 
[AF/armbru: Inline GTestFunc/GTestDataFunc typedef for old GLib]
Signed-off-by: Markus Armbruster 
Signed-off-by: Andreas Färber 
---
 tests/ide-test.c|  4 ++--
 tests/libqtest.c| 14 ++
 tests/libqtest.h|  7 ---
 tests/vhost-user-test.c |  3 ++-
 4 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/tests/ide-test.c b/tests/ide-test.c
index c3aacd2..b864701 100644
--- a/tests/ide-test.c
+++ b/tests/ide-test.c
@@ -593,12 +593,12 @@ static void test_flush_nodev(void)
 ide_test_quit();
 }
 
-static void test_pci_retry_flush(const char *machine)
+static void test_pci_retry_flush(void)
 {
 test_retry_flush("pc");
 }
 
-static void test_isa_retry_flush(const char *machine)
+static void test_isa_retry_flush(void)
 {
 test_retry_flush("isapc");
 }
diff --git a/tests/libqtest.c b/tests/libqtest.c
index 9753161..fa314e1 100644
--- a/tests/libqtest.c
+++ b/tests/libqtest.c
@@ -110,6 +110,11 @@ static void kill_qemu(QTestState *s)
 }
 }
 
+static void kill_qemu_hook_func(void *s)
+{
+kill_qemu(s);
+}
+
 static void sigabrt_handler(int signo)
 {
 g_hook_list_invoke(_hooks, FALSE);
@@ -133,7 +138,7 @@ static void cleanup_sigabrt_handler(void)
 sigaction(SIGABRT, _old, NULL);
 }
 
-void qtest_add_abrt_handler(void (*fn), const void *data)
+void qtest_add_abrt_handler(GHookFunc fn, const void *data)
 {
 GHook *hook;
 
@@ -170,7 +175,7 @@ QTestState *qtest_init(const char *extra_args)
 sock = init_socket(socket_path);
 qmpsock = init_socket(qmp_socket_path);
 
-qtest_add_abrt_handler(kill_qemu, s);
+qtest_add_abrt_handler(kill_qemu_hook_func, s);
 
 s->qemu_pid = fork();
 if (s->qemu_pid == 0) {
@@ -755,14 +760,15 @@ void qtest_memread(QTestState *s, uint64_t addr, void 
*data, size_t size)
 g_strfreev(args);
 }
 
-void qtest_add_func(const char *str, void (*fn))
+void qtest_add_func(const char *str, void (*fn)(void))
 {
 gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
 g_test_add_func(path, fn);
 g_free(path);
 }
 
-void qtest_add_data_func(const char *str, const void *data, void (*fn))
+void qtest_add_data_func(const char *str, const void *data,
+ void (*fn)(const void *))
 {
 gchar *path = g_strdup_printf("/%s/%s", qtest_get_arch(), str);
 g_test_add_data_func(path, data, fn);
diff --git a/tests/libqtest.h b/tests/libqtest.h
index df08745..ebdd5bb 100644
--- a/tests/libqtest.h
+++ b/tests/libqtest.h
@@ -416,7 +416,7 @@ const char *qtest_get_arch(void);
  * The path is prefixed with the architecture under test, as
  * returned by qtest_get_arch().
  */
-void qtest_add_func(const char *str, void (*fn));
+void qtest_add_func(const char *str, void (*fn)(void));
 
 /**
  * qtest_add_data_func:
@@ -428,7 +428,8 @@ void qtest_add_func(const char *str, void (*fn));
  * The path is prefixed with the architecture under test, as
  * returned by qtest_get_arch().
  */
-void qtest_add_data_func(const char *str, const void *data, void (*fn));
+void qtest_add_data_func(const char *str, const void *data,
+ void (*fn)(const void *));
 
 /**
  * qtest_add:
@@ -450,7 +451,7 @@ void qtest_add_data_func(const char *str, const void *data, 
void (*fn));
 g_free(path); \
 } while (0)
 
-void qtest_add_abrt_handler(void (*fn), const void *data);
+void qtest_add_abrt_handler(GHookFunc fn, const void *data);
 
 /**
  * qtest_start:
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index 29de739..991fd85 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -173,8 +173,9 @@ static void wait_for_fds(TestServer *s)
 g_mutex_unlock(>data_mutex);
 }
 
-static void read_guest_mem(TestServer *s)
+static void read_guest_mem(const void *data)
 {
+TestServer *s = (void *)data;
 uint32_t *guest_mem;
 int i, j;
 size_t size;
-- 
2.6.2