Re: [Qemu-block] [PATCH v6 35/42] block: Fix check_to_replace_node()
16.08.2019 16:30, Max Reitz wrote:
> On 16.08.19 13:01, Vladimir Sementsov-Ogievskiy wrote:
>> 15.08.2019 20:01, Max Reitz wrote:
>>> On 15.08.19 17:21, Vladimir Sementsov-Ogievskiy wrote:
09.08.2019 19:14, Max Reitz wrote:
> Currently, check_to_replace_node() only allows mirror to replace a node
> in the chain of the source node, and only if it is the first non-filter
> node below the source. Well, technically, the idea is that you can
> exactly replace a quorum child by mirroring from quorum.
>
> This has (probably) two reasons:
> (1) We do not want to create loops.
> (2) @replaces and @device should have exactly the same content so
>replacing them does not cause visible data to change.
>
> This has two issues:
> (1) It is overly restrictive. It is completely fine for @replaces to be
>a filter.
> (2) It is not restrictive enough. You can create loops with this as
>follows:
>
> $ qemu-img create -f qcow2 /tmp/source.qcow2 64M
> $ qemu-system-x86_64 -qmp stdio
> {"execute": "qmp_capabilities"}
> {"execute": "object-add",
> "arguments": {"qom-type": "throttle-group", "id": "tg0"}}
> {"execute": "blockdev-add",
> "arguments": {
> "node-name": "source",
> "driver": "throttle",
> "throttle-group": "tg0",
> "file": {
> "node-name": "filtered",
> "driver": "qcow2",
> "file": {
> "driver": "file",
> "filename": "/tmp/source.qcow2"
> } } } }
> {"execute": "drive-mirror",
> "arguments": {
> "job-id": "mirror",
> "device": "source",
> "target": "/tmp/target.qcow2",
> "format": "qcow2",
> "node-name": "target",
> "sync" :"none",
> "replaces": "filtered"
> } }
> {"execute": "block-job-complete", "arguments": {"device": "mirror"}}
>
> And qemu crashes because of a stack overflow due to the loop being
> created (target's backing file is source, so when it replaces filtered,
> it points to itself through source).
>
> (blockdev-mirror can be broken similarly.)
>
> So let us make the checks for the two conditions above explicit, which
> makes the whole function exactly as restrictive as it needs to be.
>
> Signed-off-by: Max Reitz
> ---
> include/block/block.h | 1 +
> block.c | 83 +++
> blockdev.c| 34 --
> 3 files changed, 110 insertions(+), 8 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 6ba853fb90..8da706cd89 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -404,6 +404,7 @@ bool bdrv_is_first_non_filter(BlockDriverState
> *candidate);
>
> /* check if a named node can be replaced when doing drive-mirror */
> BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
> +BlockDriverState *backing_bs,
> const char *node_name, Error
> **errp);
>
> /* async block I/O */
> diff --git a/block.c b/block.c
> index 915b80153c..4858d3e718 100644
> --- a/block.c
> +++ b/block.c
> @@ -6290,7 +6290,59 @@ bool bdrv_is_first_non_filter(BlockDriverState
> *candidate)
> return false;
> }
>
> +static bool is_child_of(BlockDriverState *child, BlockDriverState
> *parent)
> +{
> +BdrvChild *c;
> +
> +if (!parent) {
> +return false;
> +}
> +
> +QLIST_FOREACH(c, >children, next) {
> +if (c->bs == child || is_child_of(child, c->bs)) {
> +return true;
> +}
> +}
> +
> +return false;
> +}
> +
> +/*
> + * Return true if there are only filters in [@top, @base). Note that
> + * this may include quorum (which bdrv_chain_contains() cannot
> + * handle).
More presizely: return true if exists chain of filters from top to base or
if
top == base.
I keep in mind backup-top filter:
[backup-top]
| \target
>>>
>>> backup-top can’t be a filter if it has two children with different
>>> contents, though.
>>
>> Why? target is special child, unrelated to what is read/written over
>> backup-top.
>> It's an own business of backup-top.
>>
>>>
>>> (commit-top and mirror-top aren’t filters either.)
>>
>> Ahm, I missed something. They have is_filter = true and their children
>> considered
>> to be filtered-rw children in your series? And than, who they are? Format
>> nodes?
>> And how they appears in backing chains than?
>
> Er,
Re: [Qemu-block] [PATCH v6 35/42] block: Fix check_to_replace_node()
On 16.08.19 13:01, Vladimir Sementsov-Ogievskiy wrote:
> 15.08.2019 20:01, Max Reitz wrote:
>> On 15.08.19 17:21, Vladimir Sementsov-Ogievskiy wrote:
>>> 09.08.2019 19:14, Max Reitz wrote:
Currently, check_to_replace_node() only allows mirror to replace a node
in the chain of the source node, and only if it is the first non-filter
node below the source. Well, technically, the idea is that you can
exactly replace a quorum child by mirroring from quorum.
This has (probably) two reasons:
(1) We do not want to create loops.
(2) @replaces and @device should have exactly the same content so
replacing them does not cause visible data to change.
This has two issues:
(1) It is overly restrictive. It is completely fine for @replaces to be
a filter.
(2) It is not restrictive enough. You can create loops with this as
follows:
$ qemu-img create -f qcow2 /tmp/source.qcow2 64M
$ qemu-system-x86_64 -qmp stdio
{"execute": "qmp_capabilities"}
{"execute": "object-add",
"arguments": {"qom-type": "throttle-group", "id": "tg0"}}
{"execute": "blockdev-add",
"arguments": {
"node-name": "source",
"driver": "throttle",
"throttle-group": "tg0",
"file": {
"node-name": "filtered",
"driver": "qcow2",
"file": {
"driver": "file",
"filename": "/tmp/source.qcow2"
} } } }
{"execute": "drive-mirror",
"arguments": {
"job-id": "mirror",
"device": "source",
"target": "/tmp/target.qcow2",
"format": "qcow2",
"node-name": "target",
"sync" :"none",
"replaces": "filtered"
} }
{"execute": "block-job-complete", "arguments": {"device": "mirror"}}
And qemu crashes because of a stack overflow due to the loop being
created (target's backing file is source, so when it replaces filtered,
it points to itself through source).
(blockdev-mirror can be broken similarly.)
So let us make the checks for the two conditions above explicit, which
makes the whole function exactly as restrictive as it needs to be.
Signed-off-by: Max Reitz
---
include/block/block.h | 1 +
block.c | 83 +++
blockdev.c| 34 --
3 files changed, 110 insertions(+), 8 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 6ba853fb90..8da706cd89 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -404,6 +404,7 @@ bool bdrv_is_first_non_filter(BlockDriverState
*candidate);
/* check if a named node can be replaced when doing drive-mirror */
BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
+BlockDriverState *backing_bs,
const char *node_name, Error
**errp);
/* async block I/O */
diff --git a/block.c b/block.c
index 915b80153c..4858d3e718 100644
--- a/block.c
+++ b/block.c
@@ -6290,7 +6290,59 @@ bool bdrv_is_first_non_filter(BlockDriverState
*candidate)
return false;
}
+static bool is_child_of(BlockDriverState *child, BlockDriverState *parent)
+{
+BdrvChild *c;
+
+if (!parent) {
+return false;
+}
+
+QLIST_FOREACH(c, >children, next) {
+if (c->bs == child || is_child_of(child, c->bs)) {
+return true;
+}
+}
+
+return false;
+}
+
+/*
+ * Return true if there are only filters in [@top, @base). Note that
+ * this may include quorum (which bdrv_chain_contains() cannot
+ * handle).
>>>
>>> More presizely: return true if exists chain of filters from top to base or
>>> if
>>> top == base.
>>>
>>> I keep in mind backup-top filter:
>>>
>>> [backup-top]
>>> | \target
>>
>> backup-top can’t be a filter if it has two children with different
>> contents, though.
>
> Why? target is special child, unrelated to what is read/written over
> backup-top.
> It's an own business of backup-top.
>
>>
>> (commit-top and mirror-top aren’t filters either.)
>
> Ahm, I missed something. They have is_filter = true and their children
> considered
> to be filtered-rw children in your series? And than, who they are? Format
> nodes?
> And how they appears in backing chains than?
Er, right, I remember, I made them filters in patch 1 of this series. m( :-)
But the chain would still be unique, in a sense, because backup-top only
has one filtered child, so you could go down the chain with
Re: [Qemu-block] [PATCH v6 35/42] block: Fix check_to_replace_node()
15.08.2019 20:01, Max Reitz wrote:
> On 15.08.19 17:21, Vladimir Sementsov-Ogievskiy wrote:
>> 09.08.2019 19:14, Max Reitz wrote:
>>> Currently, check_to_replace_node() only allows mirror to replace a node
>>> in the chain of the source node, and only if it is the first non-filter
>>> node below the source. Well, technically, the idea is that you can
>>> exactly replace a quorum child by mirroring from quorum.
>>>
>>> This has (probably) two reasons:
>>> (1) We do not want to create loops.
>>> (2) @replaces and @device should have exactly the same content so
>>> replacing them does not cause visible data to change.
>>>
>>> This has two issues:
>>> (1) It is overly restrictive. It is completely fine for @replaces to be
>>> a filter.
>>> (2) It is not restrictive enough. You can create loops with this as
>>> follows:
>>>
>>> $ qemu-img create -f qcow2 /tmp/source.qcow2 64M
>>> $ qemu-system-x86_64 -qmp stdio
>>> {"execute": "qmp_capabilities"}
>>> {"execute": "object-add",
>>>"arguments": {"qom-type": "throttle-group", "id": "tg0"}}
>>> {"execute": "blockdev-add",
>>>"arguments": {
>>>"node-name": "source",
>>>"driver": "throttle",
>>>"throttle-group": "tg0",
>>>"file": {
>>>"node-name": "filtered",
>>>"driver": "qcow2",
>>>"file": {
>>>"driver": "file",
>>>"filename": "/tmp/source.qcow2"
>>>} } } }
>>> {"execute": "drive-mirror",
>>>"arguments": {
>>>"job-id": "mirror",
>>>"device": "source",
>>>"target": "/tmp/target.qcow2",
>>>"format": "qcow2",
>>>"node-name": "target",
>>>"sync" :"none",
>>>"replaces": "filtered"
>>>} }
>>> {"execute": "block-job-complete", "arguments": {"device": "mirror"}}
>>>
>>> And qemu crashes because of a stack overflow due to the loop being
>>> created (target's backing file is source, so when it replaces filtered,
>>> it points to itself through source).
>>>
>>> (blockdev-mirror can be broken similarly.)
>>>
>>> So let us make the checks for the two conditions above explicit, which
>>> makes the whole function exactly as restrictive as it needs to be.
>>>
>>> Signed-off-by: Max Reitz
>>> ---
>>>include/block/block.h | 1 +
>>>block.c | 83 +++
>>>blockdev.c| 34 --
>>>3 files changed, 110 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/include/block/block.h b/include/block/block.h
>>> index 6ba853fb90..8da706cd89 100644
>>> --- a/include/block/block.h
>>> +++ b/include/block/block.h
>>> @@ -404,6 +404,7 @@ bool bdrv_is_first_non_filter(BlockDriverState
>>> *candidate);
>>>
>>>/* check if a named node can be replaced when doing drive-mirror */
>>>BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
>>> +BlockDriverState *backing_bs,
>>>const char *node_name, Error
>>> **errp);
>>>
>>>/* async block I/O */
>>> diff --git a/block.c b/block.c
>>> index 915b80153c..4858d3e718 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -6290,7 +6290,59 @@ bool bdrv_is_first_non_filter(BlockDriverState
>>> *candidate)
>>>return false;
>>>}
>>>
>>> +static bool is_child_of(BlockDriverState *child, BlockDriverState *parent)
>>> +{
>>> +BdrvChild *c;
>>> +
>>> +if (!parent) {
>>> +return false;
>>> +}
>>> +
>>> +QLIST_FOREACH(c, >children, next) {
>>> +if (c->bs == child || is_child_of(child, c->bs)) {
>>> +return true;
>>> +}
>>> +}
>>> +
>>> +return false;
>>> +}
>>> +
>>> +/*
>>> + * Return true if there are only filters in [@top, @base). Note that
>>> + * this may include quorum (which bdrv_chain_contains() cannot
>>> + * handle).
>>
>> More presizely: return true if exists chain of filters from top to base or if
>> top == base.
>>
>> I keep in mind backup-top filter:
>>
>> [backup-top]
>> | \target
>
> backup-top can’t be a filter if it has two children with different
> contents, though.
Why? target is special child, unrelated to what is read/written over backup-top.
It's an own business of backup-top.
>
> (commit-top and mirror-top aren’t filters either.)
Ahm, I missed something. They have is_filter = true and their children
considered
to be filtered-rw children in your series? And than, who they are? Format nodes?
And how they appears in backing chains than?
>
> That’s why there must be a unique chain [@top, @base).
>
> I should probably not that it will return true if top == base, though, yes.
>
>> |backing>[target]
>> V/
>> [source] <-/backing
>>
>>> + */
>>> +static bool is_filtered_child(BlockDriverState *top, BlockDriverState
>>> *base)
>>> +{
>>> +BdrvChild *c;
>>> +
>>> +if (!top) {
>>> +return false;
>>> +}
Re: [Qemu-block] [PATCH v6 35/42] block: Fix check_to_replace_node()
On 15.08.19 17:21, Vladimir Sementsov-Ogievskiy wrote:
> 09.08.2019 19:14, Max Reitz wrote:
>> Currently, check_to_replace_node() only allows mirror to replace a node
>> in the chain of the source node, and only if it is the first non-filter
>> node below the source. Well, technically, the idea is that you can
>> exactly replace a quorum child by mirroring from quorum.
>>
>> This has (probably) two reasons:
>> (1) We do not want to create loops.
>> (2) @replaces and @device should have exactly the same content so
>> replacing them does not cause visible data to change.
>>
>> This has two issues:
>> (1) It is overly restrictive. It is completely fine for @replaces to be
>> a filter.
>> (2) It is not restrictive enough. You can create loops with this as
>> follows:
>>
>> $ qemu-img create -f qcow2 /tmp/source.qcow2 64M
>> $ qemu-system-x86_64 -qmp stdio
>> {"execute": "qmp_capabilities"}
>> {"execute": "object-add",
>> "arguments": {"qom-type": "throttle-group", "id": "tg0"}}
>> {"execute": "blockdev-add",
>> "arguments": {
>> "node-name": "source",
>> "driver": "throttle",
>> "throttle-group": "tg0",
>> "file": {
>> "node-name": "filtered",
>> "driver": "qcow2",
>> "file": {
>> "driver": "file",
>> "filename": "/tmp/source.qcow2"
>> } } } }
>> {"execute": "drive-mirror",
>> "arguments": {
>> "job-id": "mirror",
>> "device": "source",
>> "target": "/tmp/target.qcow2",
>> "format": "qcow2",
>> "node-name": "target",
>> "sync" :"none",
>> "replaces": "filtered"
>> } }
>> {"execute": "block-job-complete", "arguments": {"device": "mirror"}}
>>
>> And qemu crashes because of a stack overflow due to the loop being
>> created (target's backing file is source, so when it replaces filtered,
>> it points to itself through source).
>>
>> (blockdev-mirror can be broken similarly.)
>>
>> So let us make the checks for the two conditions above explicit, which
>> makes the whole function exactly as restrictive as it needs to be.
>>
>> Signed-off-by: Max Reitz
>> ---
>> include/block/block.h | 1 +
>> block.c | 83 +++
>> blockdev.c| 34 --
>> 3 files changed, 110 insertions(+), 8 deletions(-)
>>
>> diff --git a/include/block/block.h b/include/block/block.h
>> index 6ba853fb90..8da706cd89 100644
>> --- a/include/block/block.h
>> +++ b/include/block/block.h
>> @@ -404,6 +404,7 @@ bool bdrv_is_first_non_filter(BlockDriverState
>> *candidate);
>>
>> /* check if a named node can be replaced when doing drive-mirror */
>> BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
>> +BlockDriverState *backing_bs,
>> const char *node_name, Error
>> **errp);
>>
>> /* async block I/O */
>> diff --git a/block.c b/block.c
>> index 915b80153c..4858d3e718 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -6290,7 +6290,59 @@ bool bdrv_is_first_non_filter(BlockDriverState
>> *candidate)
>> return false;
>> }
>>
>> +static bool is_child_of(BlockDriverState *child, BlockDriverState *parent)
>> +{
>> +BdrvChild *c;
>> +
>> +if (!parent) {
>> +return false;
>> +}
>> +
>> +QLIST_FOREACH(c, >children, next) {
>> +if (c->bs == child || is_child_of(child, c->bs)) {
>> +return true;
>> +}
>> +}
>> +
>> +return false;
>> +}
>> +
>> +/*
>> + * Return true if there are only filters in [@top, @base). Note that
>> + * this may include quorum (which bdrv_chain_contains() cannot
>> + * handle).
>
> More presizely: return true if exists chain of filters from top to base or if
> top == base.
>
> I keep in mind backup-top filter:
>
> [backup-top]
> | \target
backup-top can’t be a filter if it has two children with different
contents, though.
(commit-top and mirror-top aren’t filters either.)
That’s why there must be a unique chain [@top, @base).
I should probably not that it will return true if top == base, though, yes.
> |backing>[target]
> V/
> [source] <-/backing
>
>> + */
>> +static bool is_filtered_child(BlockDriverState *top, BlockDriverState *base)
>> +{
>> +BdrvChild *c;
>> +
>> +if (!top) {
>> +return false;
>> +}
>> +
>> +if (top == base) {
>> +return true;
>> +}
>> +
>> +if (!top->drv->is_filter) {
>> +return false;
>> +}
>> +
>> +QLIST_FOREACH(c, >children, next) {
>> +if (is_filtered_child(c->bs, base)) {
>> +return true;
>> +}
>> +}
>
> interesting, how much is it better to somehow reuse DFS search written in
> should_update_child()..
> [just note, don't do it in these series please]
>
>> +
>> +return false;
>> +}
>> +
>> +/*
>> + * @parent_bs is mirror's source BDS,
Re: [Qemu-block] [PATCH v6 35/42] block: Fix check_to_replace_node()
09.08.2019 19:14, Max Reitz wrote:
> Currently, check_to_replace_node() only allows mirror to replace a node
> in the chain of the source node, and only if it is the first non-filter
> node below the source. Well, technically, the idea is that you can
> exactly replace a quorum child by mirroring from quorum.
>
> This has (probably) two reasons:
> (1) We do not want to create loops.
> (2) @replaces and @device should have exactly the same content so
> replacing them does not cause visible data to change.
>
> This has two issues:
> (1) It is overly restrictive. It is completely fine for @replaces to be
> a filter.
> (2) It is not restrictive enough. You can create loops with this as
> follows:
>
> $ qemu-img create -f qcow2 /tmp/source.qcow2 64M
> $ qemu-system-x86_64 -qmp stdio
> {"execute": "qmp_capabilities"}
> {"execute": "object-add",
> "arguments": {"qom-type": "throttle-group", "id": "tg0"}}
> {"execute": "blockdev-add",
> "arguments": {
> "node-name": "source",
> "driver": "throttle",
> "throttle-group": "tg0",
> "file": {
> "node-name": "filtered",
> "driver": "qcow2",
> "file": {
> "driver": "file",
> "filename": "/tmp/source.qcow2"
> } } } }
> {"execute": "drive-mirror",
> "arguments": {
> "job-id": "mirror",
> "device": "source",
> "target": "/tmp/target.qcow2",
> "format": "qcow2",
> "node-name": "target",
> "sync" :"none",
> "replaces": "filtered"
> } }
> {"execute": "block-job-complete", "arguments": {"device": "mirror"}}
>
> And qemu crashes because of a stack overflow due to the loop being
> created (target's backing file is source, so when it replaces filtered,
> it points to itself through source).
>
> (blockdev-mirror can be broken similarly.)
>
> So let us make the checks for the two conditions above explicit, which
> makes the whole function exactly as restrictive as it needs to be.
>
> Signed-off-by: Max Reitz
> ---
> include/block/block.h | 1 +
> block.c | 83 +++
> blockdev.c| 34 --
> 3 files changed, 110 insertions(+), 8 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 6ba853fb90..8da706cd89 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -404,6 +404,7 @@ bool bdrv_is_first_non_filter(BlockDriverState
> *candidate);
>
> /* check if a named node can be replaced when doing drive-mirror */
> BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
> +BlockDriverState *backing_bs,
> const char *node_name, Error
> **errp);
>
> /* async block I/O */
> diff --git a/block.c b/block.c
> index 915b80153c..4858d3e718 100644
> --- a/block.c
> +++ b/block.c
> @@ -6290,7 +6290,59 @@ bool bdrv_is_first_non_filter(BlockDriverState
> *candidate)
> return false;
> }
>
> +static bool is_child_of(BlockDriverState *child, BlockDriverState *parent)
> +{
> +BdrvChild *c;
> +
> +if (!parent) {
> +return false;
> +}
> +
> +QLIST_FOREACH(c, >children, next) {
> +if (c->bs == child || is_child_of(child, c->bs)) {
> +return true;
> +}
> +}
> +
> +return false;
> +}
> +
> +/*
> + * Return true if there are only filters in [@top, @base). Note that
> + * this may include quorum (which bdrv_chain_contains() cannot
> + * handle).
More presizely: return true if exists chain of filters from top to base or if
top == base.
I keep in mind backup-top filter:
[backup-top]
| \target
|backing>[target]
V/
[source] <-/backing
> + */
> +static bool is_filtered_child(BlockDriverState *top, BlockDriverState *base)
> +{
> +BdrvChild *c;
> +
> +if (!top) {
> +return false;
> +}
> +
> +if (top == base) {
> +return true;
> +}
> +
> +if (!top->drv->is_filter) {
> +return false;
> +}
> +
> +QLIST_FOREACH(c, >children, next) {
> +if (is_filtered_child(c->bs, base)) {
> +return true;
> +}
> +}
interesting, how much is it better to somehow reuse DFS search written in
should_update_child()..
[just note, don't do it in these series please]
> +
> +return false;
> +}
> +
> +/*
> + * @parent_bs is mirror's source BDS, @backing_bs is the BDS which
> + * will be attached to the target when mirror completes.
> + */
> BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
> +BlockDriverState *backing_bs,
> const char *node_name, Error **errp)
> {
> BlockDriverState *to_replace_bs = bdrv_find_node(node_name);
> @@ -6309,13 +6361,32 @@ BlockDriverState
> *check_to_replace_node(BlockDriverState *parent_bs,
[Qemu-block] [PATCH v6 35/42] block: Fix check_to_replace_node()
Currently, check_to_replace_node() only allows mirror to replace a node
in the chain of the source node, and only if it is the first non-filter
node below the source. Well, technically, the idea is that you can
exactly replace a quorum child by mirroring from quorum.
This has (probably) two reasons:
(1) We do not want to create loops.
(2) @replaces and @device should have exactly the same content so
replacing them does not cause visible data to change.
This has two issues:
(1) It is overly restrictive. It is completely fine for @replaces to be
a filter.
(2) It is not restrictive enough. You can create loops with this as
follows:
$ qemu-img create -f qcow2 /tmp/source.qcow2 64M
$ qemu-system-x86_64 -qmp stdio
{"execute": "qmp_capabilities"}
{"execute": "object-add",
"arguments": {"qom-type": "throttle-group", "id": "tg0"}}
{"execute": "blockdev-add",
"arguments": {
"node-name": "source",
"driver": "throttle",
"throttle-group": "tg0",
"file": {
"node-name": "filtered",
"driver": "qcow2",
"file": {
"driver": "file",
"filename": "/tmp/source.qcow2"
} } } }
{"execute": "drive-mirror",
"arguments": {
"job-id": "mirror",
"device": "source",
"target": "/tmp/target.qcow2",
"format": "qcow2",
"node-name": "target",
"sync" :"none",
"replaces": "filtered"
} }
{"execute": "block-job-complete", "arguments": {"device": "mirror"}}
And qemu crashes because of a stack overflow due to the loop being
created (target's backing file is source, so when it replaces filtered,
it points to itself through source).
(blockdev-mirror can be broken similarly.)
So let us make the checks for the two conditions above explicit, which
makes the whole function exactly as restrictive as it needs to be.
Signed-off-by: Max Reitz
---
include/block/block.h | 1 +
block.c | 83 +++
blockdev.c| 34 --
3 files changed, 110 insertions(+), 8 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 6ba853fb90..8da706cd89 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -404,6 +404,7 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate);
/* check if a named node can be replaced when doing drive-mirror */
BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
+BlockDriverState *backing_bs,
const char *node_name, Error **errp);
/* async block I/O */
diff --git a/block.c b/block.c
index 915b80153c..4858d3e718 100644
--- a/block.c
+++ b/block.c
@@ -6290,7 +6290,59 @@ bool bdrv_is_first_non_filter(BlockDriverState
*candidate)
return false;
}
+static bool is_child_of(BlockDriverState *child, BlockDriverState *parent)
+{
+BdrvChild *c;
+
+if (!parent) {
+return false;
+}
+
+QLIST_FOREACH(c, >children, next) {
+if (c->bs == child || is_child_of(child, c->bs)) {
+return true;
+}
+}
+
+return false;
+}
+
+/*
+ * Return true if there are only filters in [@top, @base). Note that
+ * this may include quorum (which bdrv_chain_contains() cannot
+ * handle).
+ */
+static bool is_filtered_child(BlockDriverState *top, BlockDriverState *base)
+{
+BdrvChild *c;
+
+if (!top) {
+return false;
+}
+
+if (top == base) {
+return true;
+}
+
+if (!top->drv->is_filter) {
+return false;
+}
+
+QLIST_FOREACH(c, >children, next) {
+if (is_filtered_child(c->bs, base)) {
+return true;
+}
+}
+
+return false;
+}
+
+/*
+ * @parent_bs is mirror's source BDS, @backing_bs is the BDS which
+ * will be attached to the target when mirror completes.
+ */
BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
+BlockDriverState *backing_bs,
const char *node_name, Error **errp)
{
BlockDriverState *to_replace_bs = bdrv_find_node(node_name);
@@ -6309,13 +6361,32 @@ BlockDriverState
*check_to_replace_node(BlockDriverState *parent_bs,
goto out;
}
-/* We don't want arbitrary node of the BDS chain to be replaced only the
top
- * most non filter in order to prevent data corruption.
- * Another benefit is that this tests exclude backing files which are
- * blocked by the backing blockers.
+/*
+ * If to_replace_bs is (recursively) a child of backing_bs,
+ * replacing it may create a loop. We cannot allow that.
*/
-if (!bdrv_recurse_is_first_non_filter(parent_bs, to_replace_bs)) {
-error_setg(errp, "Only top most non filter can be replaced");
+if (to_replace_bs == backing_bs || is_child_of(to_replace_bs, backing_bs))
{
+error_setg(errp, "Replacing this node would result in a loop");
+to_replace_bs
