[Qemu-block] [PATCH v2 1/2] qcow2: Make size_to_clusters() return uint64_t

[Max Reitz]

Sadly, some images may have more clusters than what can be represented
using a plain int. We should be prepared for that case (in
qcow2_check_refcounts() we actually were trying to catch that case, but
since size_to_clusters() truncated the returned value, that check never
did anything useful).

Cc: qemu-stable <qemu-sta...@nongnu.org>
Signed-off-by: Max Reitz <mre...@redhat.com>
---
 block/qcow2-cluster.c  | 30 +++---
 block/qcow2-refcount.c | 10 +++---
 block/qcow2.h  |  6 +++---
 3 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 2975b83..dc217e7 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -298,7 +298,7 @@ fail:
  * as contiguous. (This allows it, for example, to stop at the first compressed
  * cluster which may require a different handling)
  */
-static int count_contiguous_clusters(uint64_t nb_clusters, int cluster_size,
+static int count_contiguous_clusters(int nb_clusters, int cluster_size,
 uint64_t *l2_table, uint64_t stop_flags)
 {
 int i;
@@ -321,7 +321,7 @@ static int count_contiguous_clusters(uint64_t nb_clusters, 
int cluster_size,
return i;
 }
 
-static int count_contiguous_free_clusters(uint64_t nb_clusters, uint64_t 
*l2_table)
+static int count_contiguous_free_clusters(int nb_clusters, uint64_t *l2_table)
 {
 int i;
 
@@ -495,6 +495,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t 
offset,
 if (nb_needed > nb_available) {
 nb_needed = nb_available;
 }
+assert(nb_needed <= INT_MAX);
 
 *cluster_offset = 0;
 
@@ -530,6 +531,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t 
offset,
 
 l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1);
 *cluster_offset = be64_to_cpu(l2_table[l2_index]);
+
+/* nb_needed <= INT_MAX, thus nb_clusters <= INT_MAX, too */
 nb_clusters = size_to_clusters(s, nb_needed << 9);
 
 ret = qcow2_get_cluster_type(*cluster_offset);
@@ -840,7 +843,7 @@ err:
 static int count_cow_clusters(BDRVQcow2State *s, int nb_clusters,
 uint64_t *l2_table, int l2_index)
 {
-int i;
+uint64_t i;
 
 for (i = 0; i < nb_clusters; i++) {
 uint64_t l2_entry = be64_to_cpu(l2_table[l2_index + i]);
@@ -960,7 +963,7 @@ static int handle_copied(BlockDriverState *bs, uint64_t 
guest_offset,
 int l2_index;
 uint64_t cluster_offset;
 uint64_t *l2_table;
-unsigned int nb_clusters;
+uint64_t nb_clusters;
 unsigned int keep_clusters;
 int ret;
 
@@ -979,6 +982,7 @@ static int handle_copied(BlockDriverState *bs, uint64_t 
guest_offset,
 
 l2_index = offset_to_l2_index(s, guest_offset);
 nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
+assert(nb_clusters <= INT_MAX);
 
 /* Find L2 entry for the first involved cluster */
 ret = get_cluster_table(bs, guest_offset, _table, _index);
@@ -1061,7 +1065,7 @@ out:
  * restarted, but the whole request should not be failed.
  */
 static int do_alloc_cluster_offset(BlockDriverState *bs, uint64_t guest_offset,
-uint64_t *host_offset, unsigned int *nb_clusters)
+   uint64_t *host_offset, uint64_t 
*nb_clusters)
 {
 BDRVQcow2State *s = bs->opaque;
 
@@ -1079,7 +1083,7 @@ static int do_alloc_cluster_offset(BlockDriverState *bs, 
uint64_t guest_offset,
 *host_offset = cluster_offset;
 return 0;
 } else {
-int ret = qcow2_alloc_clusters_at(bs, *host_offset, *nb_clusters);
+int64_t ret = qcow2_alloc_clusters_at(bs, *host_offset, *nb_clusters);
 if (ret < 0) {
 return ret;
 }
@@ -1115,7 +1119,7 @@ static int handle_alloc(BlockDriverState *bs, uint64_t 
guest_offset,
 int l2_index;
 uint64_t *l2_table;
 uint64_t entry;
-unsigned int nb_clusters;
+uint64_t nb_clusters;
 int ret;
 
 uint64_t alloc_cluster_offset;
@@ -1133,6 +1137,7 @@ static int handle_alloc(BlockDriverState *bs, uint64_t 
guest_offset,
 
 l2_index = offset_to_l2_index(s, guest_offset);
 nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
+assert(nb_clusters <= INT_MAX);
 
 /* Find L2 entry for the first involved cluster */
 ret = get_cluster_table(bs, guest_offset, _table, _index);
@@ -1426,7 +1431,8 @@ int qcow2_decompress_cluster(BlockDriverState *bs, 
uint64_t cluster_offset)
  * clusters.
  */
 static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
-unsigned int nb_clusters, enum qcow2_discard_type type, bool full_discard)
+ uint64_t nb_clusters, enum qcow2_discard_type 
type,
+ bool full_discard)
 {
 BDRVQcow2State *s = bs->opaque;
 uint64_t *l2_table;
@@ -1441,6 +1447,7 @@ static int discard_single_l2(BlockDriverState *bs, 
uint64_t offset,
 
 /* Limit nb_clusters to one L2 table */
 nb_clust

[Qemu-block] [PATCH v3 1/2] qcow2: Make size_to_clusters() return uint64_t

[Max Reitz]

Sadly, some images may have more clusters than what can be represented
using a plain int. We should be prepared for that case (in
qcow2_check_refcounts() we actually were trying to catch that case, but
since size_to_clusters() truncated the returned value, that check never
did anything useful).

Cc: qemu-stable <qemu-sta...@nongnu.org>
Signed-off-by: Max Reitz <mre...@redhat.com>
---
 block/qcow2-cluster.c  | 30 +++---
 block/qcow2-refcount.c | 12 
 block/qcow2.h  |  6 +++---
 3 files changed, 30 insertions(+), 18 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 2975b83..dc217e7 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -298,7 +298,7 @@ fail:
  * as contiguous. (This allows it, for example, to stop at the first compressed
  * cluster which may require a different handling)
  */
-static int count_contiguous_clusters(uint64_t nb_clusters, int cluster_size,
+static int count_contiguous_clusters(int nb_clusters, int cluster_size,
 uint64_t *l2_table, uint64_t stop_flags)
 {
 int i;
@@ -321,7 +321,7 @@ static int count_contiguous_clusters(uint64_t nb_clusters, 
int cluster_size,
return i;
 }
 
-static int count_contiguous_free_clusters(uint64_t nb_clusters, uint64_t 
*l2_table)
+static int count_contiguous_free_clusters(int nb_clusters, uint64_t *l2_table)
 {
 int i;
 
@@ -495,6 +495,7 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t 
offset,
 if (nb_needed > nb_available) {
 nb_needed = nb_available;
 }
+assert(nb_needed <= INT_MAX);
 
 *cluster_offset = 0;
 
@@ -530,6 +531,8 @@ int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t 
offset,
 
 l2_index = (offset >> s->cluster_bits) & (s->l2_size - 1);
 *cluster_offset = be64_to_cpu(l2_table[l2_index]);
+
+/* nb_needed <= INT_MAX, thus nb_clusters <= INT_MAX, too */
 nb_clusters = size_to_clusters(s, nb_needed << 9);
 
 ret = qcow2_get_cluster_type(*cluster_offset);
@@ -840,7 +843,7 @@ err:
 static int count_cow_clusters(BDRVQcow2State *s, int nb_clusters,
 uint64_t *l2_table, int l2_index)
 {
-int i;
+uint64_t i;
 
 for (i = 0; i < nb_clusters; i++) {
 uint64_t l2_entry = be64_to_cpu(l2_table[l2_index + i]);
@@ -960,7 +963,7 @@ static int handle_copied(BlockDriverState *bs, uint64_t 
guest_offset,
 int l2_index;
 uint64_t cluster_offset;
 uint64_t *l2_table;
-unsigned int nb_clusters;
+uint64_t nb_clusters;
 unsigned int keep_clusters;
 int ret;
 
@@ -979,6 +982,7 @@ static int handle_copied(BlockDriverState *bs, uint64_t 
guest_offset,
 
 l2_index = offset_to_l2_index(s, guest_offset);
 nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
+assert(nb_clusters <= INT_MAX);
 
 /* Find L2 entry for the first involved cluster */
 ret = get_cluster_table(bs, guest_offset, _table, _index);
@@ -1061,7 +1065,7 @@ out:
  * restarted, but the whole request should not be failed.
  */
 static int do_alloc_cluster_offset(BlockDriverState *bs, uint64_t guest_offset,
-uint64_t *host_offset, unsigned int *nb_clusters)
+   uint64_t *host_offset, uint64_t 
*nb_clusters)
 {
 BDRVQcow2State *s = bs->opaque;
 
@@ -1079,7 +1083,7 @@ static int do_alloc_cluster_offset(BlockDriverState *bs, 
uint64_t guest_offset,
 *host_offset = cluster_offset;
 return 0;
 } else {
-int ret = qcow2_alloc_clusters_at(bs, *host_offset, *nb_clusters);
+int64_t ret = qcow2_alloc_clusters_at(bs, *host_offset, *nb_clusters);
 if (ret < 0) {
 return ret;
 }
@@ -1115,7 +1119,7 @@ static int handle_alloc(BlockDriverState *bs, uint64_t 
guest_offset,
 int l2_index;
 uint64_t *l2_table;
 uint64_t entry;
-unsigned int nb_clusters;
+uint64_t nb_clusters;
 int ret;
 
 uint64_t alloc_cluster_offset;
@@ -1133,6 +1137,7 @@ static int handle_alloc(BlockDriverState *bs, uint64_t 
guest_offset,
 
 l2_index = offset_to_l2_index(s, guest_offset);
 nb_clusters = MIN(nb_clusters, s->l2_size - l2_index);
+assert(nb_clusters <= INT_MAX);
 
 /* Find L2 entry for the first involved cluster */
 ret = get_cluster_table(bs, guest_offset, _table, _index);
@@ -1426,7 +1431,8 @@ int qcow2_decompress_cluster(BlockDriverState *bs, 
uint64_t cluster_offset)
  * clusters.
  */
 static int discard_single_l2(BlockDriverState *bs, uint64_t offset,
-unsigned int nb_clusters, enum qcow2_discard_type type, bool full_discard)
+ uint64_t nb_clusters, enum qcow2_discard_type 
type,
+ bool full_discard)
 {
 BDRVQcow2State *s = bs->opaque;
 uint64_t *l2_table;
@@ -1441,6 +1447,7 @@ static int discard_single_l2(BlockDriverState *bs, 
uint64_t offset,
 
 /* Limit nb_clusters to one L2 table */
 

[Qemu-block] [PATCH v3 0/2] qcow2: Make size_to_clusters() return uint64_t

[Max Reitz]

Some callers actually expected that function to return uint64_t. As it
turns out, it doesn't. Fix that.


v3:
- Patch 1: Fix build for sizeof(void *) < 8 machines [Peter]


git-backport-diff against v2:

Key:
[] : patches are identical
[] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/2:[0002] [FC] 'qcow2: Make size_to_clusters() return uint64_t'
002/2:[] [--] 'iotests: Add test for checking large image files'


Max Reitz (2):
  qcow2: Make size_to_clusters() return uint64_t
  iotests: Add test for checking large image files

 block/qcow2-cluster.c  | 30 ---
 block/qcow2-refcount.c | 12 +---
 block/qcow2.h  |  6 ++--
 tests/qemu-iotests/138 | 73 ++
 tests/qemu-iotests/138.out |  9 ++
 tests/qemu-iotests/group   |  1 +
 6 files changed, 113 insertions(+), 18 deletions(-)
 create mode 100755 tests/qemu-iotests/138
 create mode 100644 tests/qemu-iotests/138.out

-- 
2.5.2

[Qemu-block] [PATCH v3 2/2] iotests: Add test for checking large image files

[Max Reitz]

Add a test for checking a qcow2 file with a multiple of 2^32 clusters.

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
 tests/qemu-iotests/138 | 73 ++
 tests/qemu-iotests/138.out |  9 ++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 83 insertions(+)
 create mode 100755 tests/qemu-iotests/138
 create mode 100644 tests/qemu-iotests/138.out

diff --git a/tests/qemu-iotests/138 b/tests/qemu-iotests/138
new file mode 100755
index 000..a5c3464
--- /dev/null
+++ b/tests/qemu-iotests/138
@@ -0,0 +1,73 @@
+#!/bin/bash
+#
+# General test case for qcow2's image check
+#
+# 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=mre...@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
+
+# This tests qocw2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+echo
+echo '=== Check on an image with a multiple of 2^32 clusters ==='
+echo
+
+IMGOPTS=$(_optstr_add "$IMGOPTS" "cluster_size=512") \
+_make_test_img 512
+
+# Allocate L2 table
+$QEMU_IO -c 'write 0 512' "$TEST_IMG" | _filter_qemu_io
+
+# Put the data cluster at a multiple of 2 TB, resulting in the image apparently
+# having a multiple of 2^32 clusters
+# (To be more specific: It is at 32 PB)
+poke_file "$TEST_IMG" 2048 "\x80\x80\x00\x00\x00\x00\x00\x00"
+
+# An offset of 32 PB results in qemu-img check having to allocate an in-memory
+# refcount table of 128 TB (16 bit refcounts, 512 byte clusters).
+# This should be generally too much for any system and thus fail.
+# What this test is checking is that the qcow2 driver actually tries to 
allocate
+# such a large amount of memory (and is consequently aborting) instead of 
having
+# truncated the cluster count somewhere (which would result in much less memory
+# being allocated and then a segfault occurring).
+_check_test_img
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/138.out b/tests/qemu-iotests/138.out
new file mode 100644
index 000..3fe911f
--- /dev/null
+++ b/tests/qemu-iotests/138.out
@@ -0,0 +1,9 @@
+QA output created by 138
+
+=== Check on an image with a multiple of 2^32 clusters ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=512
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+qemu-img: Check failed: Cannot allocate memory
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 3a6a8f0..439b1d2 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -135,3 +135,4 @@
 134 rw auto quick
 135 rw auto
 137 rw auto
+138 rw auto quick
-- 
2.5.2

Re: [Qemu-block] [PATCH v3 1/4] block: rename BlockdevSnapshot to BlockdevSnapshotSync

[Max Reitz]

On 10.09.2015 15:39, Alberto Garcia wrote:
> We will introduce the 'blockdev-snapshot' command that will require
> its own struct for the parameters, so we need to rename this one in
> order to avoid name clashes.
> 
> Signed-off-by: Alberto Garcia <be...@igalia.com>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
> ---
>  blockdev.c   | 2 +-
>  qapi-schema.json | 2 +-
>  qapi/block-core.json | 8 
>  3 files changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v4 29/38] blockdev: Add blockdev-insert-medium

[Max Reitz]

On 11.09.2015 09:30, Wen Congyang wrote:
> On 09/11/2015 03:09 AM, Max Reitz wrote:
>> On 10.09.2015 03:12, Wen Congyang wrote:
>>> On 09/09/2015 08:59 PM, Max Reitz wrote:
>>>> On 09.09.2015 12:01, Wen Congyang wrote:
>>>>> On 09/09/2015 05:20 AM, Max Reitz wrote:
>>>>>> On 08.09.2015 11:13, Wen Congyang wrote:
>>>>>>> On 07/21/2015 01:45 AM, Max Reitz wrote:
>>>>>>>> And a helper function for that, which directly takes a pointer to the
>>>>>>>> BDS to be inserted instead of its node-name (which will be used for
>>>>>>>> implementing 'change' using blockdev-insert-medium).
>>>>>>>>
>>>>>>>> Signed-off-by: Max Reitz <mre...@redhat.com>
>>>>>>>> ---
>>>>>>>>  blockdev.c   | 48 
>>>>>>>> 
>>>>>>>>  qapi/block-core.json | 17 +
>>>>>>>>  qmp-commands.hx  | 37 +
>>>>>>>>  3 files changed, 102 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>>>>> index 481760a..a80d0e2 100644
>>>>>>>> --- a/blockdev.c
>>>>>>>> +++ b/blockdev.c
>>>>>>>> @@ -2164,6 +2164,54 @@ void qmp_blockdev_remove_medium(const char 
>>>>>>>> *device, Error **errp)
>>>>>>>>  }
>>>>>>>>  }
>>>>>>>>  
>>>>>>>> +static void qmp_blockdev_insert_anon_medium(const char *device,
>>>>>>>> +BlockDriverState *bs, 
>>>>>>>> Error **errp)
>>>>>>>> +{
>>>>>>>> +BlockBackend *blk;
>>>>>>>> +bool has_device;
>>>>>>>> +
>>>>>>>> +blk = blk_by_name(device);
>>>>>>>> +if (!blk) {
>>>>>>>> +error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>>>>>>>> +  "Device '%s' not found", device);
>>>>>>>> +return;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/* For BBs without a device, we can exchange the BDS tree at will 
>>>>>>>> */
>>>>>>>> +has_device = blk_get_attached_dev(blk);
>>>>>>>> +
>>>>>>>> +if (has_device && !blk_dev_has_removable_media(blk)) {
>>>>>>>> +error_setg(errp, "Device '%s' is not removable", device);
>>>>>>>> +return;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +if (has_device && !blk_dev_is_tray_open(blk)) {
>>>>>>>> +error_setg(errp, "Tray of device '%s' is not open", device);
>>>>>>>> +return;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +if (blk_bs(blk)) {
>>>>>>>> +error_setg(errp, "There already is a medium in device '%s'", 
>>>>>>>> device);
>>>>>>>> +return;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +blk_insert_bs(blk, bs);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +void qmp_blockdev_insert_medium(const char *device, const char 
>>>>>>>> *node_name,
>>>>>>>> +Error **errp)
>>>>>>>> +{
>>>>>>>> +BlockDriverState *bs;
>>>>>>>> +
>>>>>>>> +bs = bdrv_find_node(node_name);
>>>>>>>> +if (!bs) {
>>>>>>>> +error_setg(errp, "Node '%s' not found", node_name);
>>>>>>>> +return;
>>>>>>>> +}
>>>>>>>
>>>>>>> Hmm, it is OK if the bs is not top BDS?
>>>>>>
>>>>>> I think so, yes. Generally, there's probably no reason to do that, but I
>>>>>> don't know why we shou

Re: [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

[Max Reitz]

On 10.09.2015 15:39, Alberto Garcia wrote:
> If set to true, the image will be opened with the BDRV_O_NO_BACKING
> flag. This is useful for creating snapshots using images opened with
> blockdev-add, since they are not supposed to have a backing image
> before the operation.
> 
> Signed-off-by: Alberto Garcia 
> ---
>  block.c  | 5 +
>  qapi/block-core.json | 6 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)

Ignorant of any possible previous discussion that might have taken
place: The documentation for @backing says it may be set to the empty
string in order to achieve exactly that.

So why do we need the new flag? Because "backing: ''" is ugly?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 2/4] block: Add 'ignore-backing' field to BlockdevOptionsGenericCOWFormat

[Max Reitz]

On 11.09.2015 19:28, Kevin Wolf wrote:
> Am 11.09.2015 um 19:21 hat Max Reitz geschrieben:
>> On 10.09.2015 15:39, Alberto Garcia wrote:
>>> If set to true, the image will be opened with the BDRV_O_NO_BACKING
>>> flag. This is useful for creating snapshots using images opened with
>>> blockdev-add, since they are not supposed to have a backing image
>>> before the operation.
>>>
>>> Signed-off-by: Alberto Garcia <be...@igalia.com>
>>> ---
>>>  block.c  | 5 +
>>>  qapi/block-core.json | 6 +-
>>>  2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> Ignorant of any possible previous discussion that might have taken
>> place: The documentation for @backing says it may be set to the empty
>> string in order to achieve exactly that.
>>
>> So why do we need the new flag? Because "backing: ''" is ugly?
> 
> I guess it's just because you're the only one who actually reads the
> documentation. When discussing this, I didn't remember that we already
> had a way to express this (an additional bool wouldn't have been my
> favourite solution anyway). Thanks for catching this.

I read the patch, it was part of the context. ;-)

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v3 3/4] block: add a 'blockdev-snapshot' QMP command

[Max Reitz]

On 10.09.2015 15:39, Alberto Garcia wrote:
> One of the limitations of the 'blockdev-snapshot-sync' command is that
> it does not allow passing BlockdevOptions to the newly created
> snapshots, so they are always opened using the default values.
> 
> Extending the command to allow passing options is not a practical
> solution because there is overlap between those options and some of
> the existing parameters of the command.
> 
> This patch introduces a new 'blockdev-snapshot' command with a simpler
> interface: it just takes two references to existing block devices that
> will be used as the source and target for the snapshot.
> 
> Since the main difference between the two commands is that one of them
> creates and opens the target image, while the other uses an already
> opened one, the bulk of the implementation is shared.
> 
> Signed-off-by: Alberto Garcia <be...@igalia.com>
> ---
>  blockdev.c   | 163 
> ---
>  qapi-schema.json |   2 +
>  qapi/block-core.json |  26 
>  qmp-commands.hx  |  29 +++++
>  4 files changed, 160 insertions(+), 60 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH v4 29/38] blockdev: Add blockdev-insert-medium

[Max Reitz]

On 10.09.2015 03:12, Wen Congyang wrote:
> On 09/09/2015 08:59 PM, Max Reitz wrote:
>> On 09.09.2015 12:01, Wen Congyang wrote:
>>> On 09/09/2015 05:20 AM, Max Reitz wrote:
>>>> On 08.09.2015 11:13, Wen Congyang wrote:
>>>>> On 07/21/2015 01:45 AM, Max Reitz wrote:
>>>>>> And a helper function for that, which directly takes a pointer to the
>>>>>> BDS to be inserted instead of its node-name (which will be used for
>>>>>> implementing 'change' using blockdev-insert-medium).
>>>>>>
>>>>>> Signed-off-by: Max Reitz <mre...@redhat.com>
>>>>>> ---
>>>>>>  blockdev.c   | 48 
>>>>>> 
>>>>>>  qapi/block-core.json | 17 +
>>>>>>  qmp-commands.hx  | 37 +
>>>>>>  3 files changed, 102 insertions(+)
>>>>>>
>>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>>> index 481760a..a80d0e2 100644
>>>>>> --- a/blockdev.c
>>>>>> +++ b/blockdev.c
>>>>>> @@ -2164,6 +2164,54 @@ void qmp_blockdev_remove_medium(const char 
>>>>>> *device, Error **errp)
>>>>>>  }
>>>>>>  }
>>>>>>  
>>>>>> +static void qmp_blockdev_insert_anon_medium(const char *device,
>>>>>> +BlockDriverState *bs, Error 
>>>>>> **errp)
>>>>>> +{
>>>>>> +BlockBackend *blk;
>>>>>> +bool has_device;
>>>>>> +
>>>>>> +blk = blk_by_name(device);
>>>>>> +if (!blk) {
>>>>>> +error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>>>>>> +  "Device '%s' not found", device);
>>>>>> +return;
>>>>>> +}
>>>>>> +
>>>>>> +/* For BBs without a device, we can exchange the BDS tree at will */
>>>>>> +has_device = blk_get_attached_dev(blk);
>>>>>> +
>>>>>> +if (has_device && !blk_dev_has_removable_media(blk)) {
>>>>>> +error_setg(errp, "Device '%s' is not removable", device);
>>>>>> +return;
>>>>>> +}
>>>>>> +
>>>>>> +if (has_device && !blk_dev_is_tray_open(blk)) {
>>>>>> +error_setg(errp, "Tray of device '%s' is not open", device);
>>>>>> +return;
>>>>>> +}
>>>>>> +
>>>>>> +if (blk_bs(blk)) {
>>>>>> +error_setg(errp, "There already is a medium in device '%s'", 
>>>>>> device);
>>>>>> +return;
>>>>>> +}
>>>>>> +
>>>>>> +blk_insert_bs(blk, bs);
>>>>>> +}
>>>>>> +
>>>>>> +void qmp_blockdev_insert_medium(const char *device, const char 
>>>>>> *node_name,
>>>>>> +Error **errp)
>>>>>> +{
>>>>>> +BlockDriverState *bs;
>>>>>> +
>>>>>> +bs = bdrv_find_node(node_name);
>>>>>> +if (!bs) {
>>>>>> +error_setg(errp, "Node '%s' not found", node_name);
>>>>>> +return;
>>>>>> +}
>>>>>
>>>>> Hmm, it is OK if the bs is not top BDS?
>>>>
>>>> I think so, yes. Generally, there's probably no reason to do that, but I
>>>> don't know why we should not allow that case. For instance, you might
>>>> want to make a backing file available read-only somewhere.
>>>>
>>>> It should be impossible to make it available writable, and it should not
>>>> be allowed to start a block-commit operation while the backing file can
>>>> be accessed by the guest, but this should be achieved using op blockers.
>>>>
>>>> What we need for this to work are fine-grained op blockers, I think. But
>>>> working around that for now by only allowing to insert top BDS won't
>>>> work, since you can still start block jobs which target top BDS, too
>>>> (e.g. blockdev-backup can write to a BDS/BB t

Re: [Qemu-block] [Qemu-devel] [PATCH v4 29/38] blockdev: Add blockdev-insert-medium

[Max Reitz]

On 10.09.2015 05:22, Wen Congyang wrote:
> On 09/09/2015 08:59 PM, Max Reitz wrote:
>> On 09.09.2015 12:01, Wen Congyang wrote:
>>> On 09/09/2015 05:20 AM, Max Reitz wrote:
>>>> On 08.09.2015 11:13, Wen Congyang wrote:
>>>>> On 07/21/2015 01:45 AM, Max Reitz wrote:
>>>>>> And a helper function for that, which directly takes a pointer to the
>>>>>> BDS to be inserted instead of its node-name (which will be used for
>>>>>> implementing 'change' using blockdev-insert-medium).
>>>>>>
>>>>>> Signed-off-by: Max Reitz <mre...@redhat.com>
>>>>>> ---
>>>>>>  blockdev.c   | 48 
>>>>>> 
>>>>>>  qapi/block-core.json | 17 +
>>>>>>  qmp-commands.hx  | 37 +
>>>>>>  3 files changed, 102 insertions(+)
>>>>>>
>>>>>> diff --git a/blockdev.c b/blockdev.c
>>>>>> index 481760a..a80d0e2 100644
>>>>>> --- a/blockdev.c
>>>>>> +++ b/blockdev.c
>>>>>> @@ -2164,6 +2164,54 @@ void qmp_blockdev_remove_medium(const char 
>>>>>> *device, Error **errp)
>>>>>>  }
>>>>>>  }
>>>>>>  
>>>>>> +static void qmp_blockdev_insert_anon_medium(const char *device,
>>>>>> +BlockDriverState *bs, Error 
>>>>>> **errp)
>>>>>> +{
>>>>>> +BlockBackend *blk;
>>>>>> +bool has_device;
>>>>>> +
>>>>>> +blk = blk_by_name(device);
>>>>>> +if (!blk) {
>>>>>> +error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
>>>>>> +  "Device '%s' not found", device);
>>>>>> +return;
>>>>>> +}
>>>>>> +
>>>>>> +/* For BBs without a device, we can exchange the BDS tree at will */
>>>>>> +has_device = blk_get_attached_dev(blk);
>>>>>> +
>>>>>> +if (has_device && !blk_dev_has_removable_media(blk)) {
>>>>>> +error_setg(errp, "Device '%s' is not removable", device);
>>>>>> +return;
>>>>>> +}
>>>>>> +
>>>>>> +if (has_device && !blk_dev_is_tray_open(blk)) {
>>>>>> +error_setg(errp, "Tray of device '%s' is not open", device);
>>>>>> +return;
>>>>>> +}
>>>>>> +
>>>>>> +if (blk_bs(blk)) {
>>>>>> +error_setg(errp, "There already is a medium in device '%s'", 
>>>>>> device);
>>>>>> +return;
>>>>>> +}
>>>>>> +
>>>>>> +blk_insert_bs(blk, bs);
>>>>>> +}
>>>>>> +
>>>>>> +void qmp_blockdev_insert_medium(const char *device, const char 
>>>>>> *node_name,
>>>>>> +Error **errp)
>>>>>> +{
>>>>>> +BlockDriverState *bs;
>>>>>> +
>>>>>> +bs = bdrv_find_node(node_name);
>>>>>> +if (!bs) {
>>>>>> +error_setg(errp, "Node '%s' not found", node_name);
>>>>>> +return;
>>>>>> +}
>>>>>
>>>>> Hmm, it is OK if the bs is not top BDS?
>>>>
>>>> I think so, yes. Generally, there's probably no reason to do that, but I
>>>> don't know why we should not allow that case. For instance, you might
>>>> want to make a backing file available read-only somewhere.
>>>>
>>>> It should be impossible to make it available writable, and it should not
>>>> be allowed to start a block-commit operation while the backing file can
>>>> be accessed by the guest, but this should be achieved using op blockers.
>>>>
>>>> What we need for this to work are fine-grained op blockers, I think. But
>>>> working around that for now by only allowing to insert top BDS won't
>>>> work, since you can still start block jobs which target top BDS, too
>>>> (e.g. blockdev-backup can write to a BDS/BB that is visible to the guest).
>>>>
>>>> All in all, I think it's fine to insert non-top BDS, but we should
>>>> definitely worry about which exact BDS one can insert once we have
>>>> fine-grained op blockers.
>>>
>>> A BDS can be written by its parent, its block backend, a block job..
>>> So I think we should have some way to avoid more than two sources writing
>>> to it, otherwise the data may be corrupted.
>>
>> Yes, and that would be op blockers.
>>
>> As I said, using blockdev-backup you can write to a BB that can be
>> written to by the guest as well. I think this is a bug, but it is a bug
>> that needs to be fixed by having better op blockers in place, which Jeff
>> Cody is working on.
> 
> I don't find such patches in the maillist.

That's because Jeff is still working on designing and writing them.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2] iotests: disable core dumps in test 061

[Max Reitz]

On 28.09.2015 16:23, Alberto Garcia wrote:
> Commit 934659c460 disabled the supression of segmentation faults in
> bash tests. The new output of test 061, however, assumes that a core
> dump will be produced if a program aborts. This is not necessarily the
> case because core dumps can be disabled using ulimit.
> 
> Since we cannot guarantee that abort() will produce a core dump, we
> should use SIGKILL instead (that does not produce any) and update the
> test output accordingly.
> 
> Signed-off-by: Alberto Garcia <be...@igalia.com>
> ---
>  tests/qemu-iotests/061 | 8 
>  tests/qemu-iotests/061.out | 4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 09/16] block: Split bdrv_move_feature_fields()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> After bdrv_swap(), some fields must be moved back to their original BDS
> to compensate for the effects that a swap of the contents of the objects
> has while keeping the old addresses. Other fields must be moved back
> because they should logically be moved and must stay on top
> 
> When replacing bdrv_swap() with operations changing the pointers in the
> parents, we only need the latter and must avoid swapping the former.
> Split the function accordingly.
> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block.c | 12 
>  1 file changed, 12 insertions(+)

Reviewed-by: Max Reitz <mre...@redhat.com>

(and an ACK to "Op blockers are a mess.")



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 15/16] block: Add and use bdrv_replace_in_backing_chain()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> This cleans up the mess we left behind in the mirror code after the
> previous patch. Instead of using bdrv_swap(), just change pointers.
> 
> The interface change of the mirror job that callers must consider is
> that after job completion, their local BDS pointers still point to the
> same node now. qemu-img must change its code accordingly (which makes it
> easier to understand); the other callers stays unchanged because after
> completion they don't do anything with the BDS, but just with the job,
> and the job is still owned by the source BDS.
> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block.c   | 32 +++-
>  block/mirror.c| 23 +++
>  include/block/block.h |  4 +++-
>  qemu-img.c| 16 
>  4 files changed, 49 insertions(+), 26 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 13/16] block: Implement bdrv_append() without bdrv_swap()

[Max Reitz]

On 29.09.2015 15:51, Kevin Wolf wrote:
> Am 23.09.2015 um 18:36 hat Max Reitz geschrieben:
>> On 17.09.2015 15:48, Kevin Wolf wrote:
>>> Remember all parent nodes and just change the pointers there instead of
>>> swapping the contents of the BlockDriverState.
>>>
>>> Handling of snapshot=on must be moved further down in bdrv_open()
>>> because *pbs (which is the bs pointer in the BlockBackend) must already
>>> be set before bdrv_append() is called. Otherwise bdrv_append() changes
>>> the BB's pointer to the temporary snapshot, but bdrv_open() overwrites
>>> it with the read-only original image.
>>>
>>> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> 
>>> @@ -2155,6 +2157,42 @@ void bdrv_swap(BlockDriverState *bs_new, 
>>> BlockDriverState *bs_old)
>>>  bdrv_rebind(bs_old);
>>>  }
>>>  
>>> +static void change_parent_backing_link(BlockDriverState *from,
>>> +   BlockDriverState *to)
>>> +{
>>> +BdrvChild *c, *next;
>>> +
>>> +QLIST_FOREACH_SAFE(c, >parents, next_parent, next) {
>>> +assert(c->role != _backing);
>>> +c->bs = to;
>>> +QLIST_REMOVE(c, next_parent);
>>> +QLIST_INSERT_HEAD(>parents, c, next_parent);
>>
>> This drops a reference from the parent BDS to @from, and adds a new one
>> from the parent BDS to @to. However, this is not reflected here.
> 
> You mean bdrv_ref(to); bdrv_unref(from); ?

Yes.

>>> +}
>>> +if (from->blk) {
>>> +blk_set_bs(from->blk, to);
>>> +if (!to->device_list.tqe_prev) {
>>> +QTAILQ_INSERT_BEFORE(from, to, device_list);
>>> +}
>>> +QTAILQ_REMOVE(_states, from, device_list);
>>> +}
>>> +}
>>> +
>>> +static void swap_feature_fields(BlockDriverState *bs_top,
>>> +BlockDriverState *bs_new)
>>> +{
>>> +BlockDriverState tmp;
>>> +
>>> +bdrv_move_feature_fields(, bs_top);
>>> +bdrv_move_feature_fields(bs_top, bs_new);
>>> +bdrv_move_feature_fields(bs_new, );
>>> +
>>> +assert(!bs_new->io_limits_enabled);
>>> +if (bs_top->io_limits_enabled) {
>>> +bdrv_io_limits_enable(bs_new, throttle_group_get_name(bs_top));
>>> +bdrv_io_limits_disable(bs_top);
>>> +}
>>> +}
>>> +
>>>  /*
>>>   * Add new bs contents at the top of an image chain while the chain is
>>>   * live, while keeping required fields on the top layer.
>>> @@ -2165,14 +2203,29 @@ void bdrv_swap(BlockDriverState *bs_new, 
>>> BlockDriverState *bs_old)
>>>   * bs_new must not be attached to a BlockBackend.
>>>   *
>>>   * This function does not create any image files.
>>> + *
>>> + * bdrv_append() takes ownership of a bs_new reference and unrefs it 
>>> because
>>> + * that's what the callers commonly need. bs_new will be referenced by the 
>>> old
>>> + * parents of bs_top after bdrv_append() returns. If the caller needs to 
>>> keep a
>>> + * reference of its own, it must call bdrv_ref().
>>>   */
>>>  void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
>>>  {
>>> -bdrv_swap(bs_new, bs_top);
>>> +assert(!bdrv_requests_pending(bs_top));
>>> +assert(!bdrv_requests_pending(bs_new));
>>> +
>>> +bdrv_ref(bs_top);
>>> +change_parent_backing_link(bs_top, bs_new);
>>> +
>>> +/* Some fields always stay on top of the backing file chain */
>>> +swap_feature_fields(bs_top, bs_new);
>>> +
>>> +bdrv_set_backing_hd(bs_new, bs_top);
>>> +bdrv_unref(bs_top);
>>>  
>>> -/* The contents of 'tmp' will become bs_top, as we are
>>> - * swapping bs_new and bs_top contents. */
>>> -bdrv_set_backing_hd(bs_top, bs_new);
>>> +/* bs_new is now referenced by its new parents, we don't need the
>>> + * additional reference any more. */
>>> +bdrv_unref(bs_new);
>>>  }
>>
>> Before, all pointers to @bs_new were moved to @bs_top. Now, they stay at
>> @bs_new. I suppose we are assuming there are no pointers to @bs_new,
>> should we assert that, and/or point it out in the documentation?
> 
> How would you assert something like this?

Of course, I have no idea.

>   Also, I think it's currently
> true, but there's no reason why it should stay so. The important part is
> just that it's true while applying the patch because the semantics
> changes. Once it's applied, we have sane behaviour and can make use of
> it.

The thing is that this is exactly the reason for the bug Berto found.
external_snapshot_commit() keeps a reference to state->new_bs (@bs_new)
and uses it afterwards for bdrv_reopen(), whereas it should be using
state->old_bs (@bs_top) after this series.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 07/38] block: Make bdrv_is_inserted() recursive

[Max Reitz]

On 29.09.2015 11:15, Alberto Garcia wrote:
> On Fri 18 Sep 2015 05:22:42 PM CEST, Max Reitz wrote:
>> If bdrv_is_inserted() is called on the top level BDS, it should make
>> sure all nodes in the BDS tree are actually inserted.
>>
>> Signed-off-by: Max Reitz <mre...@redhat.com>
>> ---
>>  block.c | 12 +---
>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 4a089e6..c4fa299 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3247,14 +3247,20 @@ void bdrv_invalidate_cache_all(Error **errp)
>>  bool bdrv_is_inserted(BlockDriverState *bs)
>>  {
>>  BlockDriver *drv = bs->drv;
>> +BdrvChild *child;
>>  
>>  if (!drv) {
>>  return false;
>>  }
>> -if (!drv->bdrv_is_inserted) {
>> -return true;
>> +if (drv->bdrv_is_inserted) {
>> +return drv->bdrv_is_inserted(bs);
>>  }
> 
> If there's drv->bdrv_is_inserted then this code stops here and does not
> iterate the children, is this correct?

Yes, that is how it's supposed to be.

Max

>> -return drv->bdrv_is_inserted(bs);
>> +QLIST_FOREACH(child, >children, next) {
>> +if (!bdrv_is_inserted(child->bs)) {
>> +return false;
>> +}
>> +}
>> +return true;
>>  }
> 
> Berto
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 15/16] block: Add and use bdrv_replace_in_backing_chain()

[Max Reitz]

On 29.09.2015 17:22, Kevin Wolf wrote:
> Am 23.09.2015 um 19:08 hat Max Reitz geschrieben:
>> On 17.09.2015 15:48, Kevin Wolf wrote:
>>> This cleans up the mess we left behind in the mirror code after the
>>> previous patch. Instead of using bdrv_swap(), just change pointers.
>>>
>>> The interface change of the mirror job that callers must consider is
>>> that after job completion, their local BDS pointers still point to the
>>> same node now. qemu-img must change its code accordingly (which makes it
>>> easier to understand); the other callers stays unchanged because after
>>> completion they don't do anything with the BDS, but just with the job,
>>> and the job is still owned by the source BDS.
>>>
>>> Signed-off-by: Kevin Wolf <kw...@redhat.com>
>>> ---
>>>  block.c   | 32 +++-
>>>  block/mirror.c| 23 +++
>>>  include/block/block.h |  4 +++-
>>>  qemu-img.c| 16 
>>>  4 files changed, 49 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 98fc17c..7c21659 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -1095,7 +1095,7 @@ static BdrvChild *bdrv_attach_child(BlockDriverState 
>>> *parent_bs,
>>>  return child;
>>>  }
>>>  
>>> -void bdrv_detach_child(BdrvChild *child)
>>> +static void bdrv_detach_child(BdrvChild *child)
>>>  {
>>>  QLIST_REMOVE(child, next);
>>>  QLIST_REMOVE(child, next_parent);
>>> @@ -2228,6 +2228,36 @@ void bdrv_append(BlockDriverState *bs_new, 
>>> BlockDriverState *bs_top)
>>>  bdrv_unref(bs_new);
>>>  }
>>>  
>>> +void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState 
>>> *new)
>>> +{
>>> +assert(!bdrv_requests_pending(old));
>>> +assert(!bdrv_requests_pending(new));
>>> +
>>> +bdrv_ref(old);
>>> +
>>> +if (old->blk) {
>>> +/* As long as these fields aren't in BlockBackend, but in the 
>>> top-level
>>> + * BlockDriverState, it's not possible for a BDS to have two BBs.
>>> + *
>>> + * We really want to copy the fields from old to new, but we go 
>>> for a
>>> + * swap instead so that pointers aren't duplicated and cause 
>>> trouble.
>>> + * (Also, bdrv_swap() used to do the same.) */
>>> +assert(!new->blk);
>>> +swap_feature_fields(old, new);
>>> +}
>>> +change_parent_backing_link(old, new);
>>> +
>>> +/* Change backing files if a previously independent node is added to 
>>> the
>>> + * chain. For active commit, we replace top by its own (indirect) 
>>> backing
>>> + * file and don't do anything here so we don't build a loop. */
>>> +if (new->backing == NULL && !bdrv_chain_contains(backing_bs(old), 
>>> new)) {
>>> +bdrv_set_backing_hd(new, backing_bs(old));
>>> +bdrv_set_backing_hd(old, NULL);
>>
>> Wouldn't we want @old to keep its backing file?
> 
> Would we? The operation I had in mind was: Given a backing file chain,
> one node in that chain and an independent node, replace the node in the
> chain. That is:
> 
> A <- B <- C <- D
> 
>  X
> 
> becomes
> 
> A <- X <- C <- D
> 
>  B
> 
> Of course, you could define a different operation, but this seemed to be
> the obvious one that the mirror completion needs.

Oops, apparently I missed the backing_bs(old) in
bdrv_set_backing_hd(new, ...).

>> Then bdrv_append() would basically be a special case of this function,
>> with it additionally decrementing the refcount of @bs_new.
> 
> Hm, less duplication sounds nice, but as long as the current way is
> technically correct, can we leave this for a cleanup on top?

And with the backing_bs() taken into account, it is not so duplicated
after all.

Max

> Kevin
> 
>> Max
>>
>>> +}
>>> +
>>> +bdrv_unref(old);
>>> +}
>>> +
>>>  static void bdrv_delete(BlockDriverState *bs)
>>>  {
>>>  assert(!bs->job);
>>
> 
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 11/16] block-backend: Add blk_set_bs()

[Max Reitz]

On 01.10.2015 15:13, Kevin Wolf wrote:
> It allows changing the BlockDriverState that a BlockBackend points to.
> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block/block-backend.c | 17 +
>  include/block/block_int.h |  2 ++
>  2 files changed, 19 insertions(+)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 13/16] block: Implement bdrv_append() without bdrv_swap()

[Max Reitz]

On 01.10.2015 15:13, Kevin Wolf wrote:
> Remember all parent nodes and just change the pointers there instead of
> swapping the contents of the BlockDriverState.
> 
> Handling of snapshot=on must be moved further down in bdrv_open()
> because *pbs (which is the bs pointer in the BlockBackend) must already
> be set before bdrv_append() is called. Otherwise bdrv_append() changes
> the BB's pointer to the temporary snapshot, but bdrv_open() overwrites
> it with the read-only original image.
> 
> We also need to be careful to update callers as the interface changes
> (becomes less insane): Previously, the meaning of the two parameters was
> inverted when bdrv_append() returns. Now any BDS pointers keep pointing
> to the same node.
> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block.c| 112 
> +
>  blockdev.c |   2 +-
>  2 files changed, 85 insertions(+), 29 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 07/16] block: Convert bs->backing_hd to BdrvChild

[Max Reitz]

On 01.10.2015 15:13, Kevin Wolf wrote:
> This is the final step in converting all of the BlockDriverState
> pointers that block drivers use to BdrvChild.
> 
> After this patch, bs->children contains the full list of child nodes
> that are referenced by a given BDS, and these children are only
> referenced through BdrvChild, so that updating the pointer in there is
> enough for changing edges in the graph.
> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block.c   | 116 
> +-
>  block/io.c|  24 +-
>  block/mirror.c|   6 +--
>  block/qapi.c  |   8 ++--
>  block/qcow.c  |   4 +-
>  block/qcow2-cluster.c |   4 +-
>  block/qcow2.c |   6 +--
>  block/qed.c   |  12 ++---
>  block/stream.c|   8 ++--
>  block/vmdk.c  |  21 +
>  block/vvfat.c |   6 +--
>  blockdev.c|   4 +-
>  include/block/block_int.h |  12 +++--
>  qemu-img.c    |   4 +-
>  14 files changed, 125 insertions(+), 110 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 14/16] blockjob: Store device name at job creation

[Max Reitz]

On 01.10.2015 15:13, Kevin Wolf wrote:
> Some block jobs change the block device graph on completion. This means
> that the device that owns the job and originally was addressed with its
> device name may no longer be what the corresponding BlockBackend points
> to.
> 
> Previously, the effects of bdrv_swap() ensured that the job was (at
> least partially) transferred to the target image. Events that contain
> the device name could still use bdrv_get_device_name(job->bs) and get
> the same result.
> 
> After removing bdrv_swap(), this won't work any more. Instead, save the
> device name at job creation and use that copy for QMP events and
> anything else identifying the job.
> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block/mirror.c   |  3 +--
>  blockjob.c   | 15 ---
>  include/block/blockjob.h |  8 
>  3 files changed, 17 insertions(+), 9 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 08/16] block: Manage backing file references in bdrv_set_backing_hd()

[Max Reitz]

On 01.10.2015 15:13, Kevin Wolf wrote:
> This simplifies the code somewhat, especially when dropping whole
> backing file subchains.
> 
> The exception is the mirroring code that does adventurous things with
> bdrv_swap() and in order to keep it working, I had to duplicate most of
> bdrv_set_backing_hd() locally. We'll get rid again of this ugliness
> shortly.
> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block.c   | 35 ++-
>  block/mirror.c| 16 
>  block/stream.c| 30 +-
>  block/vvfat.c |  6 +-
>  include/block/block.h |  1 +
>  5 files changed, 33 insertions(+), 55 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 05/16] block: Convert bs->file to BdrvChild

[Max Reitz]

On 01.10.2015 15:13, Kevin Wolf wrote:
> This patch removes the temporary duplication between bs->file and
> bs->file_child by converting everything to BdrvChild.
> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block.c   | 63 
> ++-
>  block/blkdebug.c  | 32 +---
>  block/blkverify.c | 33 ++---
>  block/bochs.c |  8 +++---
>  block/cloop.c | 10 
>  block/dmg.c   | 20 +++
>  block/io.c| 50 ++---
>  block/parallels.c | 38 ++--
>  block/qapi.c  |  2 +-
>  block/qcow.c  | 42 ---
>  block/qcow2-cache.c   | 11 +
>  block/qcow2-cluster.c | 38 
>  block/qcow2-refcount.c| 45 +
>  block/qcow2-snapshot.c| 30 +++---
>  block/qcow2.c | 62 --
>  block/qed-table.c |  4 +--
>  block/qed.c   | 32 
>  block/raw_bsd.c   | 40 +++---
>  block/snapshot.c  | 12 -
>  block/vdi.c   | 17 +++--
>  block/vhdx-log.c  | 25 ++-
>  block/vhdx.c  | 36 ++-
>  block/vmdk.c  | 27 ++--
>  block/vpc.c   | 34 +
>  include/block/block.h |  8 +-
>  include/block/block_int.h |  3 +--
>  26 files changed, 378 insertions(+), 344 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 2/3] block: mirror - split out part of mirror_run()

[Max Reitz]

On 28.09.2015 05:29, Jeff Cody wrote:
> This is code relocation, to pull the part of mirror_run() that
> calls mirror_iteration out into a separate function.
> 
> Signed-off-by: Jeff Cody <jc...@redhat.com>
> ---
>  block/mirror.c | 206 
> ++---
>  1 file changed, 110 insertions(+), 96 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 1/3] block: allow creation of detached dirty bitmaps

[Max Reitz]

On 28.09.2015 05:29, Jeff Cody wrote:
> This allows the creation of detached dirty bitmaps, so that the
> block driver dirty bitmaps can be used without inserting the
> bitmap into the dirty bitmap list for a BDS.
> 
> To free a bitmap that was created "detached = true", call
> bdrv_release_dirty_bitmap() with the BlockDriverState argument
> as NULL.
> 
> Signed-off-by: Jeff Cody <jc...@redhat.com>
> ---
>  block.c   | 26 --
>  block/mirror.c|  3 ++-
>  blockdev.c|  2 +-
>  include/block/block.h |  1 +
>  migration/block.c |  2 +-
>  5 files changed, 25 insertions(+), 9 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 3/3] block: mirror - zero unallocated target sectors when zero init not present

[Max Reitz]

On 28.09.2015 05:29, Jeff Cody wrote:
> During mirror, if the target device does not have support zero
> initialization, a mirror may result in a corrupt image.
> 
> For instance, on mirror to a host device with format = raw, whatever
> random data is on the target device will still be there for unallocated
> sectors.
> 
> This is because during the mirror, we set the dirty bitmap to copy only
> sectors allocated above 'base'.  In the case of target devices where we
> cannot assume unallocated sectors will be read as zeroes, we need to
> explicitely zero out this data.
> 
> In order to avoid zeroing out all sectors of the target device prior to
> mirroring, we do zeroing as part of the block job.  A second dirty
> bitmap cache is created, to track sectors that are unallocated above
> 'base'.  These sectors are then checked for status of BDRV_BLOCK_ZERO
> on the target - if they are not, then zeroes are explicitly written.
> 
> This only occurs under two conditions:
> 
> 1. 'mode' != "existing"
> 2. bdrv_has_zero_init(target) == NULL
> 
> We perform the mirroring through mirror_iteration() as before, except
> in two passes.  If the above two conditions are met, the first pass
> is using the bitmap tracking unallocated sectors, to write the needed
> zeroes.  Then, the second pass is performed, to mirror the actual data
> as before.
> 
> If the above two conditions are not met, then the first pass is skipped,
> and only the second pass (the one with the actual data) is performed.
> 
> Signed-off-by: Jeff Cody 
> ---
>  block/mirror.c| 109 
> ++
>  blockdev.c|   2 +-
>  include/block/block_int.h |   3 +-
>  qapi/block-core.json  |   6 ++-
>  4 files changed, 87 insertions(+), 33 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 405e5c4..b599176 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -50,7 +50,9 @@ typedef struct MirrorBlockJob {
>  int64_t bdev_length;
>  unsigned long *cow_bitmap;
>  BdrvDirtyBitmap *dirty_bitmap;
> -HBitmapIter hbi;
> +HBitmapIter zero_hbi;
> +HBitmapIter allocated_hbi;
> +HBitmapIter *hbi;
>  uint8_t *buf;
>  QSIMPLEQ_HEAD(, MirrorBuffer) buf_free;
>  int buf_free_count;
> @@ -60,6 +62,8 @@ typedef struct MirrorBlockJob {
>  int sectors_in_flight;
>  int ret;
>  bool unmap;
> +bool zero_unallocated;
> +bool zero_cycle;
>  bool waiting_for_io;
>  } MirrorBlockJob;
>  
> @@ -166,10 +170,10 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>  int pnum;
>  int64_t ret;
>  
> -s->sector_num = hbitmap_iter_next(>hbi);
> +s->sector_num = hbitmap_iter_next(s->hbi);
>  if (s->sector_num < 0) {
> -bdrv_dirty_iter_init(s->dirty_bitmap, >hbi);
> -s->sector_num = hbitmap_iter_next(>hbi);
> +bdrv_dirty_iter_init(s->dirty_bitmap, s->hbi);
> +s->sector_num = hbitmap_iter_next(s->hbi);
>  trace_mirror_restart_iter(s, bdrv_get_dirty_count(s->dirty_bitmap));
>  assert(s->sector_num >= 0);
>  }
> @@ -287,7 +291,7 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>   */
>  if (next_sector > hbitmap_next_sector
>  && bdrv_get_dirty(source, s->dirty_bitmap, next_sector)) {
> -hbitmap_next_sector = hbitmap_iter_next(>hbi);
> +hbitmap_next_sector = hbitmap_iter_next(s->hbi);
>  }
>  
>  next_sector += sectors_per_chunk;
> @@ -300,25 +304,34 @@ static uint64_t coroutine_fn 
> mirror_iteration(MirrorBlockJob *s)
>  s->sectors_in_flight += nb_sectors;
>  trace_mirror_one_iteration(s, sector_num, nb_sectors);
>  
> -ret = bdrv_get_block_status_above(source, NULL, sector_num,
> -  nb_sectors, );
> -if (ret < 0 || pnum < nb_sectors ||
> -(ret & BDRV_BLOCK_DATA && !(ret & BDRV_BLOCK_ZERO))) {
> -bdrv_aio_readv(source, sector_num, >qiov, nb_sectors,
> -   mirror_read_complete, op);
> -} else if (ret & BDRV_BLOCK_ZERO) {
> -bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
> -  s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
> -  mirror_write_complete, op);
> +if (s->zero_cycle) {
> +ret = bdrv_get_block_status(s->target, sector_num, nb_sectors, 
> );
> +if (!(ret & BDRV_BLOCK_ZERO)) {
> +bdrv_aio_write_zeroes(s->target, sector_num, op->nb_sectors,
> +  s->unmap ? BDRV_REQ_MAY_UNMAP : 0,
> +  mirror_write_complete, op);
> +}
>  } else {
> -assert(!(ret & BDRV_BLOCK_DATA));
> -bdrv_aio_discard(s->target, sector_num, op->nb_sectors,
> - mirror_write_complete, op);
> +ret = bdrv_get_block_status_above(source, NULL, sector_num,
> +

Re: [Qemu-block] [PATCH 10/16] block/io: Make bdrv_requests_pending() public

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block/io.c| 2 +-
>  include/block/block_int.h | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mre...@redhat.com>

In addition: Shouldn't we iterate over all children in that function
instead of just file and backing?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 14/16] blockjob: Store device name at job creation

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> Some block jobs change the block device graph on completion. This means
> that the device that owns the job and originally was addressed with its
> device name may no longer be what the corresponding BlockBackend points
> to.
> 
> Previously, the effects of bdrv_swap() ensured that the job was (at
> least partially) transferred to the target image. Events that contain
> the device name could still use bdrv_get_device_name(job->bs) and get
> the same result.
> 
> After removing bdrv_swap(), this won't work any more. Instead, save the
> device name at job creation and use that copy for the events.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  blockjob.c   | 8 +---
>  include/block/blockjob.h | 8 
>  2 files changed, 13 insertions(+), 3 deletions(-)

Why not %s/bdrv_get_device_name(job->bs)/job->id/?

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 16/16] block: Remove bdrv_swap()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> bdrv_swap() is unused now. Remove it and all functions that have
> no other users than bdrv_swap(). In particular, this removes the
> .bdrv_rebind callbacks from block drivers.
> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block.c   | 155 
> +-
>  block/qed.c   |   7 ---
>  block/vvfat.c |   7 ---
>  include/block/block_int.h |   1 -
>  include/qemu/queue.h  |   6 --
>  5 files changed, 1 inse

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 11/16] block-backend: Add blk_set_bs()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> It allows changing the BlockDriverState that a BlockBackend points to.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block/block-backend.c | 16 
>  include/block/block_int.h |  2 ++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index c2e8732..a2afcff 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -239,6 +239,22 @@ BlockDriverState *blk_bs(BlockBackend *blk)
>  }
>  
>  /*
> + * Changes the BlockDriverState attached to @blk
> + */
> +void blk_set_bs(BlockBackend *blk, BlockDriverState *bs)
> +{
> +bdrv_ref(bs);
> +
> +if (blk->bs) {
> +blk->bs->blk = NULL;
> +bdrv_unref(blk->bs);
> +}
> +
> +blk->bs = bs;
> +bs->blk = blk;
> +}
> +
> +/*

As discussed in the BlockBackend and media thread, you should probably
assert(bs->blk == NULL), and I'd rather have this implemented as
blk_remove_bs(blk); blk_insert_bs(blk, bs);, but that's probably
something that won't happen until I do so myself.

Max

>   * Return @blk's DriveInfo if any, else null.
>   */
>  DriveInfo *blk_legacy_dinfo(BlockBackend *blk)
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 4598101..cfcae52 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -659,6 +659,8 @@ void backup_start(BlockDriverState *bs, BlockDriverState 
> *target,
>BlockCompletionFunc *cb, void *opaque,
>Error **errp);
>  
> +void blk_set_bs(BlockBackend *blk, BlockDriverState *bs);
> +
>  void blk_dev_change_media_cb(BlockBackend *blk, bool load);
>  bool blk_dev_has_removable_media(BlockBackend *blk);
>  void blk_dev_eject_request(BlockBackend *blk, bool force);
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 09/16] block: Split bdrv_move_feature_fields()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> After bdrv_swap(), some fields must be moved back to their original BDS
> to compensate for the effects that a swap of the contents of the objects
> has while keeping the old addresses. Other fields must be moved back
> because they should logically be moved and must stay on top
> 
> When replacing bdrv_swap() with operations changing the pointers in the
> parents, we only need the latter and must avoid swapping the former.
> Split the function accordingly.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 12 
>  1 file changed, 12 insertions(+)
> 
> diff --git a/block.c b/block.c
> index 743f82e..7930f3c 100644
> --- a/block.c
> +++ b/block.c
> @@ -1984,6 +1984,8 @@ static void bdrv_rebind(BlockDriverState *bs)
>  }
>  }
>  
> +/* Fields that need to stay with the top-level BDS, no matter whether the
> + * address of the top-level BDS stays the same or not. */
>  static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
>   BlockDriverState *bs_src)
>  {
> @@ -2019,7 +2021,13 @@ static void bdrv_move_feature_fields(BlockDriverState 
> *bs_dest,
>  
>  /* dirty bitmap */
>  bs_dest->dirty_bitmaps  = bs_src->dirty_bitmaps;
> +}
>  
> +/* Fields that only need to be swapped if the contents of BDSes is swapped
> + * rather than pointers being changed in the parents. */
> +static void bdrv_move_reference_fields(BlockDriverState *bs_dest,
> +   BlockDriverState *bs_src)
> +{
>  /* reference count */
>  bs_dest->refcnt = bs_src->refcnt;
>  

I'm not sure whether the op blockers should be moved in this function...
I think they should be moved in bdrv_move_feasture_fields(), because
they generally depend on the position within the node graph and not on
the BDS itself, don't they?

Max

> @@ -2090,6 +2098,10 @@ void bdrv_swap(BlockDriverState *bs_new, 
> BlockDriverState *bs_old)
>  bdrv_move_feature_fields(bs_old, bs_new);
>  bdrv_move_feature_fields(bs_new, );
>  
> +bdrv_move_reference_fields(, bs_old);
> +bdrv_move_reference_fields(bs_old, bs_new);
> +bdrv_move_reference_fields(bs_new, );
> +
>  /* bs_new must remain unattached */
>  assert(!bs_new->blk);
>  
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 12/16] block: Introduce parents list

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block.c   | 3 +++
>  include/block/block_int.h | 2 ++
>  2 files changed, 5 insertions(+)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 13/16] block: Implement bdrv_append() without bdrv_swap()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> Remember all parent nodes and just change the pointers there instead of
> swapping the contents of the BlockDriverState.
> 
> Handling of snapshot=on must be moved further down in bdrv_open()
> because *pbs (which is the bs pointer in the BlockBackend) must already
> be set before bdrv_append() is called. Otherwise bdrv_append() changes
> the BB's pointer to the temporary snapshot, but bdrv_open() overwrites
> it with the read-only original image.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c | 109 
> +++-
>  1 file changed, 81 insertions(+), 28 deletions(-)
> 
> diff --git a/block.c b/block.c
> index c196f83..98fc17c 100644
> --- a/block.c
> +++ b/block.c
> @@ -1516,15 +1516,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>  
>  bdrv_refresh_filename(bs);
>  
> -/* For snapshot=on, create a temporary qcow2 overlay. bs points to the
> - * temporary snapshot afterwards. */
> -if (snapshot_flags) {
> -ret = bdrv_append_temp_snapshot(bs, snapshot_flags, _err);
> -if (local_err) {
> -goto close_and_fail;
> -}
> -}
> -
>  /* Check if any unknown options were used */
>  if (options && (qdict_size(options) != 0)) {
>  const QDictEntry *entry = qdict_first(options);
> @@ -1556,6 +1547,16 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>  
>  QDECREF(options);
>  *pbs = bs;
> +
> +/* For snapshot=on, create a temporary qcow2 overlay. bs points to the
> + * temporary snapshot afterwards. */
> +if (snapshot_flags) {
> +ret = bdrv_append_temp_snapshot(bs, snapshot_flags, _err);
> +if (local_err) {
> +goto close_and_fail;
> +}
> +}
> +
>  return 0;
>  
>  fail:
> @@ -1999,20 +2000,6 @@ static void bdrv_move_feature_fields(BlockDriverState 
> *bs_dest,
>  
>  bs_dest->enable_write_cache = bs_src->enable_write_cache;
>  
> -/* i/o throttled req */
> -bs_dest->throttle_state = bs_src->throttle_state,
> -bs_dest->io_limits_enabled  = bs_src->io_limits_enabled;
> -bs_dest->pending_reqs[0]= bs_src->pending_reqs[0];
> -bs_dest->pending_reqs[1]= bs_src->pending_reqs[1];
> -bs_dest->throttled_reqs[0]  = bs_src->throttled_reqs[0];
> -bs_dest->throttled_reqs[1]  = bs_src->throttled_reqs[1];
> -memcpy(_dest->round_robin,
> -   _src->round_robin,
> -   sizeof(bs_dest->round_robin));
> -memcpy(_dest->throttle_timers,
> -   _src->throttle_timers,
> -   sizeof(ThrottleTimers));
> -
>  /* r/w error */
>  bs_dest->on_read_error  = bs_src->on_read_error;
>  bs_dest->on_write_error = bs_src->on_write_error;
> @@ -2026,10 +2013,25 @@ static void bdrv_move_feature_fields(BlockDriverState 
> *bs_dest,
>  }
>  
>  /* Fields that only need to be swapped if the contents of BDSes is swapped
> - * rather than pointers being changed in the parents. */
> + * rather than pointers being changed in the parents, and throttling fields
> + * because only bdrv_swap() messes with internals of throttling. */
>  static void bdrv_move_reference_fields(BlockDriverState *bs_dest,
> BlockDriverState *bs_src)
>  {
> +/* i/o throttled req */
> +bs_dest->throttle_state = bs_src->throttle_state,
> +bs_dest->io_limits_enabled  = bs_src->io_limits_enabled;
> +bs_dest->pending_reqs[0]= bs_src->pending_reqs[0];
> +bs_dest->pending_reqs[1]= bs_src->pending_reqs[1];
> +bs_dest->throttled_reqs[0]  = bs_src->throttled_reqs[0];
> +bs_dest->throttled_reqs[1]  = bs_src->throttled_reqs[1];
> +memcpy(_dest->round_robin,
> +   _src->round_robin,
> +   sizeof(bs_dest->round_robin));
> +memcpy(_dest->throttle_timers,
> +   _src->throttle_timers,
> +   sizeof(ThrottleTimers));
> +
>  /* reference count */
>  bs_dest->refcnt = bs_src->refcnt;
>  
> @@ -2155,6 +2157,42 @@ void bdrv_swap(BlockDriverState *bs_new, 
> BlockDriverState *bs_old)
>  bdrv_rebind(bs_old);
>  }
>  
> +static void change_parent_backing_link(BlockDriverState *from,
> +   BlockDriverState *to)
> +{
> +BdrvChild *c, *next;
> +
> +QLIST_FOREACH_SAFE(c, >parents, next_parent, next) {
> +assert(c->role != _backing);
> +c->bs = to;
> +QLIST_REMOVE(c, next_parent);
> +QLIST_INSERT_HEAD(>parents, c, next_parent);

This drops a reference from the parent BDS to @from, and adds a new one
from the parent BDS to @to. However, this is not reflected here.

> +}
> +if (from->blk) {
> +blk_set_bs(from->blk, to);
> +if (!to->device_list.tqe_prev) {
> +QTAILQ_INSERT_BEFORE(from, to, device_list);
> +}
> +QTAILQ_REMOVE(_states, from, 

Re: [Qemu-block] [PATCH 15/16] block: Add and use bdrv_replace_in_backing_chain()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> This cleans up the mess we left behind in the mirror code after the
> previous patch. Instead of using bdrv_swap(), just change pointers.
> 
> The interface change of the mirror job that callers must consider is
> that after job completion, their local BDS pointers still point to the
> same node now. qemu-img must change its code accordingly (which makes it
> easier to understand); the other callers stays unchanged because after
> completion they don't do anything with the BDS, but just with the job,
> and the job is still owned by the source BDS.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 32 +++-
>  block/mirror.c| 23 +++
>  include/block/block.h |  4 +++-
>  qemu-img.c| 16 
>  4 files changed, 49 insertions(+), 26 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 98fc17c..7c21659 100644
> --- a/block.c
> +++ b/block.c
> @@ -1095,7 +1095,7 @@ static BdrvChild *bdrv_attach_child(BlockDriverState 
> *parent_bs,
>  return child;
>  }
>  
> -void bdrv_detach_child(BdrvChild *child)
> +static void bdrv_detach_child(BdrvChild *child)
>  {
>  QLIST_REMOVE(child, next);
>  QLIST_REMOVE(child, next_parent);
> @@ -2228,6 +2228,36 @@ void bdrv_append(BlockDriverState *bs_new, 
> BlockDriverState *bs_top)
>  bdrv_unref(bs_new);
>  }
>  
> +void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState 
> *new)
> +{
> +assert(!bdrv_requests_pending(old));
> +assert(!bdrv_requests_pending(new));
> +
> +bdrv_ref(old);
> +
> +if (old->blk) {
> +/* As long as these fields aren't in BlockBackend, but in the 
> top-level
> + * BlockDriverState, it's not possible for a BDS to have two BBs.
> + *
> + * We really want to copy the fields from old to new, but we go for a
> + * swap instead so that pointers aren't duplicated and cause trouble.
> + * (Also, bdrv_swap() used to do the same.) */
> +assert(!new->blk);
> +swap_feature_fields(old, new);
> +}
> +change_parent_backing_link(old, new);
> +
> +/* Change backing files if a previously independent node is added to the
> + * chain. For active commit, we replace top by its own (indirect) backing
> + * file and don't do anything here so we don't build a loop. */
> +if (new->backing == NULL && !bdrv_chain_contains(backing_bs(old), new)) {
> +bdrv_set_backing_hd(new, backing_bs(old));
> +bdrv_set_backing_hd(old, NULL);

Wouldn't we want @old to keep its backing file?

Then bdrv_append() would basically be a special case of this function,
with it additionally decrementing the refcount of @bs_new.

Max

> +}
> +
> +bdrv_unref(old);
> +}
> +
>  static void bdrv_delete(BlockDriverState *bs)
>  {
>  assert(!bs->job);



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 02/16] vmdk: Use BdrvChild instead of BDS for references to extents

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block/vmdk.c | 99 
> +++-
>  1 file changed, 51 insertions(+), 48 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 06/16] block: Remove bdrv_open_image()

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> It is unused now.
> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block.c   | 34 --
>  include/block/block.h |  4 
>  2 files changed, 38 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 07/16] block: Convert bs->backing_hd to BdrvChild

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> This is the final step in converting all of the BlockDriverState
> pointers that block drivers use to BdrvChild.
> 
> After this patch, bs->children contains the full list of child nodes
> that are referenced by a given BDS, and these children are only
> referenced through BdrvChild, so that updating the pointer in there is
> enough for changing edges in the graph.
> 
> Signed-off-by: Kevin Wolf 
> ---
>  block.c   | 116 
> +-
>  block/io.c|  24 +-
>  block/mirror.c|   7 +--
>  block/qapi.c  |   8 ++--
>  block/qcow.c  |   4 +-
>  block/qcow2-cluster.c |   4 +-
>  block/qcow2.c |   6 +--
>  block/qed.c   |  12 ++---
>  block/stream.c|  10 ++--
>  block/vmdk.c  |  21 +
>  block/vvfat.c |   6 +--
>  blockdev.c|   6 +--
>  include/block/block_int.h |  12 +++--
>  qemu-img.c|   8 ++--
>  14 files changed, 130 insertions(+), 114 deletions(-)
> 

[...]

> diff --git a/block/io.c b/block/io.c
> index 8a27efa..d7e742a 100644
> --- a/block/io.c
> +++ b/block/io.c

[...]

> @@ -1604,7 +1604,7 @@ int64_t bdrv_get_block_status(BlockDriverState *bs,
>int64_t sector_num,
>int nb_sectors, int *pnum)
>  {
> -return bdrv_get_block_status_above(bs, bs->backing_hd,
> +return bdrv_get_block_status_above(bs, backing_bs(bs),
> sector_num, nb_sectors, pnum);
>  }
>  
> @@ -1662,7 +1662,7 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>  n = pnum_inter;
>  }
>  
> -intermediate = intermediate->backing_hd;
> +intermediate = intermediate->backing ? intermediate->backing->bs : 
> NULL;

backing_bs(intermediate)?

>  }
>  
>  *pnum = n;
> diff --git a/block/mirror.c b/block/mirror.c
> index a258926..259e11a 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -371,7 +371,8 @@ static void mirror_exit(BlockJob *job, void *opaque)
>  if (s->common.driver->job_type == BLOCK_JOB_TYPE_COMMIT) {
>  /* drop the bs loop chain formed by the swap: break the loop then
>   * trigger the unref from the top one */
> -BlockDriverState *p = s->base->backing_hd;
> +BlockDriverState *p = s->base->backing
> +? s->base->backing->bs : NULL;

Maybe you don't want to use backing_bs() outside of the core block
layer, but it could be used here, too.

(There are two similar expressions in block/stream.c, and maybe
elsewhere, too)

>  bdrv_set_backing_hd(s->base, NULL);
>  bdrv_unref(p);
>  }

[...]

> diff --git a/blockdev.c b/blockdev.c
> index 32b04b4..bc158ff 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -2508,10 +2508,10 @@ void qmp_drive_backup(const char *device, const char 
> *target,
>  /* See if we have a backing HD we can use to create our new image
>   * on top of. */
>  if (sync == MIRROR_SYNC_MODE_TOP) {
> -source = bs->backing_hd;
> -if (!source) {
> +if (!bs->backing) {
>  sync = MIRROR_SYNC_MODE_FULL;
>  }
> +source = bs->backing->bs;

That doesn't seem right. In case of !bs->backing, this won't abort but
just continue and run into bs->backing->bs, which should therefore
probably be backing_bs(bs) instead.

>  }
>  if (sync == MIRROR_SYNC_MODE_NONE) {
>  source = bs;
> @@ -2716,7 +2716,7 @@ void qmp_drive_mirror(const char *device, const char 
> *target,
>  }
>  
>  flags = bs->open_flags | BDRV_O_RDWR;
> -source = bs->backing_hd;
> +source = bs->backing ? bs->backing->bs : NULL;

Why not source = backing_bs(bs)?

>  if (!source && sync == MIRROR_SYNC_MODE_TOP) {
>  sync = MIRROR_SYNC_MODE_FULL;
>  }

[...]

> diff --git a/qemu-img.c b/qemu-img.c
> index 6ff4e85..c4454da 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c

[...]

> @@ -2206,11 +2206,11 @@ static int get_block_status(BlockDriverState *bs, 
> int64_t sector_num,
>  if (ret & (BDRV_BLOCK_ZERO|BDRV_BLOCK_DATA)) {
>  break;
>  }
> -bs = bs->backing_hd;
> -if (bs == NULL) {
> +if (bs->backing == NULL) {
>  ret = 0;
>  break;
>  }
> +bs = bs->backing->bs;

This changes behavior. bs needs to be set to NULL in the
if (bs->backing == NULL) block, or the break will break: Before, if
bs->backing_hd == NULL, the loop was left with bs == NULL. Now, bs won't
be NULL anymore (but its value is used after the loop and stored in e->bs).

Max

>  
>  depth++;
>  }
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 05/16] block: Convert bs->file to BdrvChild

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> This patch removes the temporary duplication between bs->file and
> bs->file_child by converting everything to BdrvChild.
> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block.c   | 61 ++
>  block/blkdebug.c  | 32 
>  block/blkverify.c | 33 ++---
>  block/bochs.c |  8 +++---
>  block/cloop.c | 10 
>  block/dmg.c   | 20 +++
>  block/io.c| 50 +++---
>  block/parallels.c | 38 +++--
>  block/qapi.c  |  2 +-
>  block/qcow.c  | 42 +---
>  block/qcow2-cache.c   | 11 +
>  block/qcow2-cluster.c | 38 +
>  block/qcow2-refcount.c| 45 ++
>  block/qcow2-snapshot.c| 30 ---
>  block/qcow2.c | 62 
> ---
>  block/qed-table.c |  4 +--
>  block/qed.c   | 32 
>  block/raw_bsd.c   | 40 +++---
>  block/snapshot.c  | 12 -
>  block/vdi.c   | 17 +++--
>  block/vhdx-log.c  | 25 ++-
>  block/vhdx.c  | 36 ++-
>  block/vmdk.c  | 27 +++--
>  block/vpc.c   | 34 ++
>  include/block/block.h |  8 +-
>  include/block/block_int.h |  3 +--
>  26 files changed, 377 insertions(+), 343 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>

> diff --git a/block.c b/block.c
> index 2e9e5e2..93d713b 100644
> --- a/block.c
> +++ b/block.c

[...]

> @@ -1929,6 +1925,11 @@ void bdrv_close(BlockDriverState *bs)
>  bdrv_unref(backing_hd);
>  }
>  
> +if (bs->file != NULL) {
> +bdrv_unref(bs->file->bs);

I think we can use bdrv_unref_child(bs->file) here. I'd personally like
it more, at least (because using bdrv_unref() and relying on the loop
below is basically what the comment inside of the loop advises against).

Yes, I know, eventually, we want to drop this block anyway and let the
loop below handle everything using bdrv_unref_child(). But when we do
that conversion, it'll be obvious to drop a bdrv_unref_child() call,
whereas dropping bdrv_unref() alone isn't so obvious.

Max

> +bs->file = NULL;
> +}
> +
>  QLIST_FOREACH_SAFE(child, >children, next, next) {
>  /* TODO Remove bdrv_unref() from drivers' close function and use
>   * bdrv_unref_child() here */
> @@ -1953,11 +1954,6 @@ void bdrv_close(BlockDriverState *bs)
>  bs->options = NULL;
>  QDECREF(bs->full_open_options);
>  bs->full_open_options = NULL;
> -
> -if (bs->file != NULL) {
> -bdrv_unref(bs->file);
> -bs->file = NULL;
> -}
>  }
>  
>  if (bs->blk) {



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 03/16] blkverify: Convert s->test_file to BdrvChild

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block/blkverify.c | 41 +
>  1 file changed, 21 insertions(+), 20 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 01/16] block: Introduce BDS.file_child

[Max Reitz]

On 22.09.2015 19:14, Max Reitz wrote:
> On 17.09.2015 15:48, Kevin Wolf wrote:
>> Store the BdrvChild for bs->file. At this point, bs->file_child->bs just
>> duplicates the bs->file pointer. Later, it will completely replace it.
>>
>> Signed-off-by: Kevin Wolf <kw...@redhat.com>
>> ---
>>  block.c   | 12 +---
>>  include/block/block_int.h |  1 +
>>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> Reviewed-by: Max Reitz <mre...@redhat.com>
> 
> Although I have a small comment below:
> 
>>
>> diff --git a/block.c b/block.c
>> index 6268e37..2e9e5e2 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -1487,11 +1487,17 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
>> const char *filename,
>>  
>>  assert(file == NULL);
> 
> This looks strange now, without the direct connection to
> bdrv_open_image(). I'd drop this...
> 
>>  bs->open_flags = flags;
>> -ret = bdrv_open_image(, filename, options, "file",
>> -  bs, _file, true, _err);
>> -if (ret < 0) {
>> +
>> +bs->file_child = bdrv_open_child(filename, options, "file", bs,
>> + _file, true, _err);
>> +if (local_err) {
>> +ret = -EINVAL;
>>  goto fail;
>>  }
>> +
>> +if (bs->file_child) {
>> +file = bs->file_child->bs;
>> +}
> 
> And make this file = bs->file_child ? bs->file_child->bs : NULL (or the
> respective long form, i.e.
> if (bs->file_child) { ... } else { file = NULL; }).
> 
> We could even put this after this if block and drop the NULL
> initialization of file, but that might go overboard.

And expecting something like this, I skipped ahead to patch 5, which
drops this altogether. An R-b without any comments it is, then. :-)

Max

>>  }
>>  
>>  /* Image format probing */
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 14ad4c3..d0dd93e 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -381,6 +381,7 @@ struct BlockDriverState {
>>  BlockDriverState *backing_hd;
>>  BdrvChild *backing_child;
>>  BlockDriverState *file;
>> +BdrvChild *file_child;
>>  
>>  NotifierList close_notifiers;
>>  
>>
> 
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 01/16] block: Introduce BDS.file_child

[Max Reitz]

On 17.09.2015 15:48, Kevin Wolf wrote:
> Store the BdrvChild for bs->file. At this point, bs->file_child->bs just
> duplicates the bs->file pointer. Later, it will completely replace it.
> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block.c   | 12 +---
>  include/block/block_int.h |  1 +
>  2 files changed, 10 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>

Although I have a small comment below:

> 
> diff --git a/block.c b/block.c
> index 6268e37..2e9e5e2 100644
> --- a/block.c
> +++ b/block.c
> @@ -1487,11 +1487,17 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
> const char *filename,
>  
>  assert(file == NULL);

This looks strange now, without the direct connection to
bdrv_open_image(). I'd drop this...

>  bs->open_flags = flags;
> -ret = bdrv_open_image(, filename, options, "file",
> -  bs, _file, true, _err);
> -if (ret < 0) {
> +
> +bs->file_child = bdrv_open_child(filename, options, "file", bs,
> + _file, true, _err);
> +if (local_err) {
> +ret = -EINVAL;
>  goto fail;
>  }
> +
> +if (bs->file_child) {
> +file = bs->file_child->bs;
> +}

And make this file = bs->file_child ? bs->file_child->bs : NULL (or the
respective long form, i.e.
if (bs->file_child) { ... } else { file = NULL; }).

We could even put this after this if block and drop the NULL
initialization of file, but that might go overboard.

Max

>  }
>  
>  /* Image format probing */
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 14ad4c3..d0dd93e 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -381,6 +381,7 @@ struct BlockDriverState {
>  BlockDriverState *backing_hd;
>  BdrvChild *backing_child;
>  BlockDriverState *file;
> +BdrvChild *file_child;
>  
>  NotifierList close_notifiers;
>  
> 




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 19/38] block: Fail requests to empty BlockBackend

[Max Reitz]

On 22.09.2015 16:30, Kevin Wolf wrote:
> Am 18.09.2015 um 17:22 hat Max Reitz geschrieben:
>> If there is no BlockDriverState in a BlockBackend or if the tray of the
>> guest device is open, fail all requests (where that is possible) with
>> -ENOMEDIUM.
>>
>> The reason the status of the guest device is taken into account is
>> because once the guest device's tray is opened, any request on the same
>> BlockBackend as the guest uses should fail. If the BDS tree is supposed
>> to be usable even after ejecting it from the guest, a different
>> BlockBackend must be used.
>>
>> Signed-off-by: Max Reitz <mre...@redhat.com>
>> Reviewed-by: Eric Blake <ebl...@redhat.com>
> 
> Do we want to include blk_drain() to make it a no-op instead of
> crashing?

Yes, we do, so that would be in patch 20. Thanks for catching that.

> Also, we're now introducing BlockAIOCBs with a NULL bs with your use of
> abort_aio_request. I haven't carefully reviewed the implications of this
> yet, but that should definitely be done before we merge the series.

That should be patch 12.

Max




signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v5 17/38] block: Add BlockBackendRootState

[Max Reitz]

On 22.09.2015 16:17, Kevin Wolf wrote:
> Am 18.09.2015 um 17:22 hat Max Reitz geschrieben:
>> This structure will store some of the state of the root BDS if the BDS
>> tree is removed, so that state can be restored once a new BDS tree is
>> inserted.
> 
> This is magic that is bound to bite us sooner or later. I see that we
> have to do this in order to maintain compatibility, but imagine we'll
> have multiple BlockBackends sitting on top of the same BDS. They'll be
> fighting for whose throttling settings are applied. With this approach,
> the winner is the BlockBackend that most recently went from no medium to
> medium, which might just be the most surprising way to solve the
> problem.

Well, my first answer to this is that as far as I remember and as far as
I can see right now, it's only qmp_blockdev_change_medium() that
accesses this structure. This is effectively a legacy command, even
though it is introduced by this series.

When using the "atomic" operations, this structure will not be used.

So let's examine what cases of multiple BBs on top of the same BDS we
can have, that are relevant here: One of the BBs always has to be the BB
belonging to a device such as a CD drive, otherwise we cannot invoke the
change command. The other BB(s) can be either:
(1) Also BBs belonging to CDD-like devices (let's call them "tray
devices")
(2) Other BBs such as block job BBs or NBD BBs

In the first case, the scenario you described can indeed occur, but only
if the user is indeed using the change command. I'm very much inclined
to say it's the user's own fault for having multiple tray device BBs on
top of a single BDS and managing them with the change command. In any
case, with the current state of having throttling at the BDS level, we
are guarenteed to break something, at least.

In the second case, throttling will change for all the attached BBs
whenever the BDS is inserted into a throttled tray device. This seems
correct to me, at least as long as we have throttling on the BDS level.

> I'll detail one aspect of the problem below.
> 
> The correct way to solve this seems to be that each BB has its own I/O
> throttling filter. Actually, if we could lift the throttling code to
> BlockBackend, that might solve the problem.

So yes, as long as we have throttling on the BDS level, something may
always break when having multiple BBs per BDS, because you'd probably
want throttling to be on the BB level. But lifting that doesn't seem a
trivial task, and I don't really know whether I want this in this series.

Especially considering that right now you cannot have multiple BBs per BDS.

All in all: Yes, before we allow multiple BBs per BDS we want throttling
to be on the BB level. But this is nothing that should be done in or for
this series, since right now we do not allow multiple BBs per BDS.

> I guess the same applies for the other fields in BlockBackendRootState.
> 
>> Signed-off-by: Max Reitz <mre...@redhat.com>
>> Reviewed-by: Eric Blake <ebl...@redhat.com>
>> ---
>>  block/block-backend.c  | 37 +
>>  include/block/block_int.h  | 10 ++
>>  include/qemu/typedefs.h|  1 +
>>  include/sysemu/block-backend.h |  2 ++
>>  4 files changed, 50 insertions(+)
>>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 1f2cd9b..9590be5 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -13,6 +13,7 @@
>>  #include "sysemu/block-backend.h"
>>  #include "block/block_int.h"
>>  #include "block/blockjob.h"
>> +#include "block/throttle-groups.h"
>>  #include "sysemu/blockdev.h"
>>  #include "sysemu/sysemu.h"
>>  #include "qapi-event.h"
>> @@ -37,6 +38,10 @@ struct BlockBackend {
>>  /* the block size for which the guest device expects atomicity */
>>  int guest_block_size;
>>  
>> +/* If the BDS tree is removed, some of its options are stored here 
>> (which
>> + * can be used to restore those options in the new BDS on insert) */
>> +BlockBackendRootState root_state;
>> +
>>  /* I/O stats (display with "info blockstats"). */
>>  BlockAcctStats stats;
>>  
>> @@ -161,6 +166,7 @@ static void blk_delete(BlockBackend *blk)
>>  bdrv_unref(blk->bs);
>>  blk->bs = NULL;
>>  }
>> +g_free(blk->root_state.throttle_group_name);
>>  /* Avoid double-remove after blk_hide_on_behalf_of_hmp_drive_del() */
>>  if (blk->name[0]) {
>>  QTAILQ_REMOVE(_backends, blk, link);
>> @@ -1050,3 +1056,34 @@ int blk_probe_geometry(BlockBack

Re: [Qemu-block] [PATCH v5 21/38] block: Add blk_insert_bs()

[Max Reitz]

On 22.09.2015 16:42, Kevin Wolf wrote:
> Am 18.09.2015 um 17:22 hat Max Reitz geschrieben:
>> This function associates the given BlockDriverState with the given
>> BlockBackend.
>>
>> Signed-off-by: Max Reitz <mre...@redhat.com>
>> Reviewed-by: Eric Blake <ebl...@redhat.com>
>> Reviewed-by: Alberto Garcia <be...@igalia.com>
>> ---
>>  block/block-backend.c  | 16 
>>  include/sysemu/block-backend.h |  1 +
>>  2 files changed, 17 insertions(+)
>>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index 33145f8..652385e 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -314,6 +314,22 @@ void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend 
>> *blk)
>>  }
>>  
>>  /*
>> + * Associates a new BlockDriverState with @blk.
>> + */
>> +void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
>> +{
>> +if (bs->blk == blk) {
>> +return;
>> +}
>> +
>> +assert(!blk->bs);
>> +assert(!bs->blk);
> 
> Why is it useful to allow reconnecting a BDS to a BB it's already
> connected to? I would have expected that we can assert that this is not
> the case.

We can do that, too, there is no use case. It's just that I in principle
don't like making things an error that do make sense and can trivially
be handled gracefully. But I can see people wanting to hit an assertion
as soon as something looks fishy.

So I'm fine either way. I think Eric asked about the same before, so
that makes two against one now. :-)

>> +bdrv_ref(bs);
>> +blk->bs = bs;
>> +bs->blk = blk;
>> +}
> 
> My series to remove bdrv_swap() introduces a blk_set_bs() function,
> which looks suspiciously similar to this one, except that it allows
> passing a BB that already had a BDS (it gets unrefed then) and that I
> don't assert that the BDS isn't attached to a BB yet (I should do that).
> 
> Do you think that's similar enough to have only one function?

Well, yours looks like blk_remove_bs()+blk_insert_bs() combined. In case
we have called blk_remove_bs() before, it will effectively be the same,
yes. But that difference still bothers me a bit... I'd prefer
implementing blk_set_bs() by calling blk_remove_bs() and then
blk_insert_bs() instead, but since these functions are not available in
your bdrv_swap() series, that would prove rather difficult.

I don't know. Maybe implement it separately for now, and trust that this
series will stay in flight long enough for the bdrv_swap() series to get
merged so I can include a commit cleaning up blk_set_bs() in this series
later on?

Or I can base this series on your bdrv_swap() series. Your call, as I
haven't looked at it yet and thus don't know how long it'll take to get
merged (albeit judging from your series in the past, it won't be too long).

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 3/4] block: add a 'blockdev-snapshot' QMP command

[Max Reitz]

On 14.09.2015 18:01, Alberto Garcia wrote:
> One of the limitations of the 'blockdev-snapshot-sync' command is that
> it does not allow passing BlockdevOptions to the newly created
> snapshots, so they are always opened using the default values.
> 
> Extending the command to allow passing options is not a practical
> solution because there is overlap between those options and some of
> the existing parameters of the command.
> 
> This patch introduces a new 'blockdev-snapshot' command with a simpler
> interface: it just takes two references to existing block devices that
> will be used as the source and target for the snapshot.
> 
> Since the main difference between the two commands is that one of them
> creates and opens the target image, while the other uses an already
> opened one, the bulk of the implementation is shared.
> 
> Signed-off-by: Alberto Garcia <be...@igalia.com>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
> Cc: Max Reitz <mre...@redhat.com>
> ---
>  blockdev.c   | 163 
> ---
>  qapi-schema.json |   2 +
>  qapi/block-core.json |  28 +
>  qmp-commands.hx      |  38 
>  4 files changed, 171 insertions(+), 60 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v4 4/4] block: add tests for the 'blockdev-snapshot' command

[Max Reitz]

On 14.09.2015 18:01, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia 
> ---
>  tests/qemu-iotests/085 | 102 
> ++---
>  tests/qemu-iotests/085.out |  34 ++-
>  2 files changed, 128 insertions(+), 8 deletions(-)

Looks good, but the output diff needs a rebase due to fe646693.

Max



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v5 19/38] block: Fail requests to empty BlockBackend

[Max Reitz]

If there is no BlockDriverState in a BlockBackend or if the tray of the
guest device is open, fail all requests (where that is possible) with
-ENOMEDIUM.

The reason the status of the guest device is taken into account is
because once the guest device's tray is opened, any request on the same
BlockBackend as the guest uses should fail. If the BDS tree is supposed
to be usable even after ejecting it from the guest, a different
BlockBackend must be used.

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
 block/block-backend.c | 55 ++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 417374f..0da4be9 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -509,7 +509,7 @@ static int blk_check_byte_request(BlockBackend *blk, 
int64_t offset,
 return -EIO;
 }
 
-if (!blk_is_inserted(blk)) {
+if (!blk_is_available(blk)) {
 return -ENOMEDIUM;
 }
 
@@ -648,6 +648,10 @@ int blk_pwrite(BlockBackend *blk, int64_t offset, const 
void *buf, int count)
 
 int64_t blk_getlength(BlockBackend *blk)
 {
+if (!blk_is_available(blk)) {
+return -ENOMEDIUM;
+}
+
 return bdrv_getlength(blk->bs);
 }
 
@@ -658,6 +662,10 @@ void blk_get_geometry(BlockBackend *blk, uint64_t 
*nb_sectors_ptr)
 
 int64_t blk_nb_sectors(BlockBackend *blk)
 {
+if (!blk_is_available(blk)) {
+return -ENOMEDIUM;
+}
+
 return bdrv_nb_sectors(blk->bs);
 }
 
@@ -688,6 +696,10 @@ BlockAIOCB *blk_aio_writev(BlockBackend *blk, int64_t 
sector_num,
 BlockAIOCB *blk_aio_flush(BlockBackend *blk,
   BlockCompletionFunc *cb, void *opaque)
 {
+if (!blk_is_available(blk)) {
+return abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
+}
+
 return bdrv_aio_flush(blk->bs, cb, opaque);
 }
 
@@ -729,12 +741,20 @@ int blk_aio_multiwrite(BlockBackend *blk, BlockRequest 
*reqs, int num_reqs)
 
 int blk_ioctl(BlockBackend *blk, unsigned long int req, void *buf)
 {
+if (!blk_is_available(blk)) {
+return -ENOMEDIUM;
+}
+
 return bdrv_ioctl(blk->bs, req, buf);
 }
 
 BlockAIOCB *blk_aio_ioctl(BlockBackend *blk, unsigned long int req, void *buf,
   BlockCompletionFunc *cb, void *opaque)
 {
+if (!blk_is_available(blk)) {
+return abort_aio_request(blk, cb, opaque, -ENOMEDIUM);
+}
+
 return bdrv_aio_ioctl(blk->bs, req, buf, cb, opaque);
 }
 
@@ -750,11 +770,19 @@ int blk_co_discard(BlockBackend *blk, int64_t sector_num, 
int nb_sectors)
 
 int blk_co_flush(BlockBackend *blk)
 {
+if (!blk_is_available(blk)) {
+return -ENOMEDIUM;
+}
+
 return bdrv_co_flush(blk->bs);
 }
 
 int blk_flush(BlockBackend *blk)
 {
+if (!blk_is_available(blk)) {
+return -ENOMEDIUM;
+}
+
 return bdrv_flush(blk->bs);
 }
 
@@ -876,6 +904,11 @@ void blk_set_enable_write_cache(BlockBackend *blk, bool 
wce)
 
 void blk_invalidate_cache(BlockBackend *blk, Error **errp)
 {
+if (!blk->bs) {
+error_setg(errp, "Device '%s' has no medium", blk->name);
+return;
+}
+
 bdrv_invalidate_cache(blk->bs, errp);
 }
 
@@ -1031,6 +1064,10 @@ int blk_write_compressed(BlockBackend *blk, int64_t 
sector_num,
 
 int blk_truncate(BlockBackend *blk, int64_t offset)
 {
+if (!blk_is_available(blk)) {
+return -ENOMEDIUM;
+}
+
 return bdrv_truncate(blk->bs, offset);
 }
 
@@ -1047,21 +1084,37 @@ int blk_discard(BlockBackend *blk, int64_t sector_num, 
int nb_sectors)
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
  int64_t pos, int size)
 {
+if (!blk_is_available(blk)) {
+return -ENOMEDIUM;
+}
+
 return bdrv_save_vmstate(blk->bs, buf, pos, size);
 }
 
 int blk_load_vmstate(BlockBackend *blk, uint8_t *buf, int64_t pos, int size)
 {
+if (!blk_is_available(blk)) {
+return -ENOMEDIUM;
+}
+
 return bdrv_load_vmstate(blk->bs, buf, pos, size);
 }
 
 int blk_probe_blocksizes(BlockBackend *blk, BlockSizes *bsz)
 {
+if (!blk_is_available(blk)) {
+return -ENOMEDIUM;
+}
+
 return bdrv_probe_blocksizes(blk->bs, bsz);
 }
 
 int blk_probe_geometry(BlockBackend *blk, HDGeometry *geo)
 {
+if (!blk_is_available(blk)) {
+return -ENOMEDIUM;
+}
+
 return bdrv_probe_geometry(blk->bs, geo);
 }
 
-- 
2.5.2

Re: [Qemu-block] [PATCH v5 2/4] block: support passing 'backing': '' to 'blockdev-add'

[Max Reitz]

On 18.09.2015 15:16, Alberto Garcia wrote:
> Passing an empty string allows opening an image but not its backing
> file. This was already described in the API documentation, only the
> implementation was missing.
> 
> This is useful for creating snapshots using images opened with
> blockdev-add, since they are not supposed to have a backing image
> before the operation.
> 
> Signed-off-by: Alberto Garcia <be...@igalia.com>
> ---
>  block.c | 7 +++
>  1 file changed, 7 insertions(+)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v5 24/38] blockdev: Pull out blockdev option extraction

[Max Reitz]

Extract some of the blockdev option extraction code from blockdev_init()
into its own function. This simplifies blockdev_init() and will allow
reusing the code in a different function added in a follow-up patch.

Signed-off-by: Max Reitz <mre...@redhat.com>
---
 blockdev.c | 209 +
 1 file changed, 113 insertions(+), 96 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index e76a7de..688ee5f 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -350,25 +350,128 @@ static bool check_throttle_config(ThrottleConfig *cfg, 
Error **errp)
 
 typedef enum { MEDIA_DISK, MEDIA_CDROM } DriveMediaType;
 
+static void extract_common_blockdev_options(QemuOpts *opts, int *bdrv_flags,
+ThrottleConfig *throttle_cfg, BlockdevDetectZeroesOptions *detect_zeroes,
+const char **throttling_group, Error **errp)
+{
+const char *discard;
+Error *local_error = NULL;
+#ifdef CONFIG_LINUX_AIO
+const char *aio;
+#endif
+
+if (!qemu_opt_get_bool(opts, "read-only", false)) {
+*bdrv_flags |= BDRV_O_RDWR;
+}
+if (qemu_opt_get_bool(opts, "copy-on-read", false)) {
+*bdrv_flags |= BDRV_O_COPY_ON_READ;
+}
+
+if ((discard = qemu_opt_get(opts, "discard")) != NULL) {
+if (bdrv_parse_discard_flags(discard, bdrv_flags) != 0) {
+error_setg(errp, "Invalid discard option");
+return;
+}
+}
+
+if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_WB, true)) {
+*bdrv_flags |= BDRV_O_CACHE_WB;
+}
+if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_DIRECT, false)) {
+*bdrv_flags |= BDRV_O_NOCACHE;
+}
+if (qemu_opt_get_bool(opts, BDRV_OPT_CACHE_NO_FLUSH, false)) {
+*bdrv_flags |= BDRV_O_NO_FLUSH;
+}
+
+#ifdef CONFIG_LINUX_AIO
+if ((aio = qemu_opt_get(opts, "aio")) != NULL) {
+if (!strcmp(aio, "native")) {
+*bdrv_flags |= BDRV_O_NATIVE_AIO;
+} else if (!strcmp(aio, "threads")) {
+/* this is the default */
+} else {
+   error_setg(errp, "invalid aio option");
+   return;
+}
+}
+#endif
+
+/* disk I/O throttling */
+memset(throttle_cfg, 0, sizeof(*throttle_cfg));
+throttle_cfg->buckets[THROTTLE_BPS_TOTAL].avg =
+qemu_opt_get_number(opts, "throttling.bps-total", 0);
+throttle_cfg->buckets[THROTTLE_BPS_READ].avg  =
+qemu_opt_get_number(opts, "throttling.bps-read", 0);
+throttle_cfg->buckets[THROTTLE_BPS_WRITE].avg =
+qemu_opt_get_number(opts, "throttling.bps-write", 0);
+throttle_cfg->buckets[THROTTLE_OPS_TOTAL].avg =
+qemu_opt_get_number(opts, "throttling.iops-total", 0);
+throttle_cfg->buckets[THROTTLE_OPS_READ].avg =
+qemu_opt_get_number(opts, "throttling.iops-read", 0);
+throttle_cfg->buckets[THROTTLE_OPS_WRITE].avg =
+qemu_opt_get_number(opts, "throttling.iops-write", 0);
+
+throttle_cfg->buckets[THROTTLE_BPS_TOTAL].max =
+qemu_opt_get_number(opts, "throttling.bps-total-max", 0);
+throttle_cfg->buckets[THROTTLE_BPS_READ].max  =
+qemu_opt_get_number(opts, "throttling.bps-read-max", 0);
+throttle_cfg->buckets[THROTTLE_BPS_WRITE].max =
+qemu_opt_get_number(opts, "throttling.bps-write-max", 0);
+throttle_cfg->buckets[THROTTLE_OPS_TOTAL].max =
+qemu_opt_get_number(opts, "throttling.iops-total-max", 0);
+throttle_cfg->buckets[THROTTLE_OPS_READ].max =
+qemu_opt_get_number(opts, "throttling.iops-read-max", 0);
+throttle_cfg->buckets[THROTTLE_OPS_WRITE].max =
+qemu_opt_get_number(opts, "throttling.iops-write-max", 0);
+
+throttle_cfg->op_size =
+qemu_opt_get_number(opts, "throttling.iops-size", 0);
+
+*throttling_group = qemu_opt_get(opts, "throttling.group");
+
+if (!check_throttle_config(throttle_cfg, errp)) {
+return;
+}
+
+*detect_zeroes =
+qapi_enum_parse(BlockdevDetectZeroesOptions_lookup,
+qemu_opt_get(opts, "detect-zeroes"),
+BLOCKDEV_DETECT_ZEROES_OPTIONS_MAX,
+BLOCKDEV_DETECT_ZEROES_OPTIONS_OFF,
+_error);
+if (local_error) {
+error_propagate(errp, local_error);
+return;
+}
+
+if (*detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
+!(*bdrv_flags & BDRV_O_UNMAP))
+{
+error_setg(errp, "setting detect-zeroes to unmap is not allowed "
+ "without setting discard operation to unmap");
+return;
+}
+}
+
 /* Takes the ownership of bs_opts */
 static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
  

[Qemu-block] [PATCH v5 25/38] blockdev: Allow more options for BB-less BDS tree

[Max Reitz]

Most of the options which blockdev_init() parses for both the
BlockBackend and the root BDS are valid for just the root BDS as well
(e.g. read-only). This patch allows specifying these options even if not
creating a BlockBackend.

Signed-off-by: Max Reitz <mre...@redhat.com>
---
 blockdev.c | 160 ++---
 1 file changed, 154 insertions(+), 6 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 688ee5f..647ce0b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -612,6 +612,65 @@ err_no_opts:
 return NULL;
 }
 
+static QemuOptsList qemu_root_bds_opts;
+
+/* Takes the ownership of bs_opts */
+static BlockDriverState *bds_tree_init(QDict *bs_opts, Error **errp)
+{
+BlockDriverState *bs;
+QemuOpts *opts;
+Error *local_error = NULL;
+ThrottleConfig cfg;
+BlockdevDetectZeroesOptions detect_zeroes;
+const char *throttling_group = NULL;
+int ret;
+int bdrv_flags = 0;
+
+opts = qemu_opts_create(_root_bds_opts, NULL, 1, errp);
+if (!opts) {
+goto fail;
+}
+
+qemu_opts_absorb_qdict(opts, bs_opts, _error);
+if (local_error) {
+error_propagate(errp, local_error);
+goto fail;
+}
+
+extract_common_blockdev_options(opts, _flags, , _zeroes,
+_group, _error);
+if (local_error) {
+error_propagate(errp, local_error);
+goto fail;
+}
+
+bs = NULL;
+ret = bdrv_open(, NULL, NULL, bs_opts, bdrv_flags, errp);
+if (ret < 0) {
+goto fail_no_bs_opts;
+}
+
+bs->detect_zeroes = detect_zeroes;
+
+/* disk I/O throttling */
+if (throttle_enabled()) {
+if (!throttling_group) {
+throttling_group = bdrv_get_node_name(bs);
+}
+bdrv_io_limits_enable(bs, throttling_group);
+bdrv_set_io_limits(bs, );
+}
+
+fail_no_bs_opts:
+qemu_opts_del(opts);
+return bs;
+
+fail:
+qemu_opts_del(opts);
+QDECREF(bs_opts);
+return NULL;
+}
+
 static void qemu_opt_rename(QemuOpts *opts, const char *from, const char *to,
 Error **errp)
 {
@@ -3170,18 +3229,14 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
**errp)
 
 bs = blk_bs(blk);
 } else {
-int ret;
-
 if (!qdict_get_try_str(qdict, "node-name")) {
 error_setg(errp, "'id' and/or 'node-name' need to be specified for 
"
"the root node");
 goto fail;
 }
 
-bs = NULL;
-ret = bdrv_open(, NULL, NULL, qdict, BDRV_O_RDWR | BDRV_O_CACHE_WB,
-errp);
-if (ret < 0) {
+bs = bds_tree_init(qdict, errp);
+if (!bs) {
 goto fail;
 }
 }
@@ -3336,6 +3391,99 @@ QemuOptsList qemu_common_drive_opts = {
 },
 };
 
+static QemuOptsList qemu_root_bds_opts = {
+.name = "root-bds",
+.head = QTAILQ_HEAD_INITIALIZER(qemu_common_drive_opts.head),
+.desc = {
+{
+.name = "discard",
+.type = QEMU_OPT_STRING,
+.help = "discard operation (ignore/off, unmap/on)",
+},{
+.name = "cache.writeback",
+.type = QEMU_OPT_BOOL,
+.help = "enables writeback mode for any caches",
+},{
+.name = "cache.direct",
+.type = QEMU_OPT_BOOL,
+.help = "enables use of O_DIRECT (bypass the host page cache)",
+},{
+.name = "cache.no-flush",
+.type = QEMU_OPT_BOOL,
+.help = "ignore any flush requests for the device",
+},{
+.name = "aio",
+.type = QEMU_OPT_STRING,
+.help = "host AIO implementation (threads, native)",
+},{
+.name = "read-only",
+.type = QEMU_OPT_BOOL,
+.help = "open drive file as read-only",
+},{
+.name = "throttling.iops-total",
+.type = QEMU_OPT_NUMBER,
+.help = "limit total I/O operations per second",
+},{
+.name = "throttling.iops-read",
+.type = QEMU_OPT_NUMBER,
+.help = "limit read operations per second",
+},{
+.name = "throttling.iops-write",
+.type = QEMU_OPT_NUMBER,
+.help = "limit write operations per second",
+},{
+.name = "throttling.bps-total",
+.type = QEMU_OPT_NUMBER,
+.help = "limit total bytes per second",
+},{
+.name = "throttling.bps-read",
+.type = QEMU_OPT_NUMBER,
+.help = "limit read bytes per second",
+},{
+.name = "throttling.bps-write",
+ 

[Qemu-block] [PATCH v5 27/38] blockdev: Add blockdev-open-tray

[Max Reitz]

Signed-off-by: Max Reitz <mre...@redhat.com>
---
 blockdev.c   | 49 +
 qapi/block-core.json | 23 +++
 qmp-commands.hx  | 39 +++
 3 files changed, 111 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 647ce0b..6bc5841 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2059,6 +2059,55 @@ out:
 aio_context_release(aio_context);
 }
 
+void qmp_blockdev_open_tray(const char *device, bool has_force, bool force,
+Error **errp)
+{
+BlockBackend *blk;
+BlockDriverState *bs;
+AioContext *aio_context = NULL;
+
+if (!has_force) {
+force = false;
+}
+
+blk = blk_by_name(device);
+if (!blk) {
+error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+  "Device '%s' not found", device);
+return;
+}
+
+if (!blk_dev_has_removable_media(blk)) {
+error_setg(errp, "Device '%s' is not removable", device);
+return;
+}
+
+if (blk_dev_is_tray_open(blk)) {
+return;
+}
+
+bs = blk_bs(blk);
+if (bs) {
+aio_context = bdrv_get_aio_context(bs);
+aio_context_acquire(aio_context);
+
+if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) {
+goto out;
+}
+}
+
+if (blk_dev_is_medium_locked(blk)) {
+blk_dev_eject_request(blk, force);
+} else {
+blk_dev_change_media_cb(blk, false);
+}
+
+out:
+if (aio_context) {
+aio_context_release(aio_context);
+}
+}
+
 /* throttling disk I/O limits */
 void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
int64_t bps_wr,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 425fdab..b9b4a24 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1876,6 +1876,29 @@
 ##
 { 'command': 'blockdev-add', 'data': { 'options': 'BlockdevOptions' } }
 
+##
+# @blockdev-open-tray:
+#
+# Opens a block device's tray. If there is a block driver state tree inserted 
as
+# a medium, it will become inaccessible to the guest (but it will remain
+# associated to the block device, so closing the tray will make it accessible
+# again).
+#
+# If the tray was already open before, this will be a no-op.
+#
+# @device: block device name
+#
+# @force:  #optional if false (the default), an eject request will be sent to
+#  the guest if it has locked the tray (and the tray will not be opened
+#  immediately); if true, the tray will be opened regardless of whether
+#  it is locked
+#
+# Since: 2.5
+##
+{ 'command': 'blockdev-open-tray',
+  'data': { 'device': 'str',
+'*force': 'bool' } }
+
 
 ##
 # @BlockErrorAction
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 8343289..f6ea5ea 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3897,6 +3897,45 @@ Example (2):
 EQMP
 
 {
+.name   = "blockdev-open-tray",
+.args_type  = "device:s,force:b?",
+.mhandler.cmd_new = qmp_marshal_input_blockdev_open_tray,
+},
+
+SQMP
+blockdev-open-tray
+--
+
+Opens a block device's tray. If there is a block driver state tree inserted as 
a
+medium, it will become inaccessible to the guest (but it will remain associated
+to the block device, so closing the tray will make it accessible again).
+
+If the tray was already open before, this will be a no-op.
+
+Arguments:
+
+- "device": block device name (json-string)
+- "force": if false (the default), an eject request will be sent to the guest 
if
+   it has locked the tray (and the tray will not be opened 
immediately);
+   if true, the tray will be opened regardless of whether it is locked
+   (json-bool, optional)
+
+Example:
+
+-> { "execute": "blockdev-open-tray",
+ "arguments": { "device": "ide1-cd0" } }
+
+<- { "timestamp": { "seconds": 1418751016,
+"microseconds": 716996 },
+ "event": "DEVICE_TRAY_MOVED",
+ "data": { "device": "ide1-cd0",
+   "tray-open": true } }
+
+<- { "return": {} }
+
+EQMP
+
+{
 .name   = "query-named-block-nodes",
 .args_type  = "",
 .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
-- 
2.5.2

[Qemu-block] [PATCH v5 36/38] blockdev: read-only-mode for blockdev-change-medium

[Max Reitz]

Add an option to qmp_blockdev_change_medium() which allows changing the
read-only status of the block device whose medium is changed.

Some drives do not have a inherently fixed read-only status; for
instance, floppy disks can be set read-only or writable independently of
the drive. Some users may find it useful to be able to therefore change
the read-only status of a block device when changing the medium.

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
 blockdev.c   | 25 -
 hmp.c|  2 +-
 qapi/block-core.json | 24 +++-
 qmp-commands.hx  | 24 +++-
 qmp.c|  3 ++-
 5 files changed, 73 insertions(+), 5 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 2ef0745..4731843 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2104,6 +2104,8 @@ void qmp_blockdev_insert_medium(const char *device, const 
char *node_name,
 
 void qmp_blockdev_change_medium(const char *device, const char *filename,
 bool has_format, const char *format,
+bool has_read_only,
+BlockdevChangeReadOnlyMode read_only,
 Error **errp)
 {
 BlockBackend *blk;
@@ -2125,7 +2127,28 @@ void qmp_blockdev_change_medium(const char *device, 
const char *filename,
 }
 
 blk_rs = blk_get_root_state(blk);
-bdrv_flags = blk_rs->read_only ? 0 : BDRV_O_RDWR;
+
+if (!has_read_only) {
+read_only = BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN;
+}
+
+switch (read_only) {
+case BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN:
+bdrv_flags = blk_rs->read_only ? 0 : BDRV_O_RDWR;
+break;
+
+case BLOCKDEV_CHANGE_READ_ONLY_MODE_READ_ONLY:
+bdrv_flags = 0;
+break;
+
+case BLOCKDEV_CHANGE_READ_ONLY_MODE_READ_WRITE:
+bdrv_flags = BDRV_O_RDWR;
+break;
+
+default:
+abort();
+}
+
 bdrv_flags |= blk_rs->open_flags & ~BDRV_O_RDWR;
 
 if (has_format) {
diff --git a/hmp.c b/hmp.c
index a753a0f..5ea7ef2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1327,7 +1327,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
 }
 qmp_change("vnc", target, !!arg, arg, );
 } else {
-qmp_blockdev_change_medium(device, target, !!arg, arg, );
+qmp_blockdev_change_medium(device, target, !!arg, arg, false, 0, );
 if (err &&
 error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
 error_free(err);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b8cc18a..5f12af7 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1949,6 +1949,24 @@
 
 
 ##
+# @BlockdevChangeReadOnlyMode:
+#
+# Specifies the new read-only mode of a block device subject to the
+# @blockdev-change-medium command.
+#
+# @retain:  Retains the current read-only mode
+#
+# @read-only:   Makes the device read-only
+#
+# @read-write:  Makes the device writable
+#
+# Since: 2.3
+##
+{ 'enum': 'BlockdevChangeReadOnlyMode',
+  'data': ['retain', 'read-only', 'read-write'] }
+
+
+##
 # @blockdev-change-medium:
 #
 # Changes the medium inserted into a block device by ejecting the current 
medium
@@ -1963,12 +1981,16 @@
 # @format:  #optional, format to open the new image with (defaults to
 #   the probed format)
 #
+# @read-only-mode:  #optional, change the read-only mode of the device; 
defaults
+#   to 'retain'
+#
 # Since: 2.5
 ##
 { 'command': 'blockdev-change-medium',
   'data': { 'device': 'str',
 'filename': 'str',
-'*format': 'str' } }
+'*format': 'str',
+'*read-only-mode': 'BlockdevChangeReadOnlyMode' } }
 
 
 ##
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 538b3d1..495670b 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4116,7 +4116,7 @@ EQMP
 
 {
 .name   = "blockdev-change-medium",
-.args_type  = "device:B,filename:F,format:s?",
+.args_type  = "device:B,filename:F,format:s?,read-only-mode:s?",
 .mhandler.cmd_new = qmp_marshal_input_blockdev_change_medium,
 },
 
@@ -4132,6 +4132,8 @@ Arguments:
 - "device": device name (json-string)
 - "filename": filename of the new image (json-string)
 - "format": format of the new image (json-string, optional)
+- "read-only-mode": new read-only mode (json-string, optional)
+  - Possible values: "retain" (default), "read-only", "read-write"
 
 Examples:
 
@@ -4143,6 +4145,26 @@ Examples:
 "format": "raw" } }
 <- { "return": {} }
 
+2. Load a read-only medium into a writable drive
+
+-> { "execute": "blockdev-change-medium",
+ "arguments"

[Qemu-block] [PATCH v5 22/38] block: Prepare for NULL BDS

[Max Reitz]

blk_bs() will not necessarily return a non-NULL value any more (unless
blk_is_available() is true or it can be assumed to otherwise, e.g.
because it is called immediately after a successful blk_new_with_bs() or
blk_new_open()).

Signed-off-by: Max Reitz <mre...@redhat.com>
---
 block.c |   5 ++
 block/qapi.c|   4 +-
 blockdev.c  | 201 ++--
 hw/block/xen_disk.c |   4 +-
 migration/block.c   |   5 ++
 monitor.c   |   4 ++
 6 files changed, 153 insertions(+), 70 deletions(-)

diff --git a/block.c b/block.c
index 67a39fb..7776871 100644
--- a/block.c
+++ b/block.c
@@ -2787,6 +2787,11 @@ BlockDriverState *bdrv_lookup_bs(const char *device,
 blk = blk_by_name(device);
 
 if (blk) {
+if (!blk_bs(blk)) {
+error_setg(errp, "Device '%s' has no medium", device);
+return NULL;
+}
+
 return blk_bs(blk);
 }
 }
diff --git a/block/qapi.c b/block/qapi.c
index f295692..e936ba7 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -306,12 +306,12 @@ static void bdrv_query_info(BlockBackend *blk, BlockInfo 
**p_info,
 info->io_status = blk_iostatus(blk);
 }
 
-if (!QLIST_EMPTY(>dirty_bitmaps)) {
+if (bs && !QLIST_EMPTY(>dirty_bitmaps)) {
 info->has_dirty_bitmaps = true;
 info->dirty_bitmaps = bdrv_query_dirty_bitmaps(bs);
 }
 
-if (bs->drv) {
+if (bs && bs->drv) {
 info->has_inserted = true;
 info->inserted = bdrv_block_device_info(bs, errp);
 if (info->inserted == NULL) {
diff --git a/blockdev.c b/blockdev.c
index 01d422e..6168b09 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -124,14 +124,16 @@ void blockdev_mark_auto_del(BlockBackend *blk)
 return;
 }
 
-aio_context = bdrv_get_aio_context(bs);
-aio_context_acquire(aio_context);
+if (bs) {
+aio_context = bdrv_get_aio_context(bs);
+aio_context_acquire(aio_context);
 
-if (bs->job) {
-block_job_cancel(bs->job);
-}
+if (bs->job) {
+block_job_cancel(bs->job);
+}
 
-aio_context_release(aio_context);
+aio_context_release(aio_context);
+}
 
 dinfo->auto_del = 1;
 }
@@ -229,8 +231,8 @@ bool drive_check_orphaned(void)
 dinfo->type != IF_NONE) {
 fprintf(stderr, "Warning: Orphaned drive without device: "
 "id=%s,file=%s,if=%s,bus=%d,unit=%d\n",
-blk_name(blk), blk_bs(blk)->filename, if_name[dinfo->type],
-dinfo->bus, dinfo->unit);
+blk_name(blk), blk_bs(blk) ? blk_bs(blk)->filename : "",
+if_name[dinfo->type], dinfo->bus, dinfo->unit);
 rs = true;
 }
 }
@@ -1040,6 +1042,10 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
 monitor_printf(mon, "Device '%s' not found\n", device);
 return;
 }
+if (!blk_is_available(blk)) {
+monitor_printf(mon, "Device '%s' has no medium\n", device);
+return;
+}
 ret = bdrv_commit(blk_bs(blk));
 }
 if (ret < 0) {
@@ -1119,7 +1125,9 @@ SnapshotInfo 
*qmp_blockdev_snapshot_delete_internal_sync(const char *device,
   "Device '%s' not found", device);
 return NULL;
 }
-bs = blk_bs(blk);
+
+aio_context = blk_get_aio_context(blk);
+aio_context_acquire(aio_context);
 
 if (!has_id) {
 id = NULL;
@@ -1131,11 +1139,14 @@ SnapshotInfo 
*qmp_blockdev_snapshot_delete_internal_sync(const char *device,
 
 if (!id && !name) {
 error_setg(errp, "Name or id must be provided");
-return NULL;
+goto out_aio_context;
 }
 
-aio_context = bdrv_get_aio_context(bs);
-aio_context_acquire(aio_context);
+if (!blk_is_available(blk)) {
+error_setg(errp, "Device '%s' has no medium", device);
+goto out_aio_context;
+}
+bs = blk_bs(blk);
 
 if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT_DELETE, errp)) {
 goto out_aio_context;
@@ -1309,16 +1320,16 @@ static void 
internal_snapshot_prepare(BlkTransactionState *common,
   "Device '%s' not found", device);
 return;
 }
-bs = blk_bs(blk);
 
 /* AioContext is released in .clean() */
-state->aio_context = bdrv_get_aio_context(bs);
+state->aio_context = blk_get_aio_context(blk);
 aio_context_acquire(state->aio_context);
 
-if (!bdrv_is_inserted(bs)) {
+if (!blk_is_available(blk)) {
 error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
 return;
 }
+bs = blk_bs(blk);
 
 if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_INTERNAL_SNAPSHOT, errp)

[Qemu-block] [PATCH v5 26/38] block: Add blk_remove_bs()

[Max Reitz]

This function removes the BlockDriverState associated with the given
BlockBackend from that BB and sets the BDS pointer in the BB to NULL.

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
 block/block-backend.c  | 22 +-
 include/sysemu/block-backend.h |  1 +
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 652385e..66ecc07 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -314,6 +314,22 @@ void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk)
 }
 
 /*
+ * Disassociates the currently associated BlockDriverState from @blk.
+ */
+void blk_remove_bs(BlockBackend *blk)
+{
+if (!blk->bs) {
+return;
+}
+
+blk_update_root_state(blk);
+
+bdrv_unref(blk->bs);
+blk->bs->blk = NULL;
+blk->bs = NULL;
+}
+
+/*
  * Associates a new BlockDriverState with @blk.
  */
 void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
@@ -323,9 +339,13 @@ void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
 }
 
 assert(!blk->bs);
-assert(!bs->blk);
 bdrv_ref(bs);
 blk->bs = bs;
+
+if (bs->blk) {
+blk_remove_bs(bs->blk);
+}
+
 bs->blk = blk;
 }
 
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 9306a52..14a6d32 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -72,6 +72,7 @@ BlockBackend *blk_by_name(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
 
 BlockDriverState *blk_bs(BlockBackend *blk);
+void blk_remove_bs(BlockBackend *blk);
 void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs);
 
 void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk);
-- 
2.5.2

[Qemu-block] [PATCH v5 33/38] block: Inquire tray state before tray-moved events

[Max Reitz]

blk_dev_change_media_cb() is called for all potential tray movements;
however, it is possible to request closing the tray but nothing actually
happening (on a floppy disk drive without a medium).

Thus, the actual tray status should be inquired before sending a
tray-moved event (and an event should be sent whenever the status
changed).

Checking @load is now superfluous; it was necessary because it was
possible to change a medium without having explicitly opened the tray
and closed it again (or it might have been possible, at least). This is
no longer possible, though.

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
 block/block-backend.c | 17 +++--
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 66ecc07..09efb8b 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -422,18 +422,15 @@ void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps 
*ops,
 void blk_dev_change_media_cb(BlockBackend *blk, bool load)
 {
 if (blk->dev_ops && blk->dev_ops->change_media_cb) {
-bool tray_was_closed = !blk_dev_is_tray_open(blk);
+bool tray_was_open, tray_is_open;
 
+tray_was_open = blk_dev_is_tray_open(blk);
 blk->dev_ops->change_media_cb(blk->dev_opaque, load);
-if (tray_was_closed) {
-/* tray open */
-qapi_event_send_device_tray_moved(blk_name(blk),
-  true, _abort);
-}
-if (load) {
-/* tray close */
-qapi_event_send_device_tray_moved(blk_name(blk),
-  false, _abort);
+tray_is_open = blk_dev_is_tray_open(blk);
+
+if (tray_was_open != tray_is_open) {
+qapi_event_send_device_tray_moved(blk_name(blk), tray_is_open,
+  _abort);
 }
 }
 }
-- 
2.5.2

Re: [Qemu-block] [PATCH] throttle: test that snapshots move the throttling configuration

[Max Reitz]

On 17.09.2015 16:33, Alberto Garcia wrote:
> If a snapshot is performed on a device that has I/O limits they should
> be moved to the target image (the new active layer).
> 
> Signed-off-by: Alberto Garcia <be...@igalia.com>
> ---
>  tests/qemu-iotests/096 | 69 
> ++
>  tests/qemu-iotests/096.out |  5 
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 75 insertions(+)
>  create mode 100644 tests/qemu-iotests/096
>  create mode 100644 tests/qemu-iotests/096.out

Looks good, I'd just like to throw in that 096 is in use by my
looks-dead-but-actually-is-not and
only-waiting-for-the-blockbackend-and-media-series-to-get-merged series
"block: Rework bdrv_close_all()":
http://lists.nongnu.org/archive/html/qemu-block/2015-03/msg00048.html

But then again, once the prerequisites are met, I'll have to send a new
version of that series anyway...

So, since 096 is not a magic number I'm extremely keen on keeping in my
greedy claws:

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v5 29/38] blockdev: Add blockdev-remove-medium

[Max Reitz]

Signed-off-by: Max Reitz <mre...@redhat.com>
---
 blockdev.c   | 30 ++
 qapi/block-core.json | 15 +++
 qmp-commands.hx  | 45 +
 3 files changed, 90 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index d07bf8a..b7835e4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2131,6 +2131,36 @@ void qmp_blockdev_close_tray(const char *device, Error 
**errp)
 blk_dev_change_media_cb(blk, true);
 }
 
+void qmp_blockdev_remove_medium(const char *device, Error **errp)
+{
+BlockBackend *blk;
+bool has_device;
+
+blk = blk_by_name(device);
+if (!blk) {
+error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+  "Device '%s' not found", device);
+return;
+}
+
+/* For BBs without a device, we can exchange the BDS tree at will */
+has_device = blk_get_attached_dev(blk);
+
+if (has_device && !blk_dev_has_removable_media(blk)) {
+error_setg(errp, "Device '%s' is not removable", device);
+return;
+}
+
+if (has_device && !blk_dev_is_tray_open(blk)) {
+error_setg(errp, "Tray of device '%s' is not open", device);
+return;
+}
+
+if (blk_bs(blk)) {
+blk_remove_bs(blk);
+}
+}
+
 /* throttling disk I/O limits */
 void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
int64_t bps_wr,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1a51829..8edf5d9 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1915,6 +1915,21 @@
 { 'command': 'blockdev-close-tray',
   'data': { 'device': 'str' } }
 
+##
+# @blockdev-remove-medium:
+#
+# Removes a medium (a block driver state tree) from a block device. That block
+# device's tray must currently be open.
+#
+# If the tray is open and there is no medium inserted, this will be a no-op.
+#
+# @device: block device name
+#
+# Since: 2.5
+##
+{ 'command': 'blockdev-remove-medium',
+  'data': { 'device': 'str' } }
+
 
 ##
 # @BlockErrorAction
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 7c7125a..a9594a7 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3971,6 +3971,51 @@ Example:
 EQMP
 
 {
+.name   = "blockdev-remove-medium",
+.args_type  = "device:s",
+.mhandler.cmd_new = qmp_marshal_input_blockdev_remove_medium,
+},
+
+SQMP
+blockdev-remove-medium
+--
+
+Removes a medium (a block driver state tree) from a block device. That block
+device's tray must currently be open.
+
+If the tray is open and there is no medium inserted, this will be a no-op.
+
+Arguments:
+
+- "device": block device name (json-string)
+
+Example:
+
+-> { "execute": "blockdev-remove-medium",
+ "arguments": { "device": "ide1-cd0" } }
+
+<- { "error": { "class": "GenericError",
+"desc": "Tray of device 'ide1-cd0' is not open" } }
+
+-> { "execute": "blockdev-open-tray",
+ "arguments": { "device": "ide1-cd0" } }
+
+<- { "timestamp": { "seconds": 1418751627,
+"microseconds": 549958 },
+ "event": "DEVICE_TRAY_MOVED",
+ "data": { "device": "ide1-cd0",
+   "tray-open": true } }
+
+<- { "return": {} }
+
+-> { "execute": "blockdev-remove-medium",
+ "arguments": { "device": "ide1-cd0" } }
+
+<- { "return": {} }
+
+EQMP
+
+{
 .name   = "query-named-block-nodes",
 .args_type  = "",
 .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
-- 
2.5.2

[Qemu-block] [PATCH v5 35/38] hmp: Use blockdev-change-medium for change command

[Max Reitz]

Use separate code paths for the two overloaded functions of the 'change'
HMP command, and invoke the 'blockdev-change-medium' QMP command if used
on a block device (by calling qmp_blockdev_change_medium()).

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
 hmp.c | 27 +++
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/hmp.c b/hmp.c
index 3f807b7..a753a0f 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1317,22 +1317,25 @@ void hmp_change(Monitor *mon, const QDict *qdict)
 const char *arg = qdict_get_try_str(qdict, "arg");
 Error *err = NULL;
 
-if (strcmp(device, "vnc") == 0 &&
-(strcmp(target, "passwd") == 0 ||
- strcmp(target, "password") == 0)) {
-if (!arg) {
-monitor_read_password(mon, hmp_change_read_arg, NULL);
+if (strcmp(device, "vnc") == 0) {
+if (strcmp(target, "passwd") == 0 ||
+strcmp(target, "password") == 0) {
+if (!arg) {
+monitor_read_password(mon, hmp_change_read_arg, NULL);
+return;
+}
+}
+qmp_change("vnc", target, !!arg, arg, );
+} else {
+qmp_blockdev_change_medium(device, target, !!arg, arg, );
+if (err &&
+error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
+error_free(err);
+monitor_read_block_device_key(mon, device, NULL, NULL);
 return;
 }
 }
 
-qmp_change(device, target, !!arg, arg, );
-if (err &&
-error_get_class(err) == ERROR_CLASS_DEVICE_ENCRYPTED) {
-error_free(err);
-monitor_read_block_device_key(mon, device, NULL, NULL);
-return;
-}
 hmp_handle_error(mon, );
 }
 
-- 
2.5.2

[Qemu-block] [PATCH v5 34/38] qmp: Introduce blockdev-change-medium

[Max Reitz]

Introduce a new QMP command 'blockdev-change-medium' which is intended
to replace the 'change' command for block devices. The existing function
qmp_change_blockdev() is accordingly renamed to
qmp_blockdev_change_medium().

Signed-off-by: Max Reitz <mre...@redhat.com>
---
 blockdev.c|  7 ---
 include/sysemu/blockdev.h |  2 --
 qapi-schema.json  |  6 --
 qapi/block-core.json  | 23 +++
 qmp-commands.hx   | 31 +++
 qmp.c |  2 +-
 6 files changed, 63 insertions(+), 8 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 3882033..2ef0745 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2102,8 +2102,9 @@ void qmp_blockdev_insert_medium(const char *device, const 
char *node_name,
 qmp_blockdev_insert_anon_medium(device, bs, errp);
 }
 
-void qmp_change_blockdev(const char *device, const char *filename,
- const char *format, Error **errp)
+void qmp_blockdev_change_medium(const char *device, const char *filename,
+bool has_format, const char *format,
+Error **errp)
 {
 BlockBackend *blk;
 BlockBackendRootState *blk_rs;
@@ -2127,7 +2128,7 @@ void qmp_change_blockdev(const char *device, const char 
*filename,
 bdrv_flags = blk_rs->read_only ? 0 : BDRV_O_RDWR;
 bdrv_flags |= blk_rs->open_flags & ~BDRV_O_RDWR;
 
-if (format) {
+if (has_format) {
 options = qdict_new();
 qdict_put(options, "driver", qstring_from_str(format));
 }
diff --git a/include/sysemu/blockdev.h b/include/sysemu/blockdev.h
index 3104150..553b53c 100644
--- a/include/sysemu/blockdev.h
+++ b/include/sysemu/blockdev.h
@@ -63,8 +63,6 @@ DriveInfo *drive_new(QemuOpts *arg, BlockInterfaceType 
block_default_type);
 
 /* device-hotplug */
 
-void qmp_change_blockdev(const char *device, const char *filename,
- const char *format, Error **errp);
 void hmp_commit(Monitor *mon, const QDict *qdict);
 void hmp_drive_del(Monitor *mon, const QDict *qdict);
 #endif
diff --git a/qapi-schema.json b/qapi-schema.json
index 2bada60..0d5190b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1809,8 +1809,10 @@
 #  device's password.  The behavior of reads and writes to the block
 #  device between when these calls are executed is undefined.
 #
-# Notes:  It is strongly recommended that this interface is not used especially
-# for changing block devices.
+# Notes:  This interface is deprecated, and it is strongly recommended that you
+# avoid using it.  For changing block devices, use
+# blockdev-change-medium; for changing VNC parameters, use
+# change-vnc-password.
 #
 # Since: 0.14.0
 ##
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 81a1f19..b8cc18a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1949,6 +1949,29 @@
 
 
 ##
+# @blockdev-change-medium:
+#
+# Changes the medium inserted into a block device by ejecting the current 
medium
+# and loading a new image file which is inserted as the new medium (this 
command
+# combines blockdev-open-tray, blockdev-remove-medium, blockdev-insert-medium
+# and blockdev-close-tray).
+#
+# @device:  block device name
+#
+# @filename:filename of the new image to be loaded
+#
+# @format:  #optional, format to open the new image with (defaults to
+#   the probed format)
+#
+# Since: 2.5
+##
+{ 'command': 'blockdev-change-medium',
+  'data': { 'device': 'str',
+'filename': 'str',
+'*format': 'str' } }
+
+
+##
 # @BlockErrorAction
 #
 # An enumeration of action that has been taken when a DISK I/O occurs
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 38a0f5f..538b3d1 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4115,6 +4115,37 @@ Example:
 EQMP
 
 {
+.name   = "blockdev-change-medium",
+.args_type  = "device:B,filename:F,format:s?",
+.mhandler.cmd_new = qmp_marshal_input_blockdev_change_medium,
+},
+
+SQMP
+blockdev-change-medium
+--
+
+Changes the medium inserted into a block device by ejecting the current medium
+and loading a new image file which is inserted as the new medium.
+
+Arguments:
+
+- "device": device name (json-string)
+- "filename": filename of the new image (json-string)
+- "format": format of the new image (json-string, optional)
+
+Examples:
+
+1. Change a removable medium
+
+-> { "execute": "blockdev-change-medium",
+ "arguments": { "device": "ide1-cd0",
+"filename": "/srv/images/Fedora-12-x86_64-DVD.iso",
+"format": "raw" } }
+<- { "return": {} }
+
+EQMP
+
+{
 .name 

[Qemu-block] [PATCH v5 38/38] iotests: Add test for change-related QMP commands

[Max Reitz]

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
 tests/qemu-iotests/118 | 638 +
 tests/qemu-iotests/118.out |   5 +
 tests/qemu-iotests/group   |   1 +
 3 files changed, 644 insertions(+)
 create mode 100755 tests/qemu-iotests/118
 create mode 100644 tests/qemu-iotests/118.out

diff --git a/tests/qemu-iotests/118 b/tests/qemu-iotests/118
new file mode 100755
index 000..915e439
--- /dev/null
+++ b/tests/qemu-iotests/118
@@ -0,0 +1,638 @@
+#!/usr/bin/env python
+#
+# Test case for the QMP 'change' command and all other associated
+# commands
+#
+# 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/>.
+#
+
+import os
+import stat
+import time
+import iotests
+from iotests import qemu_img
+
+old_img = os.path.join(iotests.test_dir, 'test0.img')
+new_img = os.path.join(iotests.test_dir, 'test1.img')
+
+class ChangeBaseClass(iotests.QMPTestCase):
+has_opened = False
+has_closed = False
+
+def process_events(self):
+for event in self.vm.get_qmp_events(wait=False):
+if (event['event'] == 'DEVICE_TRAY_MOVED' and
+event['data']['device'] == 'drive0'):
+if event['data']['tray-open'] == False:
+self.has_closed = True
+else:
+self.has_opened = True
+
+def wait_for_open(self):
+timeout = time.clock() + 3
+while not self.has_opened and time.clock() < timeout:
+self.process_events()
+if not self.has_opened:
+self.fail('Timeout while waiting for the tray to open')
+
+def wait_for_close(self):
+timeout = time.clock() + 3
+while not self.has_closed and time.clock() < timeout:
+self.process_events()
+if not self.has_opened:
+self.fail('Timeout while waiting for the tray to close')
+
+class GeneralChangeTestsBaseClass(ChangeBaseClass):
+def test_change(self):
+result = self.vm.qmp('change', device='drive0', target=new_img,
+   arg=iotests.imgfmt)
+self.assert_qmp(result, 'return', {})
+
+self.wait_for_open()
+self.wait_for_close()
+
+result = self.vm.qmp('query-block')
+self.assert_qmp(result, 'return[0]/tray_open', False)
+self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+def test_blockdev_change_medium(self):
+result = self.vm.qmp('blockdev-change-medium', device='drive0',
+   filename=new_img,
+   format=iotests.imgfmt)
+self.assert_qmp(result, 'return', {})
+
+self.wait_for_open()
+self.wait_for_close()
+
+result = self.vm.qmp('query-block')
+self.assert_qmp(result, 'return[0]/tray_open', False)
+self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+def test_eject(self):
+result = self.vm.qmp('eject', device='drive0', force=True)
+self.assert_qmp(result, 'return', {})
+
+self.wait_for_open()
+
+result = self.vm.qmp('query-block')
+self.assert_qmp(result, 'return[0]/tray_open', True)
+self.assert_qmp_absent(result, 'return[0]/inserted')
+
+def test_tray_eject_change(self):
+result = self.vm.qmp('eject', device='drive0', force=True)
+self.assert_qmp(result, 'return', {})
+
+self.wait_for_open()
+
+result = self.vm.qmp('query-block')
+self.assert_qmp(result, 'return[0]/tray_open', True)
+self.assert_qmp_absent(result, 'return[0]/inserted')
+
+result = self.vm.qmp('blockdev-change-medium', device='drive0',
+   filename=new_img,
+   format=iotests.imgfmt)
+self.assert_qmp(result, 'return', {})
+
+self.wait_for_close()
+
+result = self.vm.qmp('query-block')
+self.assert_qmp(result, 'return[0]/tray_open', False)
+self.assert_qmp(result, 'return[0]/inserted/image/filename', new_img)
+
+def test_tray_open_close(self):
+result = self.vm.qmp('blockdev-open-tray', device='drive0', force=True)
+self.as

[Qemu-block] [PATCH v5 30/38] blockdev: Add blockdev-insert-medium

[Max Reitz]

And a helper function for that, which directly takes a pointer to the
BDS to be inserted instead of its node-name (which will be used for
implementing 'change' using blockdev-insert-medium).

Signed-off-by: Max Reitz <mre...@redhat.com>
---
 blockdev.c   | 48 
 qapi/block-core.json | 17 +
 qmp-commands.hx  | 37 +
 3 files changed, 102 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index b7835e4..bbcea00 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2161,6 +2161,54 @@ void qmp_blockdev_remove_medium(const char *device, 
Error **errp)
 }
 }
 
+static void qmp_blockdev_insert_anon_medium(const char *device,
+BlockDriverState *bs, Error **errp)
+{
+BlockBackend *blk;
+bool has_device;
+
+blk = blk_by_name(device);
+if (!blk) {
+error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+  "Device '%s' not found", device);
+return;
+}
+
+/* For BBs without a device, we can exchange the BDS tree at will */
+has_device = blk_get_attached_dev(blk);
+
+if (has_device && !blk_dev_has_removable_media(blk)) {
+error_setg(errp, "Device '%s' is not removable", device);
+return;
+}
+
+if (has_device && !blk_dev_is_tray_open(blk)) {
+error_setg(errp, "Tray of device '%s' is not open", device);
+return;
+}
+
+if (blk_bs(blk)) {
+error_setg(errp, "There already is a medium in device '%s'", device);
+return;
+}
+
+blk_insert_bs(blk, bs);
+}
+
+void qmp_blockdev_insert_medium(const char *device, const char *node_name,
+Error **errp)
+{
+BlockDriverState *bs;
+
+bs = bdrv_find_node(node_name);
+if (!bs) {
+error_setg(errp, "Node '%s' not found", node_name);
+return;
+}
+
+qmp_blockdev_insert_anon_medium(device, bs, errp);
+}
+
 /* throttling disk I/O limits */
 void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
int64_t bps_wr,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8edf5d9..81a1f19 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1930,6 +1930,23 @@
 { 'command': 'blockdev-remove-medium',
   'data': { 'device': 'str' } }
 
+##
+# @blockdev-insert-medium:
+#
+# Inserts a medium (a block driver state tree) into a block device. That block
+# device's tray must currently be open and there must be no medium inserted
+# already.
+#
+# @device:block device name
+#
+# @node-name: name of a node in the block driver state graph
+#
+# Since: 2.5
+##
+{ 'command': 'blockdev-insert-medium',
+  'data': { 'device': 'str',
+'node-name': 'str'} }
+
 
 ##
 # @BlockErrorAction
diff --git a/qmp-commands.hx b/qmp-commands.hx
index a9594a7..38a0f5f 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -4016,6 +4016,43 @@ Example:
 EQMP
 
 {
+.name   = "blockdev-insert-medium",
+.args_type  = "device:s,node-name:s",
+.mhandler.cmd_new = qmp_marshal_input_blockdev_insert_medium,
+},
+
+SQMP
+blockdev-insert-medium
+--
+
+Inserts a medium (a block driver state tree) into a block device. That block
+device's tray must currently be open and there must be no medium inserted
+already.
+
+Arguments:
+
+- "device": block device name (json-string)
+- "node-name": root node of the BDS tree to insert into the block device
+
+Example:
+
+-> { "execute": "blockdev-add",
+ "arguments": { "options": { "node-name": "node0",
+ "driver": "raw",
+ "file": { "driver": "file",
+   "filename": "fedora.iso" } } } }
+
+<- { "return": {} }
+
+-> { "execute": "blockdev-insert-medium",
+ "arguments": { "device": "ide1-cd0",
+"node-name": "node0" } }
+
+<- { "return": {} }
+
+EQMP
+
+{
 .name   = "query-named-block-nodes",
 .args_type  = "",
 .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
-- 
2.5.2

[Qemu-block] [PATCH v5 03/38] blockdev: Allow creation of BDS trees without BB

[Max Reitz]

If the "id" field is missing from the options given to blockdev-add,
just omit the BlockBackend and create the BlockDriverState tree alone.

However, if "id" is missing, "node-name" must be specified; otherwise,
the BDS tree would no longer be accessible.

Many BDS options which are not parsed by bdrv_open() (like caching)
cannot be specified for these BB-less BDS trees yet. A future patch will
remove this limitation.

Signed-off-by: Max Reitz <mre...@redhat.com>
---
 blockdev.c | 44 +++-
 qapi/block-core.json   | 13 +
 tests/qemu-iotests/087 |  2 +-
 tests/qemu-iotests/087.out |  4 ++--
 4 files changed, 43 insertions(+), 20 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index ab6eaea..013583b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3028,17 +3028,12 @@ out:
 void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
 {
 QmpOutputVisitor *ov = qmp_output_visitor_new();
-BlockBackend *blk;
+BlockDriverState *bs;
+BlockBackend *blk = NULL;
 QObject *obj;
 QDict *qdict;
 Error *local_err = NULL;
 
-/* Require an ID in the top level */
-if (!options->has_id) {
-error_setg(errp, "Block device needs an ID");
-goto fail;
-}
-
 /* TODO Sort it out in raw-posix and drive_new(): Reject aio=native with
  * cache.direct=false instead of silently switching to aio=threads, except
  * when called from drive_new().
@@ -3066,14 +3061,37 @@ void qmp_blockdev_add(BlockdevOptions *options, Error 
**errp)
 
 qdict_flatten(qdict);
 
-blk = blockdev_init(NULL, qdict, _err);
-if (local_err) {
-error_propagate(errp, local_err);
-goto fail;
+if (options->has_id) {
+blk = blockdev_init(NULL, qdict, _err);
+if (local_err) {
+error_propagate(errp, local_err);
+goto fail;
+}
+
+bs = blk_bs(blk);
+} else {
+int ret;
+
+if (!qdict_get_try_str(qdict, "node-name")) {
+error_setg(errp, "'id' and/or 'node-name' need to be specified for 
"
+   "the root node");
+goto fail;
+}
+
+bs = NULL;
+ret = bdrv_open(, NULL, NULL, qdict, BDRV_O_RDWR | BDRV_O_CACHE_WB,
+errp);
+if (ret < 0) {
+goto fail;
+}
 }
 
-if (bdrv_key_required(blk_bs(blk))) {
-blk_unref(blk);
+if (bs && bdrv_key_required(bs)) {
+if (blk) {
+blk_unref(blk);
+} else {
+bdrv_unref(bs);
+}
 error_setg(errp, "blockdev-add doesn't support encrypted devices");
 goto fail;
 }
diff --git a/qapi/block-core.json b/qapi/block-core.json
index c042561..425fdab 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1393,9 +1393,12 @@
 #
 # @driver:block driver name
 # @id:#optional id by which the new block device can be referred 
to.
-# This is a required option on the top level of blockdev-add, 
and
-# currently not allowed on any other level.
-# @node-name: #optional the name of a block driver state node (Since 2.0)
+# This option is only allowed on the top level of blockdev-add.
+# A BlockBackend will be created by blockdev-add if and only if
+# this option is given.
+# @node-name: #optional the name of a block driver state node (Since 2.0).
+# This option is required on the top level of blockdev-add if
+# the @id option is not given there.
 # @discard:   #optional discard-related options (default: ignore)
 # @cache: #optional cache-related options
 # @aio:   #optional AIO backend (default: threads)
@@ -1859,7 +1862,9 @@
 ##
 # @blockdev-add:
 #
-# Creates a new block device.
+# Creates a new block device. If the @id option is given at the top level, a
+# BlockBackend will be created; otherwise, @node-name is mandatory at the top
+# level and no BlockBackend will be created.
 #
 # This command is still a work in progress.  It doesn't support all
 # block drivers, it lacks a matching blockdev-del, and more.  Stay
diff --git a/tests/qemu-iotests/087 b/tests/qemu-iotests/087
index 8694749..af44299 100755
--- a/tests/qemu-iotests/087
+++ b/tests/qemu-iotests/087
@@ -54,7 +54,7 @@ size=128M
 _make_test_img $size
 
 echo
-echo === Missing ID ===
+echo === Missing ID and node-name ===
 echo
 
 run_qemu <

Re: [Qemu-block] [PATCH v5 4/4] block: add tests for the 'blockdev-snapshot' command

[Max Reitz]

On 18.09.2015 15:16, Alberto Garcia wrote:
> Signed-off-by: Alberto Garcia <be...@igalia.com>
> ---
>  tests/qemu-iotests/085 | 102 
> ++---
>  tests/qemu-iotests/085.out |  34 ++-
>  2 files changed, 128 insertions(+), 8 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH v5 23/38] blockdev: Do not create BDS for empty drive

[Max Reitz]

Do not use "rudimentary" BDSs for empty drives any longer (for
freshly created drives).

After a follow-up patch, empty drives will generally use a NULL BDS, not
only the freshly created drives.

Signed-off-by: Max Reitz <mre...@redhat.com>
---
 blockdev.c | 72 ++
 1 file changed, 44 insertions(+), 28 deletions(-)

diff --git a/blockdev.c b/blockdev.c
index 6168b09..e76a7de 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -514,16 +514,44 @@ static BlockBackend *blockdev_init(const char *file, 
QDict *bs_opts,
 goto early_err;
 }
 
+if (snapshot) {
+/* always use cache=unsafe with snapshot */
+bdrv_flags &= ~BDRV_O_CACHE_MASK;
+bdrv_flags |= (BDRV_O_SNAPSHOT|BDRV_O_CACHE_WB|BDRV_O_NO_FLUSH);
+}
+
+if (copy_on_read) {
+bdrv_flags |= BDRV_O_COPY_ON_READ;
+}
+
+if (runstate_check(RUN_STATE_INMIGRATE)) {
+bdrv_flags |= BDRV_O_INCOMING;
+}
+
+bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
+
 /* init */
 if ((!file || !*file) && !has_driver_specific_opts) {
-blk = blk_new_with_bs(qemu_opts_id(opts), errp);
+BlockBackendRootState *blk_rs;
+
+blk = blk_new(qemu_opts_id(opts), errp);
 if (!blk) {
 goto early_err;
 }
 
-bs = blk_bs(blk);
-bs->open_flags = snapshot ? BDRV_O_SNAPSHOT : 0;
-bs->read_only = ro;
+blk_rs = blk_get_root_state(blk);
+blk_rs->open_flags= bdrv_flags;
+blk_rs->read_only = ro;
+blk_rs->detect_zeroes = detect_zeroes;
+
+if (throttle_enabled()) {
+if (!throttling_group) {
+throttling_group = blk_name(blk);
+}
+blk_rs->io_limits_enabled = true;
+blk_rs->throttle_config = cfg;
+blk_rs->throttle_group_name = g_strdup(throttling_group);
+}
 
 QDECREF(bs_opts);
 } else {
@@ -531,42 +559,30 @@ static BlockBackend *blockdev_init(const char *file, 
QDict *bs_opts,
 file = NULL;
 }
 
-if (snapshot) {
-/* always use cache=unsafe with snapshot */
-bdrv_flags &= ~BDRV_O_CACHE_MASK;
-bdrv_flags |= (BDRV_O_SNAPSHOT|BDRV_O_CACHE_WB|BDRV_O_NO_FLUSH);
-}
-
-if (copy_on_read) {
-bdrv_flags |= BDRV_O_COPY_ON_READ;
-}
-
-bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
-
 blk = blk_new_open(qemu_opts_id(opts), file, NULL, bs_opts, bdrv_flags,
errp);
 if (!blk) {
 goto err_no_bs_opts;
 }
 bs = blk_bs(blk);
-}
 
-bs->detect_zeroes = detect_zeroes;
+bs->detect_zeroes = detect_zeroes;
 
-blk_set_on_error(blk, on_read_error, on_write_error);
+/* disk I/O throttling */
+if (throttle_enabled()) {
+if (!throttling_group) {
+throttling_group = blk_name(blk);
+}
+bdrv_io_limits_enable(bs, throttling_group);
+bdrv_set_io_limits(bs, );
+}
 
-/* disk I/O throttling */
-if (throttle_enabled()) {
-if (!throttling_group) {
-throttling_group = blk_name(blk);
+if (bdrv_key_required(bs)) {
+autostart = 0;
 }
-bdrv_io_limits_enable(bs, throttling_group);
-bdrv_set_io_limits(bs, );
 }
 
-if (bdrv_key_required(bs)) {
-autostart = 0;
-}
+blk_set_on_error(blk, on_read_error, on_write_error);
 
 err_no_bs_opts:
 qemu_opts_del(opts);
-- 
2.5.2

[Qemu-block] [PATCH v5 05/38] block: Make bdrv_is_inserted() return a bool

[Max Reitz]

Make bdrv_is_inserted(), blk_is_inserted(), and the callback
BlockDriver.bdrv_is_inserted() return a bool.

Suggested-by: Eric Blake <ebl...@redhat.com>
Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alberto Garcia <be...@igalia.com>
---
 block.c| 12 +++-
 block/block-backend.c  |  2 +-
 block/raw-posix.c  |  8 +++-
 block/raw_bsd.c|  2 +-
 include/block/block.h  |  2 +-
 include/block/block_int.h  |  2 +-
 include/sysemu/block-backend.h |  2 +-
 7 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/block.c b/block.c
index 51d8c51..4a089e6 100644
--- a/block.c
+++ b/block.c
@@ -3244,14 +3244,16 @@ void bdrv_invalidate_cache_all(Error **errp)
 /**
  * Return TRUE if the media is present
  */
-int bdrv_is_inserted(BlockDriverState *bs)
+bool bdrv_is_inserted(BlockDriverState *bs)
 {
 BlockDriver *drv = bs->drv;
 
-if (!drv)
-return 0;
-if (!drv->bdrv_is_inserted)
-return 1;
+if (!drv) {
+return false;
+}
+if (!drv->bdrv_is_inserted) {
+return true;
+}
 return drv->bdrv_is_inserted(bs);
 }
 
diff --git a/block/block-backend.c b/block/block-backend.c
index c2e8732..5ad2dd4 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -752,7 +752,7 @@ void blk_invalidate_cache(BlockBackend *blk, Error **errp)
 bdrv_invalidate_cache(blk->bs, errp);
 }
 
-int blk_is_inserted(BlockBackend *blk)
+bool blk_is_inserted(BlockBackend *blk)
 {
 return bdrv_is_inserted(blk->bs);
 }
diff --git a/block/raw-posix.c b/block/raw-posix.c
index e650c9b..d6e88e3 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -2389,15 +2389,13 @@ out:
 return prio;
 }
 
-static int cdrom_is_inserted(BlockDriverState *bs)
+static bool cdrom_is_inserted(BlockDriverState *bs)
 {
 BDRVRawState *s = bs->opaque;
 int ret;
 
 ret = ioctl(s->fd, CDROM_DRIVE_STATUS, CDSL_CURRENT);
-if (ret == CDS_DISC_OK)
-return 1;
-return 0;
+return ret == CDS_DISC_OK;
 }
 
 static void cdrom_eject(BlockDriverState *bs, bool eject_flag)
@@ -2523,7 +2521,7 @@ static int cdrom_reopen(BlockDriverState *bs)
 return 0;
 }
 
-static int cdrom_is_inserted(BlockDriverState *bs)
+static bool cdrom_is_inserted(BlockDriverState *bs)
 {
 return raw_getlength(bs) > 0;
 }
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index e3d2d04..d718583 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -154,7 +154,7 @@ static int raw_truncate(BlockDriverState *bs, int64_t 
offset)
 return bdrv_truncate(bs->file, offset);
 }
 
-static int raw_is_inserted(BlockDriverState *bs)
+static bool raw_is_inserted(BlockDriverState *bs)
 {
 return bdrv_is_inserted(bs->file);
 }
diff --git a/include/block/block.h b/include/block/block.h
index ef67353..c9cc62b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -397,7 +397,7 @@ int bdrv_is_read_only(BlockDriverState *bs);
 int bdrv_is_sg(BlockDriverState *bs);
 int bdrv_enable_write_cache(BlockDriverState *bs);
 void bdrv_set_enable_write_cache(BlockDriverState *bs, bool wce);
-int bdrv_is_inserted(BlockDriverState *bs);
+bool bdrv_is_inserted(BlockDriverState *bs);
 int bdrv_media_changed(BlockDriverState *bs);
 void bdrv_lock_medium(BlockDriverState *bs, bool locked);
 void bdrv_eject(BlockDriverState *bs, bool eject_flag);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 14ad4c3..f3b3354 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -213,7 +213,7 @@ struct BlockDriver {
 const char *backing_file, const char *backing_fmt);
 
 /* removable device specific */
-int (*bdrv_is_inserted)(BlockDriverState *bs);
+bool (*bdrv_is_inserted)(BlockDriverState *bs);
 int (*bdrv_media_changed)(BlockDriverState *bs);
 void (*bdrv_eject)(BlockDriverState *bs, bool eject_flag);
 void (*bdrv_lock_medium)(BlockDriverState *bs, bool locked);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 8fc960f..8f2bf10 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -130,7 +130,7 @@ int blk_is_sg(BlockBackend *blk);
 int blk_enable_write_cache(BlockBackend *blk);
 void blk_set_enable_write_cache(BlockBackend *blk, bool wce);
 void blk_invalidate_cache(BlockBackend *blk, Error **errp);
-int blk_is_inserted(BlockBackend *blk);
+bool blk_is_inserted(BlockBackend *blk);
 void blk_lock_medium(BlockBackend *blk, bool locked);
 void blk_eject(BlockBackend *blk, bool eject_flag);
 int blk_get_flags(BlockBackend *blk);
-- 
2.5.2

[Qemu-block] [PATCH v5 06/38] block: Add blk_is_available()

[Max Reitz]

blk_is_available() returns true iff the BDS is inserted (which means
blk_bs() is not NULL and bdrv_is_inserted() returns true) and if the
tray of the guest device is closed.

blk_is_inserted() is changed to return true only if blk_bs() is not
NULL.

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alberto Garcia <be...@igalia.com>
---
 block/block-backend.c  | 7 ++-
 include/sysemu/block-backend.h | 1 +
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 5ad2dd4..cf30cad 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -754,7 +754,12 @@ void blk_invalidate_cache(BlockBackend *blk, Error **errp)
 
 bool blk_is_inserted(BlockBackend *blk)
 {
-return bdrv_is_inserted(blk->bs);
+return blk->bs && bdrv_is_inserted(blk->bs);
+}
+
+bool blk_is_available(BlockBackend *blk)
+{
+return blk_is_inserted(blk) && !blk_dev_is_tray_open(blk);
 }
 
 void blk_lock_medium(BlockBackend *blk, bool locked)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 8f2bf10..1e19d1b 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -131,6 +131,7 @@ int blk_enable_write_cache(BlockBackend *blk);
 void blk_set_enable_write_cache(BlockBackend *blk, bool wce);
 void blk_invalidate_cache(BlockBackend *blk, Error **errp);
 bool blk_is_inserted(BlockBackend *blk);
+bool blk_is_available(BlockBackend *blk);
 void blk_lock_medium(BlockBackend *blk, bool locked);
 void blk_eject(BlockBackend *blk, bool eject_flag);
 int blk_get_flags(BlockBackend *blk);
-- 
2.5.2

[Qemu-block] [PATCH v5 04/38] iotests: Only create BB if necessary

[Max Reitz]

Tests 071 and 081 test giving references in blockdev-add. It is not
necessary to create a BlockBackend here, so omit it.

While at it, fix up some blockdev-add invocations in the vicinity
(s/raw/$IMGFMT/ in 081, drop the format BDS for blkverify's raw child in
071).

Signed-off-by: Max Reitz <mre...@redhat.com>
---
 tests/qemu-iotests/071 | 54 ++
 tests/qemu-iotests/071.out | 12 +++
 tests/qemu-iotests/081 | 18 +---
 tests/qemu-iotests/081.out |  5 +++--
 4 files changed, 71 insertions(+), 18 deletions(-)

diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
index 9eaa49b..92ab991 100755
--- a/tests/qemu-iotests/071
+++ b/tests/qemu-iotests/071
@@ -104,11 +104,20 @@ echo
 echo "=== Testing blkdebug on existing block device ==="
 echo
 
-run_qemu -drive "file=$TEST_IMG,format=raw,if=none,id=drive0" <

[Qemu-block] [PATCH v5 11/38] hw/usb-storage: Check whether BB is inserted

[Max Reitz]

Only call bdrv_add_key() on the BlockDriverState if it is not NULL.

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alberto Garcia <be...@igalia.com>
---
 hw/usb/dev-storage.c | 30 --
 1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c
index 9a4e7dc..597d8fd 100644
--- a/hw/usb/dev-storage.c
+++ b/hw/usb/dev-storage.c
@@ -613,20 +613,22 @@ static void usb_msd_realize_storage(USBDevice *dev, Error 
**errp)
 return;
 }
 
-bdrv_add_key(blk_bs(blk), NULL, );
-if (err) {
-if (monitor_cur_is_qmp()) {
-error_propagate(errp, err);
-return;
-}
-error_free(err);
-err = NULL;
-if (cur_mon) {
-monitor_read_bdrv_key_start(cur_mon, blk_bs(blk),
-usb_msd_password_cb, s);
-s->dev.auto_attach = 0;
-} else {
-autostart = 0;
+if (blk_bs(blk)) {
+bdrv_add_key(blk_bs(blk), NULL, );
+if (err) {
+if (monitor_cur_is_qmp()) {
+error_propagate(errp, err);
+return;
+}
+error_free(err);
+err = NULL;
+if (cur_mon) {
+monitor_read_bdrv_key_start(cur_mon, blk_bs(blk),
+usb_msd_password_cb, s);
+s->dev.auto_attach = 0;
+} else {
+autostart = 0;
+}
 }
 }
 
-- 
2.5.2

[Qemu-block] [PATCH v5 12/38] block: Fix BB AIOCB AioContext without BDS

[Max Reitz]

Fix the BlockBackend's AIOCB AioContext for aborting AIO in case there
is no BDS. If there is no implementation of AIOCBInfo::get_aio_context()
the AioContext is derived from the BDS the AIOCB belongs to. If that BDS
is NULL (because it has been removed from the BB) this will not work.

This patch makes blk_get_aio_context() fall back to the main loop
context if the BDS pointer is NULL and implements
AIOCBInfo::get_aio_context() (blk_aiocb_get_aio_context()) which invokes
blk_get_aio_context().

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alberto Garcia <be...@igalia.com>
---
 block/block-backend.c | 17 -
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index cf30cad..501130b 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -18,6 +18,8 @@
 /* Number of coroutines to reserve per attached device model */
 #define COROUTINE_POOL_RESERVATION 64
 
+static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb);
+
 struct BlockBackend {
 char *name;
 int refcnt;
@@ -34,10 +36,12 @@ struct BlockBackend {
 typedef struct BlockBackendAIOCB {
 BlockAIOCB common;
 QEMUBH *bh;
+BlockBackend *blk;
 int ret;
 } BlockBackendAIOCB;
 
 static const AIOCBInfo block_backend_aiocb_info = {
+.get_aio_context = blk_aiocb_get_aio_context,
 .aiocb_size = sizeof(BlockBackendAIOCB),
 };
 
@@ -541,6 +545,7 @@ static BlockAIOCB *abort_aio_request(BlockBackend *blk, 
BlockCompletionFunc *cb,
 QEMUBH *bh;
 
 acb = blk_aio_get(_backend_aiocb_info, blk, cb, opaque);
+acb->blk = blk;
 acb->ret = ret;
 
 bh = aio_bh_new(blk_get_aio_context(blk), error_callback_bh, acb);
@@ -814,7 +819,17 @@ void blk_op_unblock_all(BlockBackend *blk, Error *reason)
 
 AioContext *blk_get_aio_context(BlockBackend *blk)
 {
-return bdrv_get_aio_context(blk->bs);
+if (blk->bs) {
+return bdrv_get_aio_context(blk->bs);
+} else {
+return qemu_get_aio_context();
+}
+}
+
+static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb)
+{
+BlockBackendAIOCB *blk_acb = DO_UPCAST(BlockBackendAIOCB, common, acb);
+return blk_get_aio_context(blk_acb->blk);
 }
 
 void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
-- 
2.5.2

[Qemu-block] [PATCH v5 07/38] block: Make bdrv_is_inserted() recursive

[Max Reitz]

If bdrv_is_inserted() is called on the top level BDS, it should make
sure all nodes in the BDS tree are actually inserted.

Signed-off-by: Max Reitz <mre...@redhat.com>
---
 block.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 4a089e6..c4fa299 100644
--- a/block.c
+++ b/block.c
@@ -3247,14 +3247,20 @@ void bdrv_invalidate_cache_all(Error **errp)
 bool bdrv_is_inserted(BlockDriverState *bs)
 {
 BlockDriver *drv = bs->drv;
+BdrvChild *child;
 
 if (!drv) {
 return false;
 }
-if (!drv->bdrv_is_inserted) {
-return true;
+if (drv->bdrv_is_inserted) {
+return drv->bdrv_is_inserted(bs);
 }
-return drv->bdrv_is_inserted(bs);
+QLIST_FOREACH(child, >children, next) {
+if (!bdrv_is_inserted(child->bs)) {
+return false;
+}
+}
+return true;
 }
 
 /**
-- 
2.5.2

[Qemu-block] [PATCH v5 14/38] block: Remove wr_highest_sector from BlockAcctStats

[Max Reitz]

BlockAcctStats contains statistics about the data transferred from and
to the device; wr_highest_sector does not fit in with the rest.

Furthermore, those statistics are supposed to be specific for a certain
device and not necessarily for a BDS (see the comment above
bdrv_get_stats()); on the other hand, wr_highest_sector may be a rather
important information to know for each BDS. When BlockAcctStats is
finally removed from the BDS, we will want to keep wr_highest_sector in
the BDS.

Finally, wr_highest_sector is renamed to wr_highest_offset and given the
appropriate meaning. Externally, it is represented as an offset so there
is no point in doing something different internally. Its definition is
changed to match that in qapi/block-core.json which is "the offset after
the greatest byte written to". Doing so should not cause any harm since
if external programs tried to calculate the volume usage by
(wr_highest_offset + 512) / volume_size, after this patch they will just
assume the volume to be full slightly earlier than before.

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alberto Garcia <be...@igalia.com>
---
 block/accounting.c | 8 
 block/io.c | 4 +++-
 block/qapi.c   | 4 ++--
 include/block/accounting.h | 3 ---
 include/block/block_int.h  | 3 +++
 qmp-commands.hx| 4 ++--
 6 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/block/accounting.c b/block/accounting.c
index 01d594f..a423560 100644
--- a/block/accounting.c
+++ b/block/accounting.c
@@ -47,14 +47,6 @@ void block_acct_done(BlockAcctStats *stats, BlockAcctCookie 
*cookie)
 }
 
 
-void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num,
-   unsigned int nb_sectors)
-{
-if (stats->wr_highest_sector < sector_num + nb_sectors - 1) {
-stats->wr_highest_sector = sector_num + nb_sectors - 1;
-}
-}
-
 void block_acct_merge_done(BlockAcctStats *stats, enum BlockAcctType type,
   int num_requests)
 {
diff --git a/block/io.c b/block/io.c
index d4bc83b..21cc82a 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1141,7 +1141,9 @@ static int coroutine_fn 
bdrv_aligned_pwritev(BlockDriverState *bs,
 
 bdrv_set_dirty(bs, sector_num, nb_sectors);
 
-block_acct_highest_sector(>stats, sector_num, nb_sectors);
+if (bs->wr_highest_offset < offset + bytes) {
+bs->wr_highest_offset = offset + bytes;
+}
 
 if (ret >= 0) {
 bs->total_sectors = MAX(bs->total_sectors, sector_num + nb_sectors);
diff --git a/block/qapi.c b/block/qapi.c
index 2ce5097..d3cbc80 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -350,13 +350,13 @@ static BlockStats *bdrv_query_stats(const 
BlockDriverState *bs,
 s->stats->wr_operations = bs->stats.nr_ops[BLOCK_ACCT_WRITE];
 s->stats->rd_merged = bs->stats.merged[BLOCK_ACCT_READ];
 s->stats->wr_merged = bs->stats.merged[BLOCK_ACCT_WRITE];
-s->stats->wr_highest_offset =
-bs->stats.wr_highest_sector * BDRV_SECTOR_SIZE;
 s->stats->flush_operations = bs->stats.nr_ops[BLOCK_ACCT_FLUSH];
 s->stats->wr_total_time_ns = bs->stats.total_time_ns[BLOCK_ACCT_WRITE];
 s->stats->rd_total_time_ns = bs->stats.total_time_ns[BLOCK_ACCT_READ];
 s->stats->flush_total_time_ns = bs->stats.total_time_ns[BLOCK_ACCT_FLUSH];
 
+s->stats->wr_highest_offset = bs->wr_highest_offset;
+
 if (bs->file) {
 s->has_parent = true;
 s->parent = bdrv_query_stats(bs->file, query_backing);
diff --git a/include/block/accounting.h b/include/block/accounting.h
index 4c406cf..66637cd 100644
--- a/include/block/accounting.h
+++ b/include/block/accounting.h
@@ -40,7 +40,6 @@ typedef struct BlockAcctStats {
 uint64_t nr_ops[BLOCK_MAX_IOTYPE];
 uint64_t total_time_ns[BLOCK_MAX_IOTYPE];
 uint64_t merged[BLOCK_MAX_IOTYPE];
-uint64_t wr_highest_sector;
 } BlockAcctStats;
 
 typedef struct BlockAcctCookie {
@@ -52,8 +51,6 @@ typedef struct BlockAcctCookie {
 void block_acct_start(BlockAcctStats *stats, BlockAcctCookie *cookie,
   int64_t bytes, enum BlockAcctType type);
 void block_acct_done(BlockAcctStats *stats, BlockAcctCookie *cookie);
-void block_acct_highest_sector(BlockAcctStats *stats, int64_t sector_num,
-   unsigned int nb_sectors);
 void block_acct_merge_done(BlockAcctStats *stats, enum BlockAcctType type,
int num_requests);
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index b7e1e16..67e05ac 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -403,6 +403,9 @@ struct BlockDriverState {
 /* I/O stats (display with "info blockstats"). */
 BlockAcctStats stats;
 
+/* Offset after the highes

[Qemu-block] [PATCH v5 02/38] block: Set BDRV_O_INCOMING in bdrv_fill_options()

[Max Reitz]

This flag should not be set for the root BDS only, but for any BDS that
is being created while incoming migration is pending, so setting it is
moved from blockdev_init() to bdrv_fill_options().

Signed-off-by: Max Reitz <mre...@redhat.com>
---
 block.c| 4 
 blockdev.c | 4 
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 6268e37..51d8c51 100644
--- a/block.c
+++ b/block.c
@@ -1076,6 +1076,10 @@ static int bdrv_fill_options(QDict **options, const char 
**pfilename,
 }
 }
 
+if (runstate_check(RUN_STATE_INMIGRATE)) {
+*flags |= BDRV_O_INCOMING;
+}
+
 return 0;
 }
 
diff --git a/blockdev.c b/blockdev.c
index 32b04b4..ab6eaea 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -539,10 +539,6 @@ static BlockBackend *blockdev_init(const char *file, QDict 
*bs_opts,
 bdrv_flags |= BDRV_O_COPY_ON_READ;
 }
 
-if (runstate_check(RUN_STATE_INMIGRATE)) {
-bdrv_flags |= BDRV_O_INCOMING;
-}
-
 bdrv_flags |= ro ? 0 : BDRV_O_RDWR;
 
 blk = blk_new_open(qemu_opts_id(opts), file, NULL, bs_opts, bdrv_flags,
-- 
2.5.2

[Qemu-block] [PATCH v5 15/38] block: Move BlockAcctStats into BlockBackend

[Max Reitz]

As the comment above bdrv_get_stats() says, BlockAcctStats is something
which belongs to the device instead of each BlockDriverState. This patch
therefore moves it into the BlockBackend.

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alberto Garcia <be...@igalia.com>
---
 block.c   | 11 ---
 block/block-backend.c |  5 -
 block/io.c|  6 +-
 block/qapi.c  | 24 ++--
 include/block/block.h |  2 --
 include/block/block_int.h |  3 ---
 6 files changed, 23 insertions(+), 28 deletions(-)

diff --git a/block.c b/block.c
index 55357c4..f81ef23 100644
--- a/block.c
+++ b/block.c
@@ -4256,14 +4256,3 @@ void bdrv_refresh_filename(BlockDriverState *bs)
 QDECREF(json);
 }
 }
-
-/* This accessor function purpose is to allow the device models to access the
- * BlockAcctStats structure embedded inside a BlockDriverState without being
- * aware of the BlockDriverState structure layout.
- * It will go away when the BlockAcctStats structure will be moved inside
- * the device models.
- */
-BlockAcctStats *bdrv_get_stats(BlockDriverState *bs)
-{
-return >stats;
-}
diff --git a/block/block-backend.c b/block/block-backend.c
index ff862ad..310ca50 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -34,6 +34,9 @@ struct BlockBackend {
 
 /* the block size for which the guest device expects atomicity */
 int guest_block_size;
+
+/* I/O stats (display with "info blockstats"). */
+BlockAcctStats stats;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -875,7 +878,7 @@ void blk_io_unplug(BlockBackend *blk)
 
 BlockAcctStats *blk_get_stats(BlockBackend *blk)
 {
-return bdrv_get_stats(blk->bs);
+return >stats;
 }
 
 void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
diff --git a/block/io.c b/block/io.c
index 21cc82a..08b50c0 100644
--- a/block/io.c
+++ b/block/io.c
@@ -23,6 +23,7 @@
  */
 
 #include "trace.h"
+#include "sysemu/block-backend.h"
 #include "block/blockjob.h"
 #include "block/block_int.h"
 #include "block/throttle-groups.h"
@@ -1895,7 +1896,10 @@ static int multiwrite_merge(BlockDriverState *bs, 
BlockRequest *reqs,
 }
 }
 
-block_acct_merge_done(>stats, BLOCK_ACCT_WRITE, num_reqs - outidx - 1);
+if (bs->blk) {
+block_acct_merge_done(blk_get_stats(bs->blk), BLOCK_ACCT_WRITE,
+  num_reqs - outidx - 1);
+}
 
 return outidx + 1;
 }
diff --git a/block/qapi.c b/block/qapi.c
index d3cbc80..2d1ef87 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -344,16 +344,20 @@ static BlockStats *bdrv_query_stats(const 
BlockDriverState *bs,
 }
 
 s->stats = g_malloc0(sizeof(*s->stats));
-s->stats->rd_bytes = bs->stats.nr_bytes[BLOCK_ACCT_READ];
-s->stats->wr_bytes = bs->stats.nr_bytes[BLOCK_ACCT_WRITE];
-s->stats->rd_operations = bs->stats.nr_ops[BLOCK_ACCT_READ];
-s->stats->wr_operations = bs->stats.nr_ops[BLOCK_ACCT_WRITE];
-s->stats->rd_merged = bs->stats.merged[BLOCK_ACCT_READ];
-s->stats->wr_merged = bs->stats.merged[BLOCK_ACCT_WRITE];
-s->stats->flush_operations = bs->stats.nr_ops[BLOCK_ACCT_FLUSH];
-s->stats->wr_total_time_ns = bs->stats.total_time_ns[BLOCK_ACCT_WRITE];
-s->stats->rd_total_time_ns = bs->stats.total_time_ns[BLOCK_ACCT_READ];
-s->stats->flush_total_time_ns = bs->stats.total_time_ns[BLOCK_ACCT_FLUSH];
+if (bs->blk) {
+BlockAcctStats *stats = blk_get_stats(bs->blk);
+
+s->stats->rd_bytes = stats->nr_bytes[BLOCK_ACCT_READ];
+s->stats->wr_bytes = stats->nr_bytes[BLOCK_ACCT_WRITE];
+s->stats->rd_operations = stats->nr_ops[BLOCK_ACCT_READ];
+s->stats->wr_operations = stats->nr_ops[BLOCK_ACCT_WRITE];
+s->stats->rd_merged = stats->merged[BLOCK_ACCT_READ];
+s->stats->wr_merged = stats->merged[BLOCK_ACCT_WRITE];
+s->stats->flush_operations = stats->nr_ops[BLOCK_ACCT_FLUSH];
+s->stats->wr_total_time_ns = stats->total_time_ns[BLOCK_ACCT_WRITE];
+s->stats->rd_total_time_ns = stats->total_time_ns[BLOCK_ACCT_READ];
+s->stats->flush_total_time_ns = stats->total_time_ns[BLOCK_ACCT_FLUSH];
+}
 
 s->stats->wr_highest_offset = bs->wr_highest_offset;
 
diff --git a/include/block/block.h b/include/block/block.h
index 3e818f4..960eb98 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -613,6 +613,4 @@ void bdrv_io_plug(BlockDriverState *bs);
 void bdrv_io_unplug(BlockDriverState *bs);
 void bdrv_flush_io_queue(BlockDriverState *bs);
 
-BlockAcctStats *bdrv_get_stats(BlockDriverState *bs);
-
 #endif
diff --git a/include/bl

[Qemu-block] [PATCH v5 18/38] block: Make some BB functions fall back to BBRS

[Max Reitz]

If there is no BDS tree attached to a BlockBackend, functions that can
do so should fall back to the BlockBackendRootState structure (which are
blk_is_read_only() and blk_get_flags(), because the read-only status and
the "open flags" are part of the BBRS).

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
 block/block-backend.c | 12 ++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 9590be5..417374f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -852,7 +852,11 @@ void blk_error_action(BlockBackend *blk, BlockErrorAction 
action,
 
 int blk_is_read_only(BlockBackend *blk)
 {
-return bdrv_is_read_only(blk->bs);
+if (blk->bs) {
+return bdrv_is_read_only(blk->bs);
+} else {
+return blk->root_state.read_only;
+}
 }
 
 int blk_is_sg(BlockBackend *blk)
@@ -897,7 +901,11 @@ void blk_eject(BlockBackend *blk, bool eject_flag)
 
 int blk_get_flags(BlockBackend *blk)
 {
-return bdrv_get_flags(blk->bs);
+if (blk->bs) {
+return bdrv_get_flags(blk->bs);
+} else {
+return blk->root_state.open_flags;
+}
 }
 
 int blk_get_max_transfer_length(BlockBackend *blk)
-- 
2.5.2

[Qemu-block] [PATCH v5 20/38] block: Prepare remaining BB functions for NULL BDS

[Max Reitz]

There are several BlockBackend functions which, in theory, cannot fail.
This patch makes them cope with the BlockDriverState pointer being NULL
by making them fall back to some default action like ignoring the value
in setters and returning the default in getters.

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
 block/block-backend.c | 76 ---
 1 file changed, 60 insertions(+), 16 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 0da4be9..33145f8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -657,7 +657,11 @@ int64_t blk_getlength(BlockBackend *blk)
 
 void blk_get_geometry(BlockBackend *blk, uint64_t *nb_sectors_ptr)
 {
-bdrv_get_geometry(blk->bs, nb_sectors_ptr);
+if (!blk->bs) {
+*nb_sectors_ptr = 0;
+} else {
+bdrv_get_geometry(blk->bs, nb_sectors_ptr);
+}
 }
 
 int64_t blk_nb_sectors(BlockBackend *blk)
@@ -889,17 +893,27 @@ int blk_is_read_only(BlockBackend *blk)
 
 int blk_is_sg(BlockBackend *blk)
 {
+if (!blk->bs) {
+return 0;
+}
+
 return bdrv_is_sg(blk->bs);
 }
 
 int blk_enable_write_cache(BlockBackend *blk)
 {
+if (!blk->bs) {
+return 0;
+}
+
 return bdrv_enable_write_cache(blk->bs);
 }
 
 void blk_set_enable_write_cache(BlockBackend *blk, bool wce)
 {
-bdrv_set_enable_write_cache(blk->bs, wce);
+if (blk->bs) {
+bdrv_set_enable_write_cache(blk->bs, wce);
+}
 }
 
 void blk_invalidate_cache(BlockBackend *blk, Error **errp)
@@ -924,12 +938,16 @@ bool blk_is_available(BlockBackend *blk)
 
 void blk_lock_medium(BlockBackend *blk, bool locked)
 {
-bdrv_lock_medium(blk->bs, locked);
+if (blk->bs) {
+bdrv_lock_medium(blk->bs, locked);
+}
 }
 
 void blk_eject(BlockBackend *blk, bool eject_flag)
 {
-bdrv_eject(blk->bs, eject_flag);
+if (blk->bs) {
+bdrv_eject(blk->bs, eject_flag);
+}
 }
 
 int blk_get_flags(BlockBackend *blk)
@@ -943,7 +961,11 @@ int blk_get_flags(BlockBackend *blk)
 
 int blk_get_max_transfer_length(BlockBackend *blk)
 {
-return blk->bs->bl.max_transfer_length;
+if (blk->bs) {
+return blk->bs->bl.max_transfer_length;
+} else {
+return 0;
+}
 }
 
 void blk_set_guest_block_size(BlockBackend *blk, int align)
@@ -958,22 +980,32 @@ void *blk_blockalign(BlockBackend *blk, size_t size)
 
 bool blk_op_is_blocked(BlockBackend *blk, BlockOpType op, Error **errp)
 {
+if (!blk->bs) {
+return false;
+}
+
 return bdrv_op_is_blocked(blk->bs, op, errp);
 }
 
 void blk_op_unblock(BlockBackend *blk, BlockOpType op, Error *reason)
 {
-bdrv_op_unblock(blk->bs, op, reason);
+if (blk->bs) {
+bdrv_op_unblock(blk->bs, op, reason);
+}
 }
 
 void blk_op_block_all(BlockBackend *blk, Error *reason)
 {
-bdrv_op_block_all(blk->bs, reason);
+if (blk->bs) {
+bdrv_op_block_all(blk->bs, reason);
+}
 }
 
 void blk_op_unblock_all(BlockBackend *blk, Error *reason)
 {
-bdrv_op_unblock_all(blk->bs, reason);
+if (blk->bs) {
+bdrv_op_unblock_all(blk->bs, reason);
+}
 }
 
 AioContext *blk_get_aio_context(BlockBackend *blk)
@@ -993,15 +1025,19 @@ static AioContext *blk_aiocb_get_aio_context(BlockAIOCB 
*acb)
 
 void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
 {
-bdrv_set_aio_context(blk->bs, new_context);
+if (blk->bs) {
+bdrv_set_aio_context(blk->bs, new_context);
+}
 }
 
 void blk_add_aio_context_notifier(BlockBackend *blk,
 void (*attached_aio_context)(AioContext *new_context, void *opaque),
 void (*detach_aio_context)(void *opaque), void *opaque)
 {
-bdrv_add_aio_context_notifier(blk->bs, attached_aio_context,
-  detach_aio_context, opaque);
+if (blk->bs) {
+bdrv_add_aio_context_notifier(blk->bs, attached_aio_context,
+  detach_aio_context, opaque);
+}
 }
 
 void blk_remove_aio_context_notifier(BlockBackend *blk,
@@ -1010,23 +1046,31 @@ void blk_remove_aio_context_notifier(BlockBackend *blk,
  void (*detach_aio_context)(void *),
  void *opaque)
 {
-bdrv_remove_aio_context_notifier(blk->bs, attached_aio_context,
- detach_aio_context, opaque);
+if (blk->bs) {
+bdrv_remove_aio_context_notifier(blk->bs, attached_aio_context,
+ detach_aio_context, opaque);
+}
 }
 
 void blk_add_close_notifier(BlockBackend *blk, Notifier *notify)
 {
-bdrv_add_close_notifier(blk->bs, notify);
+if (blk->bs) {
+bdrv_add_close_notifier(blk->bs, notify);
+}
 }
 
 void blk_io_plug(BlockBa

[Qemu-block] [PATCH v5 17/38] block: Add BlockBackendRootState

[Max Reitz]

This structure will store some of the state of the root BDS if the BDS
tree is removed, so that state can be restored once a new BDS tree is
inserted.

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
 block/block-backend.c  | 37 +
 include/block/block_int.h  | 10 ++
 include/qemu/typedefs.h|  1 +
 include/sysemu/block-backend.h |  2 ++
 4 files changed, 50 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 1f2cd9b..9590be5 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -13,6 +13,7 @@
 #include "sysemu/block-backend.h"
 #include "block/block_int.h"
 #include "block/blockjob.h"
+#include "block/throttle-groups.h"
 #include "sysemu/blockdev.h"
 #include "sysemu/sysemu.h"
 #include "qapi-event.h"
@@ -37,6 +38,10 @@ struct BlockBackend {
 /* the block size for which the guest device expects atomicity */
 int guest_block_size;
 
+/* If the BDS tree is removed, some of its options are stored here (which
+ * can be used to restore those options in the new BDS on insert) */
+BlockBackendRootState root_state;
+
 /* I/O stats (display with "info blockstats"). */
 BlockAcctStats stats;
 
@@ -161,6 +166,7 @@ static void blk_delete(BlockBackend *blk)
 bdrv_unref(blk->bs);
 blk->bs = NULL;
 }
+g_free(blk->root_state.throttle_group_name);
 /* Avoid double-remove after blk_hide_on_behalf_of_hmp_drive_del() */
 if (blk->name[0]) {
 QTAILQ_REMOVE(_backends, blk, link);
@@ -1050,3 +1056,34 @@ int blk_probe_geometry(BlockBackend *blk, HDGeometry 
*geo)
 {
 return bdrv_probe_geometry(blk->bs, geo);
 }
+
+/*
+ * Updates the BlockBackendRootState object with data from the currently
+ * attached BlockDriverState.
+ */
+void blk_update_root_state(BlockBackend *blk)
+{
+assert(blk->bs);
+
+blk->root_state.open_flags= blk->bs->open_flags;
+blk->root_state.read_only = blk->bs->read_only;
+blk->root_state.detect_zeroes = blk->bs->detect_zeroes;
+
+blk->root_state.io_limits_enabled = blk->bs->io_limits_enabled;
+
+g_free(blk->root_state.throttle_group_name);
+if (blk->bs->throttle_state) {
+throttle_get_config(blk->bs->throttle_state,
+>root_state.throttle_config);
+blk->root_state.throttle_group_name =
+g_strdup(throttle_group_get_name(blk->bs));
+} else {
+blk->root_state.throttle_config = (ThrottleConfig){};
+blk->root_state.throttle_group_name = NULL;
+}
+}
+
+BlockBackendRootState *blk_get_root_state(BlockBackend *blk)
+{
+return >root_state;
+}
diff --git a/include/block/block_int.h b/include/block/block_int.h
index bd35570..ca1eefa 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -449,6 +449,16 @@ struct BlockDriverState {
 NotifierWithReturn write_threshold_notifier;
 };
 
+struct BlockBackendRootState {
+int open_flags;
+bool read_only;
+BlockdevDetectZeroesOptions detect_zeroes;
+
+bool io_limits_enabled;
+ThrottleConfig throttle_config;
+char *throttle_group_name;
+};
+
 
 /* Essential block drivers which must always be statically linked into qemu, 
and
  * which therefore can be accessed without using bdrv_find_format() */
diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
index 97ac727..5105cc7 100644
--- a/include/qemu/typedefs.h
+++ b/include/qemu/typedefs.h
@@ -11,6 +11,7 @@ typedef struct AddressSpace AddressSpace;
 typedef struct AioContext AioContext;
 typedef struct AudioState AudioState;
 typedef struct BlockBackend BlockBackend;
+typedef struct BlockBackendRootState BlockBackendRootState;
 typedef struct BlockDriverState BlockDriverState;
 typedef struct BusClass BusClass;
 typedef struct BusState BusState;
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index eafcef0..52e35a1 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -163,6 +163,8 @@ void blk_add_close_notifier(BlockBackend *blk, Notifier 
*notify);
 void blk_io_plug(BlockBackend *blk);
 void blk_io_unplug(BlockBackend *blk);
 BlockAcctStats *blk_get_stats(BlockBackend *blk);
+BlockBackendRootState *blk_get_root_state(BlockBackend *blk);
+void blk_update_root_state(BlockBackend *blk);
 
 void *blk_aio_get(const AIOCBInfo *aiocb_info, BlockBackend *blk,
   BlockCompletionFunc *cb, void *opaque);
-- 
2.5.2

[Qemu-block] [PATCH v5 10/38] hw/block/fdc: Implement tray status

[Max Reitz]

The tray of an FDD is open iff there is no medium inserted (there are
only two states for an FDD: "medium inserted" or "no medium inserted").

Signed-off-by: Max Reitz <mre...@redhat.com>
---
 hw/block/fdc.c   | 20 
 tests/fdc-test.c |  4 +---
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 6686a72..4292ece 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -192,6 +192,8 @@ typedef struct FDrive {
 uint8_t ro;   /* Is read-only   */
 uint8_t media_changed;/* Is media changed   */
 uint8_t media_rate;   /* Data rate of medium*/
+
+bool media_inserted;  /* Is there a medium in the tray */
 } FDrive;
 
 static void fd_init(FDrive *drv)
@@ -261,7 +263,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t 
track, uint8_t sect,
 #endif
 drv->head = head;
 if (drv->track != track) {
-if (drv->blk != NULL && blk_is_inserted(drv->blk)) {
+if (drv->media_inserted) {
 drv->media_changed = 0;
 }
 ret = 1;
@@ -270,7 +272,7 @@ static int fd_seek(FDrive *drv, uint8_t head, uint8_t 
track, uint8_t sect,
 drv->sect = sect;
 }
 
-if (drv->blk == NULL || !blk_is_inserted(drv->blk)) {
+if (!drv->media_inserted) {
 ret = 2;
 }
 
@@ -296,7 +298,7 @@ static void fd_revalidate(FDrive *drv)
 ro = blk_is_read_only(drv->blk);
 pick_geometry(drv->blk, _heads, _track,
   _sect, drv->drive, , );
-if (!blk_is_inserted(drv->blk)) {
+if (!drv->media_inserted) {
 FLOPPY_DPRINTF("No disk in drive\n");
 } else {
 FLOPPY_DPRINTF("Floppy disk (%d h %d t %d s) %s\n", nb_heads,
@@ -692,7 +694,7 @@ static bool fdrive_media_changed_needed(void *opaque)
 {
 FDrive *drive = opaque;
 
-return (drive->blk != NULL && drive->media_changed != 1);
+return (drive->media_inserted && drive->media_changed != 1);
 }
 
 static const VMStateDescription vmstate_fdrive_media_changed = {
@@ -2184,12 +2186,21 @@ static void fdctrl_change_cb(void *opaque, bool load)
 {
 FDrive *drive = opaque;
 
+drive->media_inserted = load && drive->blk && blk_is_inserted(drive->blk);
+
 drive->media_changed = 1;
 fd_revalidate(drive);
 }
 
+static bool fdctrl_is_tray_open(void *opaque)
+{
+FDrive *drive = opaque;
+return !drive->media_inserted;
+}
+
 static const BlockDevOps fdctrl_block_ops = {
 .change_media_cb = fdctrl_change_cb,
+.is_tray_open = fdctrl_is_tray_open,
 };
 
 /* Init functions */
@@ -2217,6 +2228,7 @@ static void fdctrl_connect_drives(FDCtrl *fdctrl, Error 
**errp)
 fdctrl_change_cb(drive, 0);
 if (drive->blk) {
 blk_set_dev_ops(drive->blk, _block_ops, drive);
+drive->media_inserted = blk_is_inserted(drive->blk);
 }
 }
 }
diff --git a/tests/fdc-test.c b/tests/fdc-test.c
index 416394f..b5a4696 100644
--- a/tests/fdc-test.c
+++ b/tests/fdc-test.c
@@ -304,9 +304,7 @@ static void test_media_insert(void)
 qmp_discard_response("{'execute':'change', 'arguments':{"
  " 'device':'floppy0', 'target': %s, 'arg': 'raw' }}",
  test_image);
-qmp_discard_response(""); /* ignore event
- (FIXME open -> open transition?!) */
-qmp_discard_response(""); /* ignore event */
+qmp_discard_response(""); /* ignore event (open -> close) */
 
 dir = inb(FLOPPY_BASE + reg_dir);
 assert_bit_set(dir, DSKCHG);
-- 
2.5.2

[Qemu-block] [PATCH v5 16/38] block: Move I/O status and error actions into BB

[Max Reitz]

These options are only relevant for the user of a whole BDS tree (like a
guest device or a block job) and should thus be moved into the
BlockBackend.

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
---
 block.c| 125 -
 block/backup.c |  17 --
 block/block-backend.c  | 116 --
 block/commit.c |   3 +-
 block/mirror.c |  17 --
 block/qapi.c   |   4 +-
 block/stream.c |   3 +-
 blockdev.c |   6 +-
 blockjob.c |   5 +-
 include/block/block.h  |  11 
 include/block/block_int.h  |   6 --
 include/sysemu/block-backend.h |   7 +++
 qmp.c  |   6 +-
 13 files changed, 158 insertions(+), 168 deletions(-)

diff --git a/block.c b/block.c
index f81ef23..67a39fb 100644
--- a/block.c
+++ b/block.c
@@ -257,7 +257,6 @@ BlockDriverState *bdrv_new(void)
 for (i = 0; i < BLOCK_OP_TYPE_MAX; i++) {
 QLIST_INIT(>op_blockers[i]);
 }
-bdrv_iostatus_disable(bs);
 notifier_list_init(>close_notifiers);
 notifier_with_return_list_init(>before_write_notifiers);
 qemu_co_queue_init(>throttled_reqs[0]);
@@ -2038,14 +2037,6 @@ static void bdrv_move_feature_fields(BlockDriverState 
*bs_dest,
_src->throttle_timers,
sizeof(ThrottleTimers));
 
-/* r/w error */
-bs_dest->on_read_error  = bs_src->on_read_error;
-bs_dest->on_write_error = bs_src->on_write_error;
-
-/* i/o status */
-bs_dest->iostatus_enabled   = bs_src->iostatus_enabled;
-bs_dest->iostatus   = bs_src->iostatus;
-
 /* dirty bitmap */
 bs_dest->dirty_bitmaps  = bs_src->dirty_bitmaps;
 
@@ -2605,82 +2596,6 @@ void bdrv_get_geometry(BlockDriverState *bs, uint64_t 
*nb_sectors_ptr)
 *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
 }
 
-void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error,
-   BlockdevOnError on_write_error)
-{
-bs->on_read_error = on_read_error;
-bs->on_write_error = on_write_error;
-}
-
-BlockdevOnError bdrv_get_on_error(BlockDriverState *bs, bool is_read)
-{
-return is_read ? bs->on_read_error : bs->on_write_error;
-}
-
-BlockErrorAction bdrv_get_error_action(BlockDriverState *bs, bool is_read, int 
error)
-{
-BlockdevOnError on_err = is_read ? bs->on_read_error : bs->on_write_error;
-
-switch (on_err) {
-case BLOCKDEV_ON_ERROR_ENOSPC:
-return (error == ENOSPC) ?
-   BLOCK_ERROR_ACTION_STOP : BLOCK_ERROR_ACTION_REPORT;
-case BLOCKDEV_ON_ERROR_STOP:
-return BLOCK_ERROR_ACTION_STOP;
-case BLOCKDEV_ON_ERROR_REPORT:
-return BLOCK_ERROR_ACTION_REPORT;
-case BLOCKDEV_ON_ERROR_IGNORE:
-return BLOCK_ERROR_ACTION_IGNORE;
-default:
-abort();
-}
-}
-
-static void send_qmp_error_event(BlockDriverState *bs,
- BlockErrorAction action,
- bool is_read, int error)
-{
-IoOperationType optype;
-
-optype = is_read ? IO_OPERATION_TYPE_READ : IO_OPERATION_TYPE_WRITE;
-qapi_event_send_block_io_error(bdrv_get_device_name(bs), optype, action,
-   bdrv_iostatus_is_enabled(bs),
-   error == ENOSPC, strerror(error),
-   _abort);
-}
-
-/* This is done by device models because, while the block layer knows
- * about the error, it does not know whether an operation comes from
- * the device or the block layer (from a job, for example).
- */
-void bdrv_error_action(BlockDriverState *bs, BlockErrorAction action,
-   bool is_read, int error)
-{
-assert(error >= 0);
-
-if (action == BLOCK_ERROR_ACTION_STOP) {
-/* First set the iostatus, so that "info block" returns an iostatus
- * that matches the events raised so far (an additional error iostatus
- * is fine, but not a lost one).
- */
-bdrv_iostatus_set_err(bs, error);
-
-/* Then raise the request to stop the VM and the event.
- * qemu_system_vmstop_request_prepare has two effects.  First,
- * it ensures that the STOP event always comes after the
- * BLOCK_IO_ERROR event.  Second, it ensures that even if management
- * can observe the STOP event and do a "cont" before the STOP
- * event is issued, the VM will not stop.  In this case, vm_start()
- * also ensures that the STOP/RESUME pair of events is emitted.
- */
-qemu_system_vmstop_request_prepare();
-send_qmp_error_event(bs, action, is_read, error);
-qemu_system_vmstop_request(RUN_STATE_IO_ERROR);
-  

[Qemu-block] [PATCH v5 13/38] block: Move guest_block_size into BlockBackend

[Max Reitz]

guest_block_size is a guest device property so it should be moved into
the interface between block layer and guest devices, which is the
BlockBackend.

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alberto Garcia <be...@igalia.com>
---
 block.c   | 7 ---
 block/block-backend.c | 7 +--
 include/block/block.h | 1 -
 include/block/block_int.h | 3 ---
 4 files changed, 5 insertions(+), 13 deletions(-)

diff --git a/block.c b/block.c
index 661c621..55357c4 100644
--- a/block.c
+++ b/block.c
@@ -852,7 +852,6 @@ static int bdrv_open_common(BlockDriverState *bs, 
BlockDriverState *file,
 goto fail_opts;
 }
 
-bs->guest_block_size = 512;
 bs->request_alignment = 512;
 bs->zero_beyond_eof = true;
 open_flags = bdrv_open_flags(bs, flags);
@@ -2021,7 +2020,6 @@ static void bdrv_move_feature_fields(BlockDriverState 
*bs_dest,
 /* move some fields that need to stay attached to the device */
 
 /* dev info */
-bs_dest->guest_block_size   = bs_src->guest_block_size;
 bs_dest->copy_on_read   = bs_src->copy_on_read;
 
 bs_dest->enable_write_cache = bs_src->enable_write_cache;
@@ -3311,11 +3309,6 @@ void bdrv_lock_medium(BlockDriverState *bs, bool locked)
 }
 }
 
-void bdrv_set_guest_block_size(BlockDriverState *bs, int align)
-{
-bs->guest_block_size = align;
-}
-
 BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name)
 {
 BdrvDirtyBitmap *bm;
diff --git a/block/block-backend.c b/block/block-backend.c
index 501130b..ff862ad 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -31,6 +31,9 @@ struct BlockBackend {
 /* TODO change to DeviceState when all users are qdevified */
 const BlockDevOps *dev_ops;
 void *dev_opaque;
+
+/* the block size for which the guest device expects atomicity */
+int guest_block_size;
 };
 
 typedef struct BlockBackendAIOCB {
@@ -334,7 +337,7 @@ void blk_detach_dev(BlockBackend *blk, void *dev)
 blk->dev = NULL;
 blk->dev_ops = NULL;
 blk->dev_opaque = NULL;
-bdrv_set_guest_block_size(blk->bs, 512);
+blk->guest_block_size = 512;
 blk_unref(blk);
 }
 
@@ -789,7 +792,7 @@ int blk_get_max_transfer_length(BlockBackend *blk)
 
 void blk_set_guest_block_size(BlockBackend *blk, int align)
 {
-bdrv_set_guest_block_size(blk->bs, align);
+blk->guest_block_size = align;
 }
 
 void *blk_blockalign(BlockBackend *blk, size_t size)
diff --git a/include/block/block.h b/include/block/block.h
index c9cc62b..3e818f4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -464,7 +464,6 @@ void bdrv_img_create(const char *filename, const char *fmt,
 size_t bdrv_min_mem_align(BlockDriverState *bs);
 /* Returns optimal alignment in bytes for bounce buffer */
 size_t bdrv_opt_mem_align(BlockDriverState *bs);
-void bdrv_set_guest_block_size(BlockDriverState *bs, int align);
 void *qemu_blockalign(BlockDriverState *bs, size_t size);
 void *qemu_blockalign0(BlockDriverState *bs, size_t size);
 void *qemu_try_blockalign(BlockDriverState *bs, size_t size);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f3b3354..b7e1e16 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -412,9 +412,6 @@ struct BlockDriverState {
 /* Alignment requirement for offset/length of I/O requests */
 unsigned int request_alignment;
 
-/* the block size for which the guest device expects atomicity */
-int guest_block_size;
-
 /* do we need to tell the quest if we have a volatile write cache? */
 int enable_write_cache;
 
-- 
2.5.2

[Qemu-block] [PATCH v5 28/38] blockdev: Add blockdev-close-tray

[Max Reitz]

Signed-off-by: Max Reitz <mre...@redhat.com>
---
 blockdev.c   | 23 +++
 qapi/block-core.json | 16 
 qmp-commands.hx  | 35 +++
 3 files changed, 74 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 6bc5841..d07bf8a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2108,6 +2108,29 @@ out:
 }
 }
 
+void qmp_blockdev_close_tray(const char *device, Error **errp)
+{
+BlockBackend *blk;
+
+blk = blk_by_name(device);
+if (!blk) {
+error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
+  "Device '%s' not found", device);
+return;
+}
+
+if (!blk_dev_has_removable_media(blk)) {
+error_setg(errp, "Device '%s' is not removable", device);
+return;
+}
+
+if (!blk_dev_is_tray_open(blk)) {
+return;
+}
+
+blk_dev_change_media_cb(blk, true);
+}
+
 /* throttling disk I/O limits */
 void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
int64_t bps_wr,
diff --git a/qapi/block-core.json b/qapi/block-core.json
index b9b4a24..1a51829 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1899,6 +1899,22 @@
   'data': { 'device': 'str',
 '*force': 'bool' } }
 
+##
+# @blockdev-close-tray:
+#
+# Closes a block device's tray. If there is a block driver state tree 
associated
+# with the block device (which is currently ejected), that tree will be loaded
+# as the medium.
+#
+# If the tray was already closed before, this will be a no-op.
+#
+# @device: block device name
+#
+# Since: 2.5
+##
+{ 'command': 'blockdev-close-tray',
+  'data': { 'device': 'str' } }
+
 
 ##
 # @BlockErrorAction
diff --git a/qmp-commands.hx b/qmp-commands.hx
index f6ea5ea..7c7125a 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3936,6 +3936,41 @@ Example:
 EQMP
 
 {
+.name   = "blockdev-close-tray",
+.args_type  = "device:s",
+.mhandler.cmd_new = qmp_marshal_input_blockdev_close_tray,
+},
+
+SQMP
+blockdev-close-tray
+---
+
+Closes a block device's tray. If there is a block driver state tree associated
+with the block device (which is currently ejected), that tree will be loaded as
+the medium.
+
+If the tray was already closed before, this will be a no-op.
+
+Arguments:
+
+- "device": block device name (json-string)
+
+Example:
+
+-> { "execute": "blockdev-close-tray",
+ "arguments": { "device": "ide1-cd0" } }
+
+<- { "timestamp": { "seconds": 1418751345,
+"microseconds": 272147 },
+ "event": "DEVICE_TRAY_MOVED",
+ "data": { "device": "ide1-cd0",
+   "tray-open": false } }
+
+<- { "return": {} }
+
+EQMP
+
+{
 .name   = "query-named-block-nodes",
 .args_type  = "",
 .mhandler.cmd_new = qmp_marshal_input_query_named_block_nodes,
-- 
2.5.2

[Qemu-block] [PATCH v5 21/38] block: Add blk_insert_bs()

[Max Reitz]

This function associates the given BlockDriverState with the given
BlockBackend.

Signed-off-by: Max Reitz <mre...@redhat.com>
Reviewed-by: Eric Blake <ebl...@redhat.com>
Reviewed-by: Alberto Garcia <be...@igalia.com>
---
 block/block-backend.c  | 16 
 include/sysemu/block-backend.h |  1 +
 2 files changed, 17 insertions(+)

diff --git a/block/block-backend.c b/block/block-backend.c
index 33145f8..652385e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -314,6 +314,22 @@ void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk)
 }
 
 /*
+ * Associates a new BlockDriverState with @blk.
+ */
+void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
+{
+if (bs->blk == blk) {
+return;
+}
+
+assert(!blk->bs);
+assert(!bs->blk);
+bdrv_ref(bs);
+blk->bs = bs;
+bs->blk = blk;
+}
+
+/*
  * Attach device model @dev to @blk.
  * Return 0 on success, -EBUSY when a device model is attached already.
  */
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 52e35a1..9306a52 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -72,6 +72,7 @@ BlockBackend *blk_by_name(const char *name);
 BlockBackend *blk_next(BlockBackend *blk);
 
 BlockDriverState *blk_bs(BlockBackend *blk);
+void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs);
 
 void blk_hide_on_behalf_of_hmp_drive_del(BlockBackend *blk);
 
-- 
2.5.2

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

[Max Reitz]

On 23.11.2015 16:59, 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 <kw...@redhat.com>
> ---
>  block.c   | 19 +++
>  include/block/block_int.h |  1 +
>  2 files changed, 16 insertions(+), 4 deletions(-)

Thanks, now I don't need to fix it myself. :-)

(I would have had to do that for an in-work series of mine)

> diff --git a/block.c b/block.c
> index 23d9e10..02125e2 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   = child_name,
>  .role   = child_role,
>  };
>  
> @@ -1165,7 +1167,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),
> @@ -1322,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);
> @@ -3952,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;
>  }
>  

A good general solution, but I think bdrv_refresh_filename() may be bad
enough to break with general solutions. ;-)

bdrv_refresh_filename() only considers "file" and "backing" (actually,
it only supports "file" for now, I'm working on "backing", though). The
only drivers with other children are quorum, blkdebug, blkverify and
VMDK. The former three have their own implementation of
bdrv_refresh_filename(), so they don't use append_open_options() at all.
The latter, however, (VMDK) does not.

This change to append_open_options results in the extent.%d options
simply being omitted altogether because bdrv_refresh_filename() does not
fetch them. Before, they were included in the VMDK BDS's options, which
is not ideal but works more or less.

In order to "fix" this, I see three ways right now:
1. Just care about "file" and "backing". bdrv_refresh_filename()
   doesn't support anything else, so that will be fine.
2. Implement bdrv_refresh_filename() specifically for VMDK so
   append_open_options() will never have to handle anything but "file"
   and "backing".
3. Fix bdrv_refresh_filename() so that it handles all children and not
   just "file" and "backing".

Since we are shooting for 2.6 anyway (I assume ;-)), I think we should
go for option 3. This means that this patch is fine, and I'll see to
fixing bdrv_refresh_filename() (because I'm working on that anyway).

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

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

[Max Reitz]

On 23.11.2015 16:59, 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/qemu-iotests/iotests.py | 2 +-
>  2 files changed, 2 insertions(+), 5 deletions(-)

Besides the failing make check, the patch looks good.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v2 18/21] blkdebug: Enable reopen

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> Just reopening the children (as block.c does now) is enough.
> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block/blkdebug.c | 7 +++
>  1 file changed, 7 insertions(+)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

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

[Max Reitz]

On 23.11.2015 16:59, 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(-)
> 
> diff --git a/block.c b/block.c
> index eff0c19..397014a 100644
> --- a/block.c
> +++ b/block.c

[...]

> @@ -1747,12 +1816,22 @@ 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)
> + * 2. Set in flags (only for top level)
>   * 3. Retained from explicitly set options of bs
>   * 4. Inherited from parent node
>   * 5. Retained from effective options of bs
>   */
>  
> +if (!parent_options) {
> +/*
> + * Any setting represented by flags is always updated. If the
> + * corresponding QDict option is set, it takes precedence. Otherwise
> + * the flag is translated into a QDict option. The old setting of bs 
> is
> + * not considered.
> + */
> +update_options_from_flags(options, flags);
> +}
> +
>  /* Old explicitly set values (don't overwrite by inherited value) */
>  old_options = qdict_clone_shallow(bs->explicit_options);
>  bdrv_join_options(bs, options, old_options);
> @@ -1923,6 +2002,19 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, 
> BlockReopenQueue *queue,
>  goto error;
>  }
>  
> +update_flags_from_options(_state->flags, opts);
> +
> +/* If a guest device is attached, it owns WCE */
> +if (reopen_state->bs->blk && 
> blk_get_attached_dev(reopen_state->bs->blk)) {
> +bool old_wce = bdrv_enable_write_cache(reopen_state->bs);
> +bool new_wce = (reopen_state->flags & BDRV_O_CACHE_WB);
> +if (old_wce != new_wce) {
> +error_setg(errp, "Cannot change cache.writeback: Device 
> attached");
> +ret = -EINVAL;
> +goto error;
> +}
> +}
> +

Time to get back to my question regarding bdrv_set_enable_write_cache():

1. bdrv_set_enable_write_cache() sets/unsets BDRV_O_CACHE_WB in
   bs->open_flags so that it is preserved through a reopen.
2. On reopen, bdrv_reopen_queue_child() calls
   update_options_from_flags() unless @parent_options is set. If that
   function is called, it will set the appropriate caching options in
   @options as dictated by bdrv_set_enable_write_cache().
3. If @parent_options was NULL, the update_flags_from_options() call
   here in bdrv_reopen_prepare() will set BDRV_O_CACHE_WB just as
   dictated by bdrv_set_enable_write_cache() (unless overwritten).
   That is what we want.
4. If @parent_options was not NULL, the caching options in
   bs->open_flags are completely overwritten, discarding everything that
   had been set by bdrv_set_enable_write_cache(). That's not so good.

@parent_options is tested so that update_options_from_flags() is called
only for root BDSs. It is possible to call bdrv_set_enable_write_cache()
on non-root BDSs (e.g. commit_active_start() does it on the commit base
via mirror_start_job()), so I do think overriding the setting made by
bdrv_set_enable_write_cache() on reopen for any non-root BDSs is not
correct.

Maybe bdrv_set_enable_write_cache() should set cache.writeback in
bs->explicit_options to on or off instead of using bs->open_flags. But
in that case, we can no longer use bdrv_set_enable_write_cache() in
bdrv_open_common(), because the cache setting there may be implicit.

Max

>  /* 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");



signature.asc
Description: OpenPGP digital signature

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

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> 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 <kw...@redhat.com>
> ---
>  block.c   | 24 ++--
>  include/block/block.h |  1 +
>  include/block/block_int.h |  1 +
>  3 files changed, 24 insertions(+), 2 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

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

[Max Reitz]

On 27.11.2015 18:58, Max Reitz wrote:
> On 23.11.2015 16:59, 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 <kw...@redhat.com>
>> ---
>>  block.c   | 19 +++
>>  include/block/block_int.h |  1 +
>>  2 files changed, 16 insertions(+), 4 deletions(-)
> 
> Thanks, now I don't need to fix it myself. :-)
> 
> (I would have had to do that for an in-work series of mine)
> 
>> diff --git a/block.c b/block.c
>> index 23d9e10..02125e2 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   = child_name,
>>  .role   = child_role,
>>  };
>>  
>> @@ -1165,7 +1167,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),
>> @@ -1322,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);
>> @@ -3952,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;
>>  }
>>  
> 
> A good general solution, but I think bdrv_refresh_filename() may be bad
> enough to break with general solutions. ;-)
> 
> bdrv_refresh_filename() only considers "file" and "backing" (actually,
> it only supports "file" for now, I'm working on "backing", though). The
> only drivers with other children are quorum, blkdebug, blkverify and
> VMDK. The former three have their own implementation of
> bdrv_refresh_filename(), so they don't use append_open_options() at all.
> The latter, however, (VMDK) does not.
> 
> This change to append_open_options results in the extent.%d options
> simply being omitted altogether because bdrv_refresh_filename() does not
> fetch them. Before, they were included in the VMDK BDS's options, which
> is not ideal but works more or less.
> 
> In order to "fix" this, I see three ways right now:
> 1. Just care about "file" and "backing". bdrv_refresh_filename()
>doesn't support anything else, so that will be fine.
> 2. Implement bdrv_refresh_filename() specifically for VMDK so
>append_open_options() will never have to handle anything but "file"
>and "backing".
> 3. Fix bdrv_refresh_filename() so that it handles all children and not
>just "file" and "backing".
> 
> Since we are shooting for 2.6 anyway (I assume ;-)), I think we should
> go for option 3. This means that this patch is fine, and I'll see to
> fixing bdrv_refresh_filename() (because I'm working on that anyway).
> 
> Reviewed-by: Max Reitz <mre...@redhat.com>

Oops, focused so much on this that I missed the issue Wen Congyang
found. So this R-b is assuming that is fixed *cough*.

Max



signature.asc
Description: OpenPGP digital signature

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

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> 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 <kw...@redhat.com>
> ---
>  block.c | 50 --
>  1 file changed, 32 insertions(+), 18 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

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

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> 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 <kw...@redhat.com>
> ---
>  block/mirror.c | 28 
>  1 file changed, 28 insertions(+)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 52c9abf..0620068 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);
> @@ -701,6 +713,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);
> @@ -724,6 +737,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
> + * multple BlockBackends per BlockDriverState. */

*multiple

With that fixed:

Reviewed-by: Max Reitz <mre...@redhat.com>

> +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;
> 




signature.asc
Description: OpenPGP digital signature

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

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> 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 <kw...@redhat.com>
> ---
>  block.c | 16 +++-
>  1 file changed, 15 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

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

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> For bs->file, using references to existing BDSes has been possible for a
> while already. This patch enables the same for bs->backing_hd.

It's just "backing" now (instead of "backing_hd").

> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block.c   | 47 +--
>  block/mirror.c|  2 +-
>  include/block/block.h |  3 ++-
>  3 files changed, 32 insertions(+), 20 deletions(-)

An additional thing I believe this patch can do (or a follow-up) is to
remove the part of bdrv_open_inherit() which sets BDRV_O_NO_BACKING if
the @backing option is the empty string.

With the changes done to bdrv_open_backing_file() here, we can easily
put that case into that function (where I think it belongs). If
!reference[0], we can just return success without having actually opened
a backing file.

We may have to think about the case of
{'backing': '', 'backing.driver': 'null-co'}, though. Right now, that is
an error (with a rather obscure error message, probably), and if we want
to keep it an error, we'd have to check whether options is empty in
bdrv_open_backing_file() if !reference[0], and emit a nice error if it
is not ("cannot specify backing options for suppressed backing file" or
something like that).

> diff --git a/block.c b/block.c
> index 675e5a8..ca6c4e9 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,17 @@ 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);

This assert() can be dropped along with the bdrv_new() call (they are
related: originally, the bdrv_open_inherit() call below took
>backing_hd as its first parameter, and when that was changed, this
assert() was apparently overlooked).

(http://lists.nongnu.org/archive/html/qemu-block/2015-11/msg00344.html)


Anyway, with the commit message fixed:

Reviewed-by: Max Reitz <mre...@redhat.com>

> +backing_hd = NULL;
>  ret = bdrv_open_inherit(_hd,
>  *backing_filename ? backing_filename : NULL,
> -NULL, options, 0, bs, _backing, 
> _err);
> +

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

[Max Reitz]

On 23.11.2015 16:59, Kevin Wolf wrote:
> Options are not actually inherited from the parent node yet, but this
> commit lays the grounds for doing so.
> 
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
>  block.c   | 52 
> ---
>  include/block/block_int.h |  3 ++-
>  2 files changed, 33 insertions(+), 22 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH v7 14/24] nbd: Switch from close to eject notifier

[Max Reitz]

On 01.12.2015 14:16, Kevin Wolf wrote:
> Am 30.11.2015 um 18:22 hat Max Reitz geschrieben:
>> On 30.11.2015 16:36, Kevin Wolf wrote:
>>> Am 09.11.2015 um 23:39 hat Max Reitz geschrieben:
>>>> The NBD code uses the BDS close notifier to determine when a medium is
>>>> ejected. However, now it should use the BB's BDS removal notifier for
>>>> that instead of the BDS's close notifier.
>>>>
>>>> Signed-off-by: Max Reitz <mre...@redhat.com>
>>>> ---
>>>>  blockdev-nbd.c | 37 +
>>>>  nbd.c  | 13 +
>>>>  2 files changed, 14 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
>>>> index bcdd18b..b28a55b 100644
>>>> --- a/blockdev-nbd.c
>>>> +++ b/blockdev-nbd.c
>>>> @@ -46,37 +46,11 @@ void qmp_nbd_server_start(SocketAddress *addr, Error 
>>>> **errp)
>>>>  }
>>>>  }
>>>>  
>>>> -/*
>>>> - * Hook into the BlockBackend notifiers to close the export when the
>>>> - * backend is closed.
>>>> - */
>>>> -typedef struct NBDCloseNotifier {
>>>> -Notifier n;
>>>> -NBDExport *exp;
>>>> -QTAILQ_ENTRY(NBDCloseNotifier) next;
>>>> -} NBDCloseNotifier;
>>>> -
>>>> -static QTAILQ_HEAD(, NBDCloseNotifier) close_notifiers =
>>>> -QTAILQ_HEAD_INITIALIZER(close_notifiers);
>>>> -
>>>> -static void nbd_close_notifier(Notifier *n, void *data)
>>>> -{
>>>> -NBDCloseNotifier *cn = DO_UPCAST(NBDCloseNotifier, n, n);
>>>> -
>>>> -notifier_remove(>n);
>>>> -QTAILQ_REMOVE(_notifiers, cn, next);
>>>> -
>>>> -nbd_export_close(cn->exp);
>>>> -nbd_export_put(cn->exp);
>>>
>>> Here you remove a close/put pair, but in the new code you only add the
>>> close call. Why is this correct?
>>
>> Thanks for putting so much unfounded faith in me. :-)
>>
>> After having put quite some time into looking into it, first I have to
>> say that it's a very good question.
>>
>> First off, it's wrong. This is because I thought every export would have
>> a refcount of one. Turns out this is wrong, they have a refcount of two:
>> It is created with a refcount of one, and then it gains another by
>> giving it a name and entering it into the export list.
>>
>> I did know about the second but I completely forgot about the former.
>> And indeed I do think it is wrong. qmp_nbd_server_add() does not keep
>> the reference to the export, once the function returns, it is gone.
>> Therefore, it should call nbd_export_put() before returning.
>>
>> So in my opinion the current code is wrong and I didn't fix it enough.
>> *cough*
>>
>> So, with the nbd_export_put() added to qmp_nbd_server_add(), every
>> export will have a refcount of 1 + |clients|. The eject notifier will
>> call nbd_close_notifier(), which first disconnects the clients, thus
>> reducing the refcount by |clients|, and then sets the name to NULL, thus
>> dropping the final refcount and destroying the export.
>>
>> In the old code, we needed another nbd_export_put() because of the
>> superfluous reference from nbd_export_new(), which doesn't actually
>> exist for blockdev-nbd (it does for qemu-nbd, though).
> 
> Okay, that makes sense to me. So first a patch that moves the existing
> nbd_export_put() call from nbd_close_notifier() to qmp_nbd_server_add()
> and then this one on top?

I would have put both in the same patch, but this does make more sense.
I'll do so.

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 7/8] block: Make bdrv_open() return a BDS

[Max Reitz]

On 01.12.2015 15:44, Alberto Garcia wrote:
> On Tue 10 Nov 2015 04:44:22 AM CET, Max Reitz wrote:
>> @@ -1398,32 +1397,21 @@ static int bdrv_open_inherit(BlockDriverState **pbs, 
>> const char *filename,
>>  bool options_non_empty = options ? qdict_size(options) : false;
>>  QDECREF(options);
>>  
>> -if (*pbs) {
>> -error_setg(errp, "Cannot reuse an existing BDS when referencing 
>> "
>> -   "another block device");
>> -return -EINVAL;
>> -}
>> -
>>  if (filename || options_non_empty) {
>>  error_setg(errp, "Cannot reference an existing block device 
>> with "
>> "additional options or a new filename");
>> -return -EINVAL;
>> +return NULL;
>>  }
>>  
>>  bs = bdrv_lookup_bs(reference, reference, errp);
>>  if (!bs) {
>> -return -ENODEV;
>> +return NULL;
>>  }
>>  bdrv_ref(bs);
>> -*pbs = bs;
>> -return 0;
>> +return bs;
>>  }
> 
> This last part is probably a bit simpler with just one return statement:
> 
>bs = bdrv_lookup_bs(reference, reference, errp);
>if (bs) {
>bdrv_ref(bs);
>}
>return bs;
> 
> but I'm fine either way.

Yes, thanks for the suggestion; but I think I like it to be explicit
("ret = do_something(); if (ret indicates failure) { fail; } go_on;").
Maybe I'm just not flexible enough to discard my precious patterns. :-)

> Reviewed-by: Alberto Garcia <be...@igalia.com>

Thanks!

Max



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH 3/8] block: Let bdrv_open_inherit() return the snapshot

[Max Reitz]

On 01.12.2015 15:35, Alberto Garcia wrote:
> On Tue 10 Nov 2015 04:44:18 AM CET, Max Reitz wrote:
>> -int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
>> +static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
>> +   int flags, Error **errp)
>>  {
>>  /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
>>  char *tmp_filename = g_malloc0(PATH_MAX + 1);
>> @@ -1354,11 +1355,15 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, 
>> int flags, Error **errp)
>>  goto out;
>>  }
>>  
>> +bdrv_ref(bs_snapshot);
>>  bdrv_append(bs_snapshot, bs);
>>  
>> +g_free(tmp_filename);
>> +return bs_snapshot;
>> +
>>  out:
>>  g_free(tmp_filename);
>> -return ret;
>> +return NULL;
>>  }
> 
> If I'm not wrong, now that you're not returning 'ret' anymore there's a
> "ret = total_size" line earlier in this function that is useless now.

Yes, indeed, thanks for finding that. Will fix.

Max

> Other than that,
> 
> Reviewed-by: Alberto Garcia <be...@igalia.com>
> 
> Berto
> 




signature.asc
Description: OpenPGP digital signature

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

[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 <mre...@redhat.com>
> 
> 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()

[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 <kw...@redhat.com>
> ---
>  block.c   | 20 
>  include/block/block_int.h |  1 +
>  2 files changed, 17 insertions(+), 4 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

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

[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 <kw...@redhat.com>
> ---
>  block.c| 100 
> ++---
>  blockdev.c |  52 ++--
>  2 files changed, 111 insertions(+), 41 deletions(-)

Reviewed-by: Max Reitz <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

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

[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 <kw...@redhat.com>
> ---
>  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 <mre...@redhat.com>



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.5?] qcow2: always initialize specific image info

[Max Reitz]

On 07.12.2015 19:11, Denis V. Lunev wrote:
> On 12/07/2015 08:54 PM, Eric Blake wrote:
>> On 12/07/2015 10:51 AM, Eric Blake wrote:
>>> [adding qemu-devel - ALL patches should go to qemu-devel, even if they
>>> are also going to a sub-list like qemu-block]
>>>
>>> On 12/07/2015 10:07 AM, Roman Kagan wrote:
 qcow2_get_specific_info() used to have a code path which would leave
 pointer to ImageInfoSpecificQCow2 uninitialized.

 We guess that it caused sporadic crashes on freeing an invalid pointer
 in response to "query-block" QMP command in
 visit_type_ImageInfoSpecificQCow2 with QapiDeallocVisitor.

 Although we have neither a solid proof nor a reproduction scenario,
 making sure the field is initialized appears a reasonable thing to do.

 Signed-off-by: Roman Kagan 
 ---
   block/qcow2.c | 2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
>> Oops; hit send too soon.  I added for-2.5? to the subject line,
>> because...
>>
 diff --git a/block/qcow2.c b/block/qcow2.c
 index 88f56c8..67c9d3d 100644
 --- a/block/qcow2.c
 +++ b/block/qcow2.c
 @@ -2739,7 +2739,7 @@ static ImageInfoSpecific
 *qcow2_get_specific_info(BlockDriverState *bs)
 *spec_info = (ImageInfoSpecific){
   .type  = IMAGE_INFO_SPECIFIC_KIND_QCOW2,
 -.u.qcow2 = g_new(ImageInfoSpecificQCow2, 1),
 +.u.qcow2 = g_new0(ImageInfoSpecificQCow2, 1),
>>> NACK.  This makes no difference, except when s->qcow_version is out
>>> of spec.
>>>
   };
   if (s->qcow_version == 2) {
   *spec_info->u.qcow2 = (ImageInfoSpecificQCow2){

>>> If s->qcow_version is exactly 2, then we end up initializing all fields
>>> due to the assignment here; same if qcow_version is exactly 3.  The only
>>> time qcow2 remains uninitialized is if qcow_version is 0, 1, or > 3; but
>>> we refuse to handle qcow files with out-of-range versions.  So I don't
>>> see how you are plugging any uninitialized values; and therefore, I
>>> don't see how this is patching any crashes.
>> ...if you can prove that we aren't gracefully handling an out-of-spec
>> qcow_version, such that the uninitialized memory in that scenario is
>> indeed causing a crash, then it is worth respinning a v2 of this patch
>> with that proof, and worth considering it for 2.5 if it really is a
>> crash fixer.

More or less unfortunately, Eric is right. I chose a g_new() over
g_new0() because I was using compound literals anyway (and members not
explicitly initialized in the initializer list given for a compound
literal are initialized implicitly like object with static storage
duration, i.e. 0, generally).

s->qcow_version is always set to 2 or 3. It is only set in:
(1) qcow2_open(): Directly before it is set, we check that the version
is either 2 or 3.
(2) qcow2_downgrade(): We make sure that target_version (the value
s->qcow_version is set to) is equal to 2.
(3) qcow2_downgrade(): On error, s->qcow_version may be reset to
current_version, which is the old value of s->qcow_version which we
can inductively assume to be either 2 or 3.
(4) qcow2_amend_options(): On version upgrade, s->qcow_version is
overwritten by new_version, which is either 2, 3, or the old value
of s->qcow_version (in practice it is always 3 because it needs to
be greater than s->qcow_version in order to get here).
(5) qcow2_amend_options(): On version upgrade error, s->qcow_version is
reset to the old value of s->qcow_version, which we can again assume
to be 2 or 3.

I'd be completely fine with adding an "else { abort(); }" branch to
qcow2_get_specific_info(). But I fear that the issue you encountered is
caused by something different than the ImageInfoSpecificQCow2 object not
being fully initialized, and therefore I'm against this patch even if it
should not change anything (because it might make as feel as if we found
the issue even though there (most probably) is none here).

Also...

> Here is an info about our crash.
> 
> we have this crash under unknown conditions on RHEV version of QEMU.
> 
> Sorry, there is no much additional info. For the 

[...]

> which looks like
> 
> More specifically (expanding inlines along the stack trace):
> 
> (gdb) l *(visit_type_ImageInfoSpecificQCow2+169)
> 0x1a71e9 is in visit_type_ImageInfoSpecificQCow2 (qapi-visit.c:552).
> 548 static void visit_type_ImageInfoSpecificQCow2_fields(Visitor *m,
> ImageInfoSpecificQCow2 **obj, Error **errp)
> 549 {
> 550 Error *err = NULL;
> 551 ==> visit_type_str(m, &(*obj)->compat, "compat", );

...the compat field is always set, in either code path (both for version
2 and 3). Therefore, if this pointer is wrong, it definitely is not due
to the field not being initialized.

Without any way of reproducing, it is difficult to find the true cause
of the issue, though. valgrind would probably tell us something, but for
that we'd need something it