Re: [Qemu-devel] [PATCH v13 06/12] qcow2: Make distinction between zero cluster types obvious

[Max Reitz]

On 07.05.2017 02:05, Eric Blake wrote:
> Treat plain zero clusters differently from allocated ones, so that
> we can simplify the logic of checking whether an offset is present.
> Do this by splitting QCOW2_CLUSTER_ZERO into two new enums,
> QCOW2_CLUSTER_ZERO_PLAIN and QCOW2_CLUSTER_ZERO_ALLOC.
> 
> I tried to arrange the enum so that we could use
> 'ret <= QCOW2_CLUSTER_ZERO_PLAIN' for all unallocated types, and
> 'ret >= QCOW2_CLUSTER_ZERO_ALLOC' for allocated types, although
> I didn't actually end up taking advantage of the layout.
> 
> In many cases, this leads to simpler code, by properly combining
> cases (sometimes, both zero types pair together, other times,
> plain zero is more like unallocated while allocated zero is more
> like normal).
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> v13: s/!preallocated/cluster_type == QCOW2_CLUSTER_ZERO_PLAIN/, fix
> error message text (including iotest fallout), rebase to earlier cleanups
> v12: new patch
> ---
>  block/qcow2.h              |  8 +++--
>  block/qcow2-cluster.c      | 81 
> ++++++++++++++++++----------------------------
>  block/qcow2-refcount.c     | 44 +++++++++++--------------
>  block/qcow2.c              |  9 ++++--
>  tests/qemu-iotests/060.out |  6 ++--
>  5 files changed, 64 insertions(+), 84 deletions(-)

[...]

> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index ed78a30..a4a3229 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c

[...]

> @@ -344,13 +343,13 @@ static int count_contiguous_clusters_unallocated(int 
> nb_clusters,
>  {
>      int i;
> 
> -    assert(wanted_type == QCOW2_CLUSTER_ZERO ||
> +    assert(wanted_type == QCOW2_CLUSTER_ZERO_PLAIN ||
>             wanted_type == QCOW2_CLUSTER_UNALLOCATED);
>      for (i = 0; i < nb_clusters; i++) {
>          uint64_t entry = be64_to_cpu(l2_table[i]);
> -        int type = qcow2_get_cluster_type(entry);
> +        QCow2ClusterType type = qcow2_get_cluster_type(entry);

With this moved into the previous patch:

Reviewed-by: Max Reitz <address@hidden>

(No, I'm not mentioning the fact that 060.out contains one more mention
of "Data cluster offset" that should definitely be a "Cluster allocation
offset" or even "Preallocated zero cluster offset" at
block/qcow2-cluster.c:1780, because let's really worry about this later)

> 
> -        if (type != wanted_type || entry & L2E_OFFSET_MASK) {
> +        if (type != wanted_type) {
>              break;
>          }
>      }

signature.asc
Description: OpenPGP digital signature