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? > --- > 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