Re: [PATCH v4 02/30] qcow2: Convert qcow2_get_cluster_offset() into qcow2_get_host_offset()

[Max Reitz]

On 17.03.20 19:15, Alberto Garcia wrote:
> qcow2_get_cluster_offset() takes an (unaligned) guest offset and
> returns the (aligned) offset of the corresponding cluster in the qcow2
> image.
> 
> In practice none of the callers need to know where the cluster starts
> so this patch makes the function calculate and return the final host
> offset directly. The function is also renamed accordingly.
> 
> There is a pre-existing exception with compressed clusters: in this
> case the function returns the complete cluster descriptor (containing
> the offset and size of the compressed data). This does not change with
> this patch but it is now documented.
> 
> Signed-off-by: Alberto Garcia <address@hidden>
> ---
>  block/qcow2.h         |  4 ++--
>  block/qcow2-cluster.c | 38 ++++++++++++++++++++++----------------
>  block/qcow2.c         | 24 +++++++-----------------
>  3 files changed, 31 insertions(+), 35 deletions(-)
> 
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 0942126232..f47ef6ca4e 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h

[...]

>      case QCOW2_CLUSTER_ZERO_ALLOC:
>      case QCOW2_CLUSTER_NORMAL:
>          /* how many allocated clusters ? */
>          c = count_contiguous_clusters(bs, nb_clusters, s->cluster_size,
>                                        &l2_slice[l2_index], QCOW_OFLAG_ZERO);
> -        *cluster_offset &= L2E_OFFSET_MASK;
> -        if (offset_into_cluster(s, *cluster_offset)) {
> +        *host_offset = l2_entry & L2E_OFFSET_MASK;
> +        if (offset_into_cluster(s, *host_offset)) {
>              qcow2_signal_corruption(bs, true, -1, -1,
>                                      "Cluster allocation offset %#"
>                                      PRIx64 " unaligned (L2 offset: %#" PRIx64
> -                                    ", L2 index: %#x)", *cluster_offset,
> +                                    ", L2 index: %#x)", *host_offset,
>                                      l2_offset, l2_index);
>              ret = -EIO;
>              goto fail;
>          }
> -        if (has_data_file(bs) && *cluster_offset != offset - 
> offset_in_cluster)
> +        if (has_data_file(bs) && *host_offset != offset - offset_in_cluster)
>          {

(1) The { should be moved to the preceding line;

(2) I think it makes more sense to move the
“*host_offset += offset_in_cluster” before this condition, so it becomes
“... && *host_offset != offset”.

>              qcow2_signal_corruption(bs, true, -1, -1,
>                                      "External data file host cluster offset 
> %#"

(Maybe we then need to drop the “cluster” from this line, but other than
that, it would fit with this error message.)

Max

>                                      PRIx64 " does not match guest cluster "
>                                      "offset: %#" PRIx64
> -                                    ", L2 index: %#x)", *cluster_offset,
> +                                    ", L2 index: %#x)", *host_offset,
>                                      offset - offset_in_cluster, l2_index);
>              ret = -EIO;
>              goto fail;
>          }
> +        *host_offset += offset_in_cluster;
>          break;
>      default:
>          abort();

signature.asc
Description: OpenPGP digital signature