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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

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 <berto@igalia.com>
>> ---
>>   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