Re: [Qemu-block] [PATCH v3 18/39] qcow2: Refactor get_cluster_table()

[Max Reitz]

On 2018-01-26 15:59, Alberto Garcia wrote:
> After the previous patch we're now always using l2_load() in
> get_cluster_table() regardless of whether a new L2 table has to be
> allocated or not.
> 
> This patch refactors that part of the code to use one single l2_load()
> call.
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  block/qcow2-cluster.c | 21 +++++++--------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 2a53c1dc5f..0c0cab85e8 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -695,15 +695,7 @@ static int get_cluster_table(BlockDriverState *bs, 
> uint64_t offset,
>          return -EIO;
>      }
>  
> -    /* seek the l2 table of the given l2 offset */
> -
> -    if (s->l1_table[l1_index] & QCOW_OFLAG_COPIED) {
> -        /* load the l2 table in memory */
> -        ret = l2_load(bs, offset, l2_offset, &l2_table);
> -        if (ret < 0) {
> -            return ret;
> -        }
> -    } else {
> +    if (!(s->l1_table[l1_index] & QCOW_OFLAG_COPIED)) {
>          /* First allocate a new L2 table (and do COW if needed) */
>          ret = l2_allocate(bs, l1_index);
>          if (ret < 0) {
> @@ -719,11 +711,12 @@ static int get_cluster_table(BlockDriverState *bs, 
> uint64_t offset,
>          /* Get the offset of the newly-allocated l2 table */
>          l2_offset = s->l1_table[l1_index] & L1E_OFFSET_MASK;
>          assert(offset_into_cluster(s, l2_offset) == 0);
> -        /* Load the l2 table in memory */
> -        ret = l2_load(bs, offset, l2_offset, &l2_table);
> -        if (ret < 0) {
> -            return ret;
> -        }
> +    }
> +
> +    /* load the l2 table in memory */
> +    ret = l2_load(bs, offset, l2_offset, &l2_table);
> +    if (ret < 0) {
> +        return ret;
>      }

I'd pull the l2_offset assignment (and alignment check) down below the
QCOW_OFLAG_COPIED check, saving us another two lines (!!1!) which we
could then spend on an

assert(s->l1_table[l1_index] & QCOW_OFLAG_COPIED);

Max

>  
>      /* find the cluster offset for the given disk offset */
>

signature.asc
Description: OpenPGP digital signature