[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH for-4.0 1/3] block: continue until base is found
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [PATCH for-4.0 1/3] block: continue until base is found in bdrv_freeze_backing_chain() et al |
Date: |
Thu, 28 Mar 2019 10:27:27 +0000 |
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 <address@hidden>
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