[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:33:55 +0000 |
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 <address@hidden>
>
> 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
- [Qemu-block] [PATCH for-4.0 2/3] block: freeze the backing chain earlier in stream_start(), (continued)