| Message ID | a19d3ff80bae414ab2fb0ca773592f0f4f93acda.1553619891.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
| Series | freeze the backing chain earlier in stream_start() | expand |
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 <berto@igalia.com> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > 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) { may be a bit more plain conversion would be just add assert(i == base) after each loop, but I'm OK with this too. > 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; > + } > } > } > >
On Thu 28 Mar 2019 10:45:51 AM CET, Vladimir Sementsov-Ogievskiy wrote: >> - 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) { > > may be a bit more plain conversion would be just add assert(i == base) > after each loop, but I'm OK with this too. It's not necessary, because the loop can only stop when i == base already, so that assertion is always going to be true. If you mean that we should keep everything as it was before and simply add that assertion then that's not enough. If base == NULL then the loop will stop when i->backing == NULL, not when i == base. Berto
28.03.2019 13:04, Alberto Garcia wrote: > On Thu 28 Mar 2019 10:45:51 AM CET, Vladimir Sementsov-Ogievskiy wrote: >>> - 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) { >> >> may be a bit more plain conversion would be just add assert(i == base) >> after each loop, but I'm OK with this too. > > It's not necessary, because the loop can only stop when i == base > already, so that assertion is always going to be true. > > If you mean yes > that we should keep everything as it was before and simply > add that assertion then that's not enough. If base == NULL then the loop > will stop when i->backing == NULL, not when i == base. and yes, I'm wrong. > > Berto >
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 <berto@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; > + } > } > } > >
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 <berto@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; >> + } >> } >> } >> > >
On Thu 28 Mar 2019 11:33:55 AM CET, Vladimir Sementsov-Ogievskiy 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 <berto@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? I think all callers of bdrv_is_allocated_above() already freeze the chain themselves? Anyway I would not change this for 4.0 without an actual bug. Berto
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; + } } }
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 <berto@igalia.com> --- block.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)