28.03.2019 13:27, Vladimir Sementsov-Ogievskiy wrote: > 26.03.2019 20:07, Alberto Garcia wrote: >> All three functions that handle the BdrvChild.frozen attribute walk >> the backing chain from 'bs' to 'base' and stop either when 'base' is >> found or at the end of the chain if 'base' is NULL. >> >> However if 'base' is not found then the functions return without >> errors as if it was NULL. >> >> This is wrong: if the caller passed an incorrect parameter that means >> that there is a bug in the code. >> >> Signed-off-by: Alberto Garcia <be...@igalia.com> > > interesting that bdrv_is_allocated_above has the same flaw. Could you fix it > too?
However bdrv_is_allocated_above is differs from your functions, as it may yield.. And graph may change while it is running.. Shouldn't we freeze backing chain every time we want to call bdrv_is_allocated_above? > >> --- >> block.c | 21 ++++++++++++++------- >> 1 file changed, 14 insertions(+), 7 deletions(-) >> >> diff --git a/block.c b/block.c >> index 0a93ee9ac8..3050854528 100644 >> --- a/block.c >> +++ b/block.c >> @@ -4218,14 +4218,15 @@ BlockDriverState *bdrv_find_base(BlockDriverState >> *bs) >> /* >> * Return true if at least one of the backing links between @bs and >> * @base is frozen. @errp is set if that's the case. >> + * @base must be reachable from @bs, or NULL. >> */ >> bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState >> *base, >> Error **errp) >> { >> BlockDriverState *i; >> - for (i = bs; i != base && i->backing; i = backing_bs(i)) { >> - if (i->backing->frozen) { >> + for (i = bs; i != base; i = backing_bs(i)) { >> + if (i->backing && i->backing->frozen) { >> error_setg(errp, "Cannot change '%s' link from '%s' to '%s'", >> i->backing->name, i->node_name, >> backing_bs(i)->node_name); >> @@ -4240,6 +4241,7 @@ bool bdrv_is_backing_chain_frozen(BlockDriverState >> *bs, BlockDriverState *base, >> * Freeze all backing links between @bs and @base. >> * If any of the links is already frozen the operation is aborted and >> * none of the links are modified. >> + * @base must be reachable from @bs, or NULL. >> * Returns 0 on success. On failure returns < 0 and sets @errp. >> */ >> int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, >> @@ -4251,8 +4253,10 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, >> BlockDriverState *base, >> return -EPERM; >> } >> - for (i = bs; i != base && i->backing; i = backing_bs(i)) { >> - i->backing->frozen = true; >> + for (i = bs; i != base; i = backing_bs(i)) { >> + if (i->backing) { >> + i->backing->frozen = true; >> + } >> } >> return 0; >> @@ -4261,14 +4265,17 @@ int bdrv_freeze_backing_chain(BlockDriverState *bs, >> BlockDriverState *base, >> /* >> * Unfreeze all backing links between @bs and @base. The caller must >> * ensure that all links are frozen before using this function. >> + * @base must be reachable from @bs, or NULL. >> */ >> void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState >> *base) >> { >> BlockDriverState *i; >> - for (i = bs; i != base && i->backing; i = backing_bs(i)) { >> - assert(i->backing->frozen); >> - i->backing->frozen = false; >> + for (i = bs; i != base; i = backing_bs(i)) { >> + if (i->backing) { >> + assert(i->backing->frozen); >> + i->backing->frozen = false; >> + } >> } >> } >> > > -- Best regards, Vladimir