diff mbox

[v2,1/7] vmdk: Refactor and introduce new helper functions

Message ID 1490440701-12037-2-git-send-email-ashijeetacharya@gmail.com
State New
Headers show

Commit Message

Ashijeet Acharya March 25, 2017, 11:18 a.m. UTC
Move the existing vmdk_find_offset_in_cluster() function to the top of
the driver. Also, introduce a new helper function size_to_clusters()
which returns the number of clusters for a given size in bytes. Here,
we leave the last cluster as we need to perform COW for that one.

Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
---
 block/vmdk.c | 32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Comments

Fam Zheng March 31, 2017, 5:52 a.m. UTC | #1
On Sat, 03/25 16:48, Ashijeet Acharya wrote:
> Move the existing vmdk_find_offset_in_cluster() function to the top of
> the driver. Also, introduce a new helper function size_to_clusters()
> which returns the number of clusters for a given size in bytes. Here,
> we leave the last cluster as we need to perform COW for that one.
> 
> Signed-off-by: Ashijeet Acharya <ashijeetacharya@gmail.com>
> ---
>  block/vmdk.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index a9bd22b..7795c5f 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -242,6 +242,26 @@ static void vmdk_free_last_extent(BlockDriverState *bs)
>      s->extents = g_renew(VmdkExtent, s->extents, s->num_extents);
>  }
>  
> +static inline uint64_t vmdk_find_offset_in_cluster(VmdkExtent *extent,
> +                                                   int64_t offset)
> +{
> +    uint64_t extent_begin_offset, extent_relative_offset;
> +    uint64_t cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
> +
> +    extent_begin_offset =
> +        (extent->end_sector - extent->sectors) * BDRV_SECTOR_SIZE;
> +    extent_relative_offset = offset - extent_begin_offset;
> +    return extent_relative_offset % cluster_size;
> +}

Code movement and new code are usually in separate patches.

> +
> +static inline uint64_t size_to_clusters(VmdkExtent *extent, uint64_t size)
> +{
> +    uint64_t cluster_size, round_off_size;
> +    cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
> +    round_off_size = cluster_size - (size % cluster_size);
> +    return ((size + round_off_size) >> 16) - 1;

Why hardcode 16? You can use DIV_ROUND_UP().

> +}
> +

Usually a static function is added in the patch that actually uses it, which
makes reviewing easier.

Fam

>  static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
>  {
>      char *desc;
> @@ -1266,18 +1286,6 @@ static VmdkExtent *find_extent(BDRVVmdkState *s,
>      return NULL;
>  }
>  
> -static inline uint64_t vmdk_find_offset_in_cluster(VmdkExtent *extent,
> -                                                   int64_t offset)
> -{
> -    uint64_t extent_begin_offset, extent_relative_offset;
> -    uint64_t cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
> -
> -    extent_begin_offset =
> -        (extent->end_sector - extent->sectors) * BDRV_SECTOR_SIZE;
> -    extent_relative_offset = offset - extent_begin_offset;
> -    return extent_relative_offset % cluster_size;
> -}
> -
>  static inline uint64_t vmdk_find_index_in_cluster(VmdkExtent *extent,
>                                                    int64_t sector_num)
>  {
> -- 
> 2.6.2
> 
>
diff mbox

Patch

diff --git a/block/vmdk.c b/block/vmdk.c
index a9bd22b..7795c5f 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -242,6 +242,26 @@  static void vmdk_free_last_extent(BlockDriverState *bs)
     s->extents = g_renew(VmdkExtent, s->extents, s->num_extents);
 }
 
+static inline uint64_t vmdk_find_offset_in_cluster(VmdkExtent *extent,
+                                                   int64_t offset)
+{
+    uint64_t extent_begin_offset, extent_relative_offset;
+    uint64_t cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
+
+    extent_begin_offset =
+        (extent->end_sector - extent->sectors) * BDRV_SECTOR_SIZE;
+    extent_relative_offset = offset - extent_begin_offset;
+    return extent_relative_offset % cluster_size;
+}
+
+static inline uint64_t size_to_clusters(VmdkExtent *extent, uint64_t size)
+{
+    uint64_t cluster_size, round_off_size;
+    cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
+    round_off_size = cluster_size - (size % cluster_size);
+    return ((size + round_off_size) >> 16) - 1;
+}
+
 static uint32_t vmdk_read_cid(BlockDriverState *bs, int parent)
 {
     char *desc;
@@ -1266,18 +1286,6 @@  static VmdkExtent *find_extent(BDRVVmdkState *s,
     return NULL;
 }
 
-static inline uint64_t vmdk_find_offset_in_cluster(VmdkExtent *extent,
-                                                   int64_t offset)
-{
-    uint64_t extent_begin_offset, extent_relative_offset;
-    uint64_t cluster_size = extent->cluster_sectors * BDRV_SECTOR_SIZE;
-
-    extent_begin_offset =
-        (extent->end_sector - extent->sectors) * BDRV_SECTOR_SIZE;
-    extent_relative_offset = offset - extent_begin_offset;
-    return extent_relative_offset % cluster_size;
-}
-
 static inline uint64_t vmdk_find_index_in_cluster(VmdkExtent *extent,
                                                   int64_t sector_num)
 {