Re: [Qemu-devel] [PATCH] qcow2: Make qcow2_alloc_bytes() more explicit

[Kevin Wolf]

Am 11.09.2015 um 18:47 hat Max Reitz geschrieben:
> In case of -EAGAIN returned by update_refcount(), we should discard the
> cluster offset we were trying to allocate and request a new one, because
> in theory that old offset might now be taken by a refcount block.
> 
> In practice, this was not the case due to update_refcount() generally
> returning strictly monotonic increasing cluster offsets. However, this
> behavior is not set in stone, and it is also not obvious when looking at
> qcow2_alloc_bytes() alone, so we should not rely on it.
> 
> Reported-by: Kevin Wolf <address@hidden>
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block/qcow2-refcount.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index e8430ec..c30bb14 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -949,11 +949,17 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int 
> size)
>  
>              if (!offset || ROUND_UP(offset, s->cluster_size) != new_cluster) 
> {
>                  offset = new_cluster;
> +                free_in_cluster = s->cluster_size;
> +            } else {
> +                free_in_cluster += s->cluster_size;
>              }
>          }

This doesn't actually do anything except confuse the reader, but as
the value of free_in_cluster doesn't matter in the second iteration
because offset == 0, this is correct.

>          assert(offset);
>          ret = update_refcount(bs, offset, size, 1, false, 
> QCOW2_DISCARD_NEVER);
> +        if (ret < 0) {
> +            offset = 0;
> +        }
>      } while (ret == -EAGAIN);
>      if (ret < 0) {
>          return ret;

Thanks, applied to the block branch.

Kevin