Patchwork [v3,09/10] vmdk: use heap allocation for whole_grain

login
register
mail settings
Submitter Fam Zheng
Date Aug. 6, 2013, 1:40 a.m.
Message ID <1375753243-19530-10-git-send-email-famz@redhat.com>
Download mbox | patch
Permalink /patch/264838/
State New
Headers show

Comments

Fam Zheng - Aug. 6, 2013, 1:40 a.m.
We should never grow the stack beyond 1 MB, otherwise we'll fall off the
end.  Thread stacks and coroutine stacks (1 MB) do not grow.
get_cluster_offset() allocates a big stack offset, it will fail for big
cluster images, change to heap allocated buffer.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/vmdk.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)
Jeff Cody - Aug. 6, 2013, 3:52 a.m.
On Tue, Aug 06, 2013 at 09:40:42AM +0800, Fam Zheng wrote:
> We should never grow the stack beyond 1 MB, otherwise we'll fall off the
> end.  Thread stacks and coroutine stacks (1 MB) do not grow.
> get_cluster_offset() allocates a big stack offset, it will fail for big
> cluster images, change to heap allocated buffer.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/vmdk.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 3bcab26..21610e3 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -842,16 +842,19 @@ static int get_whole_cluster(BlockDriverState *bs,
>                  uint64_t offset,
>                  bool allocate)
>  {
> -    /* 128 sectors * 512 bytes each = grain size 64KB */
> -    uint8_t  whole_grain[extent->cluster_sectors * 512];
> +    int ret = VMDK_OK;
> +    uint8_t *whole_grain = NULL;
>  
>      /* we will be here if it's first write on non-exist grain(cluster).
>       * try to read from parent image, if exist */
>      if (bs->backing_hd) {
>          int ret;
            ^^^^^^
This above line should be removed; as it is this function will always return
VMDK_OK with this here, due to the scoping of this 'ret' inside the
if() block.

>  
> +        whole_grain =
> +            qemu_blockalign(bs, extent->cluster_sectors << BDRV_SECTOR_BITS);
>          if (!vmdk_is_cid_valid(bs)) {
> -            return VMDK_ERROR;
> +            ret = VMDK_ERROR;
> +            goto exit;
>          }
>  
>          /* floor offset to cluster */
> @@ -859,17 +862,21 @@ static int get_whole_cluster(BlockDriverState *bs,
>          ret = bdrv_read(bs->backing_hd, offset >> 9, whole_grain,
>                  extent->cluster_sectors);
>          if (ret < 0) {
> -            return VMDK_ERROR;
> +            ret = VMDK_ERROR;
> +            goto exit;
>          }
>  
>          /* Write grain only into the active image */
>          ret = bdrv_write(extent->file, cluster_offset, whole_grain,
>                  extent->cluster_sectors);
>          if (ret < 0) {
> -            return VMDK_ERROR;
> +            ret = VMDK_ERROR;
> +            goto exit;
>          }
>      }
> -    return VMDK_OK;
> +exit:
> +    qemu_vfree(whole_grain);
> +    return ret;
>  }
>  
>  static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data)
> -- 
> 1.8.3.4
>

Patch

diff --git a/block/vmdk.c b/block/vmdk.c
index 3bcab26..21610e3 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -842,16 +842,19 @@  static int get_whole_cluster(BlockDriverState *bs,
                 uint64_t offset,
                 bool allocate)
 {
-    /* 128 sectors * 512 bytes each = grain size 64KB */
-    uint8_t  whole_grain[extent->cluster_sectors * 512];
+    int ret = VMDK_OK;
+    uint8_t *whole_grain = NULL;
 
     /* we will be here if it's first write on non-exist grain(cluster).
      * try to read from parent image, if exist */
     if (bs->backing_hd) {
         int ret;
 
+        whole_grain =
+            qemu_blockalign(bs, extent->cluster_sectors << BDRV_SECTOR_BITS);
         if (!vmdk_is_cid_valid(bs)) {
-            return VMDK_ERROR;
+            ret = VMDK_ERROR;
+            goto exit;
         }
 
         /* floor offset to cluster */
@@ -859,17 +862,21 @@  static int get_whole_cluster(BlockDriverState *bs,
         ret = bdrv_read(bs->backing_hd, offset >> 9, whole_grain,
                 extent->cluster_sectors);
         if (ret < 0) {
-            return VMDK_ERROR;
+            ret = VMDK_ERROR;
+            goto exit;
         }
 
         /* Write grain only into the active image */
         ret = bdrv_write(extent->file, cluster_offset, whole_grain,
                 extent->cluster_sectors);
         if (ret < 0) {
-            return VMDK_ERROR;
+            ret = VMDK_ERROR;
+            goto exit;
         }
     }
-    return VMDK_OK;
+exit:
+    qemu_vfree(whole_grain);
+    return ret;
 }
 
 static int vmdk_L2update(VmdkExtent *extent, VmdkMetaData *m_data)