[Qemu-block] [PATCH v1 1/1] vmstate: fix failed iotests case 68 and 91

[QingFeng Hao]

This problem affects s390x only if we are running without KVM.
Basically, S390CPU.irqstate is unused if we do not use KVM,
and thus no buffer is allocated.
This causes size=0, first_elem=NULL and n_elems=1 in
vmstate_load_state and vmstate_save_state. And the assert fails.
With this fix we can go back to the old behavior and support
VMS_VBUFFER with size 0 and nullptr.

Signed-off-by: QingFeng Hao 
Signed-off-by: Halil Pasic 
---
 migration/vmstate.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/migration/vmstate.c b/migration/vmstate.c
index 78b3cd4..7b4a607 100644
--- a/migration/vmstate.c
+++ b/migration/vmstate.c
@@ -109,7 +109,7 @@ int vmstate_load_state(QEMUFile *f, const 
VMStateDescription *vmsd,
 vmstate_handle_alloc(first_elem, field, opaque);
 if (field->flags & VMS_POINTER) {
 first_elem = *(void **)first_elem;
-assert(first_elem  || !n_elems);
+assert(first_elem || !n_elems || !size);
 }
 for (i = 0; i < n_elems; i++) {
 void *curr_elem = first_elem + size * i;
@@ -117,7 +117,7 @@ int vmstate_load_state(QEMUFile *f, const 
VMStateDescription *vmsd,
 if (field->flags & VMS_ARRAY_OF_POINTER) {
 curr_elem = *(void **)curr_elem;
 }
-if (!curr_elem) {
+if (!curr_elem && size) {
 /* if null pointer check placeholder and do not follow */
 assert(field->flags & VMS_ARRAY_OF_POINTER);
 ret = vmstate_info_nullptr.get(f, curr_elem, size, NULL);
@@ -325,7 +325,7 @@ void vmstate_save_state(QEMUFile *f, const 
VMStateDescription *vmsd,
 trace_vmstate_save_state_loop(vmsd->name, field->name, n_elems);
 if (field->flags & VMS_POINTER) {
 first_elem = *(void **)first_elem;
-assert(first_elem  || !n_elems);
+assert(first_elem || !n_elems || !size);
 }
 for (i = 0; i < n_elems; i++) {
 void *curr_elem = first_elem + size * i;
@@ -336,7 +336,7 @@ void vmstate_save_state(QEMUFile *f, const 
VMStateDescription *vmsd,
 assert(curr_elem);
 curr_elem = *(void **)curr_elem;
 }
-if (!curr_elem) {
+if (!curr_elem && size) {
 /* if null pointer write placeholder and do not follow */
 assert(field->flags & VMS_ARRAY_OF_POINTER);
 vmstate_info_nullptr.put(f, curr_elem, size, NULL, NULL);
-- 
1.8.3.1

[Qemu-block] [PATCH v1 0/1] vmstate: fix failed iotests case 68 and 91

[QingFeng Hao]

Hi All,
This patch is to fix the failed iotests case 68 and 91 and has been
tested. It's based on commit dd4d2578215 "Merge remote-tracking branch
'remotes/kraxel/tags/pull-fixes-20170309-1' into staging" and according
to Halil and Dave's comments. Also thanks for Fam and Kevin.

QingFeng Hao (1):
  vmstate: fix failed iotests case 68 and 91

 migration/vmstate.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

-- 
1.8.3.1

Re: [Qemu-block] [PATCH v3 5/6] replication: Implement block replication for shared disk case

[Stefan Hajnoczi]

On Tue, Mar 07, 2017 at 10:30:30PM +0800, Hailiang Zhang wrote:
> Hi Stefan,
> 
> Sorry for the delayed reply.
> 
> On 2017/2/28 1:37, Stefan Hajnoczi wrote:
> > On Fri, Jan 20, 2017 at 11:47:59AM +0800, zhanghailiang wrote:
> > > Just as the scenario of non-shared disk block replication,
> > > we are going to implement block replication from many basic
> > > blocks that are already in QEMU.
> > > The architecture is:
> > > 
> > >   virtio-blk ||   
> > > .--
> > >   /  ||   
> > > | Secondary
> > >  /   ||   
> > > '--
> > > /||   
> > >   virtio-blk
> > >/ ||   
> > >|
> > >| ||   
> > > replication(5)
> > >|NBD  >   NBD   (2)
> > >|
> > >|  client ||server ---> hidden disk 
> > > <-- active disk(4)
> > >| ^   ||  |
> > >|  replication(1) ||  |
> > >| |   ||  |
> > >|   +-'   ||  |
> > >   (3)  |drive-backup sync=none   ||  |
> > > . |   +-+   ||  |
> > > Primary | | |   ||   backing|
> > > ' | |   ||  |
> > >V |   |
> > > +---+|
> > > |   shared disk | <--+
> > > +---+
> > > 
> > >  1) Primary writes will read original data and forward it to Secondary
> > > QEMU.
> > >  2) The hidden-disk is created automatically. It buffers the original 
> > > content
> > > that is modified by the primary VM. It should also be an empty 
> > > disk, and
> > > the driver supports bdrv_make_empty() and backing file.
> > >  3) Primary write requests will be written to Shared disk.
> > >  4) Secondary write requests will be buffered in the active disk and 
> > > it
> > > will overwrite the existing sector content in the buffer.
> > > 
> > > Signed-off-by: zhanghailiang 
> > > Signed-off-by: Wen Congyang 
> > > Signed-off-by: Zhang Chen 
> > 
> > Are there any restrictions on the shared disk?  For example the -drive
> > cache= mode must be 'none'.  If the cache mode isn't 'none' the
> > secondary host might have old data in the host page cache.  The
> 
> While do checkpoint, we will call vm_stop(), in which, the bdrv_flush_all()
> will be called, is it enough ?
> 
> > Secondary QEMU would have an inconsistent view of the shared disk.
> > 
> > Are image file formats like qcow2 supported for the shared disk?  Extra
> 
> In the above scenario, it has no limitation of formats for the shared disk.
> 
> > steps are required to achieve consistency, see bdrv_invalidate_cache().
> > 
> 
> Hmm, in that case, we should call bdrv_invalidate_cache_all() while 
> checkpoint.

Yes, it's not enough to just call bdrv_drain_all()/bdrv_flush_all().
The Secondary may need to reread metadata that is loaded in memory (e.g.
qcow2's L2 table cache) so bdrv_invalidate_cache() is needed.

Stefan


signature.asc
Description: PGP signature

Re: [Qemu-block] [Qemu-devel] [PATCH for-2.9] mirror: Fix backwards mirror_yield parameters

[Eric Blake]

[adding Stefan in cc, as trace maintainer]

On 03/09/2017 09:15 PM, Eric Blake wrote:

Perhaps I should update the subject to mention trace?

> trace-events lists the parameters for mirror_yield consistently
> with other events (cnt just after s, like in mirror_before_sleep;
> in_flight last, like in mirror_yield_in_flight).  But the callers
> were passing parameters in the wrong order, leading to poor trace
> messages, including type truncation when there are more than 4G
> dirty sectors involved.
> 
> Signed-off-by: Eric Blake 
> ---
>  block/mirror.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 4f3a5cb..1c403c5 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -633,7 +633,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob 
> *s)
>  }
> 
>  if (s->in_flight >= MAX_IN_FLIGHT) {
> -trace_mirror_yield(s, s->in_flight, s->buf_free_count, -1);
> +trace_mirror_yield(s, -1, s->buf_free_count, s->in_flight);
>  mirror_wait_for_io(s);
>  continue;
>  }
> @@ -808,7 +808,7 @@ static void coroutine_fn mirror_run(void *opaque)
>  s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
>  if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
>  (cnt == 0 && s->in_flight > 0)) {
> -trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
> +trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight);
>  mirror_wait_for_io(s);
>  continue;
>  } else if (cnt != 0) {
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-block] [PATCH for-2.9] mirror: Fix backwards mirror_yield parameters

[Eric Blake]

trace-events lists the parameters for mirror_yield consistently
with other events (cnt just after s, like in mirror_before_sleep;
in_flight last, like in mirror_yield_in_flight).  But the callers
were passing parameters in the wrong order, leading to poor trace
messages, including type truncation when there are more than 4G
dirty sectors involved.

Signed-off-by: Eric Blake 
---
 block/mirror.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 4f3a5cb..1c403c5 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -633,7 +633,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 }

 if (s->in_flight >= MAX_IN_FLIGHT) {
-trace_mirror_yield(s, s->in_flight, s->buf_free_count, -1);
+trace_mirror_yield(s, -1, s->buf_free_count, s->in_flight);
 mirror_wait_for_io(s);
 continue;
 }
@@ -808,7 +808,7 @@ static void coroutine_fn mirror_run(void *opaque)
 s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
 if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
 (cnt == 0 && s->in_flight > 0)) {
-trace_mirror_yield(s, s->in_flight, s->buf_free_count, cnt);
+trace_mirror_yield(s, cnt, s->buf_free_count, s->in_flight);
 mirror_wait_for_io(s);
 continue;
 } else if (cnt != 0) {
-- 
2.9.3

Re: [Qemu-block] [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[QingFeng Hao]

在 2017/3/9 19:45, Halil Pasic 写道:


On 03/09/2017 03:55 AM, QingFeng Hao wrote:


在 2017/3/8 19:33, Halil Pasic 写道:

On 03/08/2017 08:05 AM, QingFeng Hao wrote:

在 2017/3/7 18:19, Halil Pasic 写道:

On 03/07/2017 11:05 AM, Kevin Wolf wrote:

Am 07.03.2017 um 10:54 hat Halil Pasic geschrieben:

On 03/07/2017 10:29 AM, Kevin Wolf wrote:

Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:

[..]

The question is: can we do that change and what the second assert of field's 
flags is used for?

The assert is there to guard against unintended use of this nullptr-marker
mechanism.

I have tested so this should work. By the way a vmstate test covering this
corner-case would be a good idea too (IMHO). Would you like to write one?

Sorry, I don't know how to write that test case. Can you please write one?
thanks!

The test is just nice to have, but we definitely need the fix!

Ok, I'll send out the discussed fix patch.


Halil


--
Regards
QingFeng Hao

Re: [Qemu-block] [Qemu-devel] [PATCH 0/6] block: More op blocker fixes

[Kevin Wolf]

Am 09.03.2017 um 15:23 hat Eric Blake geschrieben:
> On 03/09/2017 05:38 AM, Kevin Wolf wrote:
> > Kevin Wolf (6):
> >   block: Remove check_new_perm from bdrv_replace_child()
> >   block: Request block status from *file for BDRV_BLOCK_RAW
> >   commit: Implement bdrv_commit_top.bdrv_co_get_block_status
> >   block: Refresh filename after changing backing file
> >   mirror: Implement .bdrv_refresh_filename
> >   commit: Implement .bdrv_refresh_filename
> > 
> 
> Reviewed-by: Eric Blake 

Applied to the block branch.

Kevin


pgpQO8KSIdnXH.pgp
Description: PGP signature

Re: [Qemu-block] [PATCH for-2.9 0/3] Fix bdrv_is_allocated usage bugs

[Kevin Wolf]

Am 08.03.2017 um 22:34 hat Eric Blake geschrieben:
> bdrv_is_allocated() returns tri-state, not just bool, although
> there were several callers using it as a bool.  Fix them to
> either propagate the error or to document why treatment of
> failure like allocation is okay.
> 
> [Found during a larger effort to convert bdrv_get_block_status
> to be byte-based for 2.10]

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH 0/6] block: More op blocker fixes

[Eric Blake]

On 03/09/2017 05:38 AM, Kevin Wolf wrote:
> Kevin Wolf (6):
>   block: Remove check_new_perm from bdrv_replace_child()
>   block: Request block status from *file for BDRV_BLOCK_RAW
>   commit: Implement bdrv_commit_top.bdrv_co_get_block_status
>   block: Refresh filename after changing backing file
>   mirror: Implement .bdrv_refresh_filename
>   commit: Implement .bdrv_refresh_filename
> 

Reviewed-by: Eric Blake 

>  block.c| 23 ---
>  block/commit.c | 28 
>  block/io.c |  2 +-
>  block/mirror.c |  9 +
>  4 files changed, 50 insertions(+), 12 deletions(-)
> 

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-block] [PATCH for-2.9] block: Drop unmaintained 'archipelago' driver

[Kevin Wolf]

Am 08.03.2017 um 21:02 hat Eric Blake geschrieben:
> The driver has failed to build since commit da34e65, in qemu 2.6,
> due to a missing include of qapi/error.h for error_setg().
> Since no one has complained in three releases, it is easier to
> remove the dead code than to keep it around, especially since it
> is not being built by default and therefore prone to bitrot.
> 
> Signed-off-by: Eric Blake 

Thanks, applied to the block branch.

Kevin

Re: [Qemu-block] [Qemu-devel] [PATCH RFC 1/1] vmstate: draft fix for failed iotests case 68 and 91

[Halil Pasic]

On 03/09/2017 03:55 AM, QingFeng Hao wrote:
> 
> 
> 在 2017/3/8 19:33, Halil Pasic 写道:
>>
>> On 03/08/2017 08:05 AM, QingFeng Hao wrote:
>>>
>>> 在 2017/3/7 18:19, Halil Pasic 写道:
 On 03/07/2017 11:05 AM, Kevin Wolf wrote:
> Am 07.03.2017 um 10:54 hat Halil Pasic geschrieben:
>> On 03/07/2017 10:29 AM, Kevin Wolf wrote:
>>> Am 07.03.2017 um 03:53 hat QingFeng Hao geschrieben:
[..]
>>> The question is: can we do that change and what the second assert of 
>>> field's flags is used for?
>> The assert is there to guard against unintended use of this nullptr-marker
>> mechanism.
>>
>> I have tested so this should work. By the way a vmstate test covering this
>> corner-case would be a good idea too (IMHO). Would you like to write one?
> Sorry, I don't know how to write that test case. Can you please write one?
> thanks!

The test is just nice to have, but we definitely need the fix!

Halil

[Qemu-block] [PATCH 6/6] commit: Implement .bdrv_refresh_filename

[Kevin Wolf]

We want query-block to return the right filename, even if a commit job
put a bdrv_commit_top on top of the actual image format driver. Let
bdrv_commit_top.bdrv_refresh_filename get the filename from its backing
file.

Signed-off-by: Kevin Wolf 
---
 block/commit.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/block/commit.c b/block/commit.c
index 932d1e6..2832482 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -13,6 +13,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "trace.h"
 #include "block/block_int.h"
 #include "block/blockjob_int.h"
@@ -242,6 +243,12 @@ static int64_t coroutine_fn 
bdrv_commit_top_get_block_status(
(sector_num << BDRV_SECTOR_BITS);
 }
 
+static void bdrv_commit_top_refresh_filename(BlockDriverState *bs, QDict *opts)
+{
+bdrv_refresh_filename(bs->backing->bs);
+pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
+bs->backing->bs->filename);
+}
 
 static void bdrv_commit_top_close(BlockDriverState *bs)
 {
@@ -262,6 +269,7 @@ static BlockDriver bdrv_commit_top = {
 .format_name= "commit_top",
 .bdrv_co_preadv = bdrv_commit_top_preadv,
 .bdrv_co_get_block_status   = bdrv_commit_top_get_block_status,
+.bdrv_refresh_filename  = bdrv_commit_top_refresh_filename,
 .bdrv_close = bdrv_commit_top_close,
 .bdrv_child_perm= bdrv_commit_top_child_perm,
 };
-- 
1.8.3.1

[Qemu-block] [PATCH 0/6] block: More op blocker fixes

[Kevin Wolf]

Kevin Wolf (6):
  block: Remove check_new_perm from bdrv_replace_child()
  block: Request block status from *file for BDRV_BLOCK_RAW
  commit: Implement bdrv_commit_top.bdrv_co_get_block_status
  block: Refresh filename after changing backing file
  mirror: Implement .bdrv_refresh_filename
  commit: Implement .bdrv_refresh_filename

 block.c| 23 ---
 block/commit.c | 28 
 block/io.c |  2 +-
 block/mirror.c |  9 +
 4 files changed, 50 insertions(+), 12 deletions(-)

-- 
1.8.3.1

[Qemu-block] [PATCH 4/6] block: Refresh filename after changing backing file

[Kevin Wolf]

In bdrv_open_inherit(), the filename is refreshed after opening the
backing file, but we neglected to do the same when the backing file
changes later.

Signed-off-by: Kevin Wolf 
---
 block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block.c b/block.c
index 756d607..516cefe 100644
--- a/block.c
+++ b/block.c
@@ -1933,6 +1933,8 @@ void bdrv_set_backing_hd(BlockDriverState *bs, 
BlockDriverState *backing_hd,
 bdrv_unref(backing_hd);
 }
 
+bdrv_refresh_filename(bs);
+
 out:
 bdrv_refresh_limits(bs, NULL);
 }
-- 
1.8.3.1

[Qemu-block] [PATCH 5/6] mirror: Implement .bdrv_refresh_filename

[Kevin Wolf]

We want query-block to return the right filename, even if a mirror job
put a bdrv_mirror_top on top of the actual image format driver. Let
bdrv_mirror_top.bdrv_refresh_filename get the filename from its backing
file.

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

diff --git a/block/mirror.c b/block/mirror.c
index a5d30ee..4f3a5cb 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -12,6 +12,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 #include "trace.h"
 #include "block/blockjob_int.h"
 #include "block/block_int.h"
@@ -1060,6 +1061,13 @@ static int coroutine_fn 
bdrv_mirror_top_pdiscard(BlockDriverState *bs,
 return bdrv_co_pdiscard(bs->backing->bs, offset, count);
 }
 
+static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs, QDict *opts)
+{
+bdrv_refresh_filename(bs->backing->bs);
+pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
+bs->backing->bs->filename);
+}
+
 static void bdrv_mirror_top_close(BlockDriverState *bs)
 {
 }
@@ -1088,6 +1096,7 @@ static BlockDriver bdrv_mirror_top = {
 .bdrv_co_pdiscard   = bdrv_mirror_top_pdiscard,
 .bdrv_co_flush  = bdrv_mirror_top_flush,
 .bdrv_co_get_block_status   = bdrv_mirror_top_get_block_status,
+.bdrv_refresh_filename  = bdrv_mirror_top_refresh_filename,
 .bdrv_close = bdrv_mirror_top_close,
 .bdrv_child_perm= bdrv_mirror_top_child_perm,
 };
-- 
1.8.3.1

[Qemu-block] [PATCH 3/6] commit: Implement bdrv_commit_top.bdrv_co_get_block_status

[Kevin Wolf]

In some cases, bdrv_co_get_block_status() is called recursively for the
whole backing chain. The automatically inserted bdrv_commit_top filter
driver must not stop the recursion, so implement a callback that simply
forwards the request to bs->backing.

Signed-off-by: Kevin Wolf 
---
 block/commit.c | 20 
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/block/commit.c b/block/commit.c
index 9c41988..932d1e6 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -232,6 +232,17 @@ static int coroutine_fn 
bdrv_commit_top_preadv(BlockDriverState *bs,
 return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags);
 }
 
+static int64_t coroutine_fn bdrv_commit_top_get_block_status(
+BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum,
+BlockDriverState **file)
+{
+*pnum = nb_sectors;
+*file = bs->backing->bs;
+return BDRV_BLOCK_RAW | BDRV_BLOCK_OFFSET_VALID | BDRV_BLOCK_DATA |
+   (sector_num << BDRV_SECTOR_BITS);
+}
+
+
 static void bdrv_commit_top_close(BlockDriverState *bs)
 {
 }
@@ -248,10 +259,11 @@ static void bdrv_commit_top_child_perm(BlockDriverState 
*bs, BdrvChild *c,
 /* Dummy node that provides consistent read to its users without requiring it
  * from its backing file and that allows writes on the backing file chain. */
 static BlockDriver bdrv_commit_top = {
-.format_name= "commit_top",
-.bdrv_co_preadv = bdrv_commit_top_preadv,
-.bdrv_close = bdrv_commit_top_close,
-.bdrv_child_perm= bdrv_commit_top_child_perm,
+.format_name= "commit_top",
+.bdrv_co_preadv = bdrv_commit_top_preadv,
+.bdrv_co_get_block_status   = bdrv_commit_top_get_block_status,
+.bdrv_close = bdrv_commit_top_close,
+.bdrv_child_perm= bdrv_commit_top_child_perm,
 };
 
 void commit_start(const char *job_id, BlockDriverState *bs,
-- 
1.8.3.1

[Qemu-block] [PATCH 2/6] block: Request block status from *file for BDRV_BLOCK_RAW

[Kevin Wolf]

This fixes bdrv_co_get_block_status() for the bdrv_mirror_top block
driver, which must fall through to bs->backing instead of bs->file.

Signed-off-by: Kevin Wolf 
---
 block/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index 8f38d46..2709a70 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1760,7 +1760,7 @@ static int64_t coroutine_fn 
bdrv_co_get_block_status(BlockDriverState *bs,
 
 if (ret & BDRV_BLOCK_RAW) {
 assert(ret & BDRV_BLOCK_OFFSET_VALID);
-ret = bdrv_get_block_status(bs->file->bs, ret >> BDRV_SECTOR_BITS,
+ret = bdrv_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
 *pnum, pnum, file);
 goto out;
 }
-- 
1.8.3.1

[Qemu-block] [PATCH 1/6] block: Remove check_new_perm from bdrv_replace_child()

[Kevin Wolf]

All callers pass false now, so the parameter can go away again.

Signed-off-by: Kevin Wolf 
---
 block.c | 21 ++---
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index dd9ded8..756d607 100644
--- a/block.c
+++ b/block.c
@@ -1751,8 +1751,18 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
 }
 }
 
-static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs,
-   bool check_new_perm)
+/*
+ * Updates @child to change its reference to point to @new_bs, including
+ * checking and applying the necessary permisson updates both to the old node
+ * and to @new_bs.
+ *
+ * NULL is passed as @new_bs for removing the reference before freeing @child.
+ *
+ * If @new_bs is not NULL, bdrv_check_perm() must be called beforehand, as this
+ * function uses bdrv_set_perm() to update the permissions according to the new
+ * reference that @new_bs gets.
+ */
+static void bdrv_replace_child(BdrvChild *child, BlockDriverState *new_bs)
 {
 BlockDriverState *old_bs = child->bs;
 uint64_t perm, shared_perm;
@@ -1770,9 +1780,6 @@ static void bdrv_replace_child(BdrvChild *child, 
BlockDriverState *new_bs,
 
 if (new_bs) {
 bdrv_get_cumulative_perm(new_bs, , _perm);
-if (check_new_perm) {
-bdrv_check_perm(new_bs, perm, shared_perm, NULL, _abort);
-}
 bdrv_set_perm(new_bs, perm, shared_perm);
 }
 }
@@ -1803,7 +1810,7 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState 
*child_bs,
 };
 
 /* This performs the matching bdrv_set_perm() for the above check. */
-bdrv_replace_child(child, child_bs, false);
+bdrv_replace_child(child, child_bs);
 
 return child;
 }
@@ -1840,7 +1847,7 @@ static void bdrv_detach_child(BdrvChild *child)
 child->next.le_prev = NULL;
 }
 
-bdrv_replace_child(child, NULL, false);
+bdrv_replace_child(child, NULL);
 
 g_free(child->name);
 g_free(child);
-- 
1.8.3.1