On 08.06.2016 10:58, Kevin Wolf wrote: > Am 06.06.2016 um 16:42 hat Max Reitz geschrieben: >> change_parent_backing_link() asserts that the BDS to be replaced is not >> used as a backing file. However, we may want to replace a BDS by its >> overlay in which case that very link should not be redirected. >> >> Signed-off-by: Max Reitz <mre...@redhat.com> > > So the scenario is like this? > > +---- to > v > base <- from <- top > > And we want to change it into this: > > base <- from <- to <- top > > Okay, makes sense.
Doesn't the assert(c->role != &child_backing) prevent this scenario?
(which is the reason for the first sentence in my commit message.)
So the scenario is rather:
+----- target
v
base <- source <- BB
to:
base <- source <- target <- BB
> (Hm, put ASCII art in the commit message? I'd be all for it.)
Well, depending on whether a v3 will come or not, I can put it there, of
course.
>
>> block.c | 15 ++++++++++++++-
>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/block.c b/block.c
>> index f54bc25..16463aa 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -2224,9 +2224,22 @@ void bdrv_close_all(void)
>> static void change_parent_backing_link(BlockDriverState *from,
>> BlockDriverState *to)
>> {
>> - BdrvChild *c, *next;
>> + BdrvChild *c, *next, *to_c;
>>
>> QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
>> + if (c->role == &child_backing) {
>> + /* Allow @from to be in a backing chain, but only if it is @to's
>> + * backing chain. Do not replace @from by @to there. */
>
> The comment suggests bdrv_chain_contains(), but you only accept it as a
> direct child of top and you accept non-backing-file children.
>
> Intuitively I would say that anywhere in the backing chain would make
> sense, but we can always allow that later when we actually need it.
> Accepting all types of children sounds right when I think about
> inserting a quorum node as 'to'.
>
> So I guess the code is fine and the comment should be changed to
> correctly reflect what the code does.
OK, will do.
>> + QLIST_FOREACH(to_c, &to->children, next) {
>> + if (to_c == c) {
>> + break;
>> + }
>> + }
>> + if (to_c) {
>> + continue;
>> + }
>> + }
>> +
>> assert(c->role != &child_backing);
>> bdrv_ref(to);
>> bdrv_replace_child(c, to);
>
> The other thing is that I'm unsure whether this function makes any sense
> at all. "Replace in all parents" is kind of arbitrary. In the long term,
> we may want to allow the user to specify the exact graph modifications
> on (block-)job-complete.
Well, as a replacement of bdrv_swap(), this function definitely does
make sense. Whether we can do even better in the long run... Is a
question for the long run, I think.
Max
signature.asc
Description: OpenPGP digital signature
