Re: [PATCH v9 21/34] qcow2: Add subcluster support to qcow2_get_host_offset()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3695 bytes --]

On 28.06.20 13:02, Alberto Garcia wrote:
> The logic of this function remains pretty much the same, except that
> it uses count_contiguous_subclusters(), which combines the logic of
> count_contiguous_clusters() / count_contiguous_clusters_unallocated()
> and checks individual subclusters.
> 
> qcow2_cluster_to_subcluster_type() is not necessary as a separate
> function anymore so it's inlined into its caller.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/qcow2.h         |  38 ++++-------
>  block/qcow2-cluster.c | 150 ++++++++++++++++++++++--------------------
>  2 files changed, 92 insertions(+), 96 deletions(-)

[...]

> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 59dd9bda29..2f3bd3a882 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -426,66 +426,66 @@ static int qcow2_get_subcluster_range_type(BlockDriverState *bs,

[...]

> +static int count_contiguous_subclusters(BlockDriverState *bs, int nb_clusters,
> +                                        unsigned sc_index, uint64_t *l2_slice,
> +                                        unsigned *l2_index)
>  {
>      BDRVQcow2State *s = bs->opaque;
> -    int i;
> -    QCow2ClusterType first_cluster_type;
> -    uint64_t mask = stop_flags | L2E_OFFSET_MASK | QCOW_OFLAG_COMPRESSED;
> -    uint64_t first_entry = get_l2_entry(s, l2_slice, l2_index);
> -    uint64_t offset = first_entry & mask;
> +    int i, count = 0;
> +    bool check_offset;
> +    uint64_t expected_offset;
> +    QCow2SubclusterType expected_type, type;
>  
> -    first_cluster_type = qcow2_get_cluster_type(bs, first_entry);
> -    if (first_cluster_type == QCOW2_CLUSTER_UNALLOCATED) {
> -        return 0;
> -    }
> -
> -    /* must be allocated */
> -    assert(first_cluster_type == QCOW2_CLUSTER_NORMAL ||
> -           first_cluster_type == QCOW2_CLUSTER_ZERO_ALLOC);
> +    assert(*l2_index + nb_clusters <= s->l2_size);

Not l2_slice_size?

>  
>      for (i = 0; i < nb_clusters; i++) {
> -        uint64_t l2_entry = get_l2_entry(s, l2_slice, l2_index + i) & mask;
> -        if (offset + (uint64_t) i * cluster_size != l2_entry) {
> +        unsigned first_sc = (i == 0) ? sc_index : 0;
> +        uint64_t l2_entry = get_l2_entry(s, l2_slice, *l2_index + i);
> +        uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, *l2_index + i);
> +        int ret = qcow2_get_subcluster_range_type(bs, l2_entry, l2_bitmap,
> +                                                  first_sc, &type);
> +        if (ret < 0) {
> +            *l2_index += i; /* Point to the invalid entry */
> +            return -EIO;
> +        }
> +        if (i == 0) {
> +            if (type == QCOW2_SUBCLUSTER_COMPRESSED) {
> +                /* Compressed clusters are always processed one by one */
> +                return ret;
> +            }
> +            expected_type = type;
> +            expected_offset = l2_entry & L2E_OFFSET_MASK;
> +            check_offset = (type == QCOW2_SUBCLUSTER_NORMAL ||
> +                            type == QCOW2_SUBCLUSTER_ZERO_ALLOC ||
> +                            type == QCOW2_SUBCLUSTER_UNALLOCATED_ALLOC);
> +        } else if (type != expected_type) {
>              break;
> +        } else if (check_offset) {

My gcc (v10.1.1) appears to be a bit daft, and so doesn’t recognize that
check_offset must always be initialized before this line is hit.

> +            expected_offset += s->cluster_size;

Same for expected_offset.

Would you mind helping it by initializing them in their definition? O:)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]