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

[Vladimir Sementsov-Ogievskiy]

08.04.2020 13:51, Max Reitz wrote:
> 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.)

Message would be less useful I think, better is compare two cluster offsets, as 
host cluster offset is specified by qcow2 metadata, not host offset.

What about squashing this:

--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -615,32 +615,34 @@ int qcow2_get_host_offset(BlockDriverState *bs, uint64_t 
offset,
         break;
     case QCOW2_CLUSTER_ZERO_ALLOC:
     case QCOW2_CLUSTER_NORMAL:
+    {
+        uint64_t host_cluster_offset = l2_slice & L2E_OFFSET_MASK;
+        *host_offset = host_cluster_offset + offset_in_cluster;
+
         /* how many allocated clusters ? */
         c = count_contiguous_clusters(bs, nb_clusters, s->cluster_size,
                                       &l2_slice[l2_index], QCOW_OFLAG_ZERO);
-        *host_offset = l2_entry & L2E_OFFSET_MASK;
-        if (offset_into_cluster(s, *host_offset)) {
+        if (offset_into_cluster(s, host_cluster_offset)) {
             qcow2_signal_corruption(bs, true, -1, -1,
                                     "Cluster allocation offset %#"
                                     PRIx64 " unaligned (L2 offset: %#" PRIx64
-                                    ", L2 index: %#x)", *host_offset,
+                                    ", L2 index: %#x)", host_cluster_offset,
                                     l2_offset, l2_index);
             ret = -EIO;
             goto fail;
         }
-        if (has_data_file(bs) && *host_offset != offset - offset_in_cluster)
-        {
+        if (has_data_file(bs) && *host_offset != offset) {
             qcow2_signal_corruption(bs, true, -1, -1,
                                     "External data file host cluster offset %#"
                                     PRIx64 " does not match guest cluster "
                                     "offset: %#" PRIx64
-                                    ", L2 index: %#x)", *host_offset,
+                                    ", L2 index: %#x)", host_cluster_offset,
                                     offset - offset_in_cluster, l2_index);
             ret = -EIO;
             goto fail;
         }
-        *host_offset += offset_in_cluster;
         break;
+    }
     default:
         abort();
     }



--
Best regards,
Vladimir