[Qemu-devel] [PATCH for-2.10 0/4] More blk_getlength() fixes

[Qemu-devel] [PATCH for-2.10 0/4] More blk_getlength() fixes

[Eric Blake]

Thanks again to Markus for catching these spots.

Eric Blake (4):
  vpc: Check failure of bdrv_getlength()
  qcow: Check failure of bdrv_getlength() and bdrv_truncate()
  qcow2: Drop debugging dump_refcounts()
  qcow2: Check failure of bdrv_getlength()

 block/qcow.c  | 64 +++++++++++++++++++++++++++++++++++++++++------------------
 block/qcow2.c | 26 ++++--------------------
 block/vpc.c   |  9 ++++++++-
 3 files changed, 57 insertions(+), 42 deletions(-)

-- 
2.13.4

[Qemu-devel] [PATCH 1/4] vpc: Check failure of bdrv_getlength()

[Eric Blake]

vpc_open() was checking for bdrv_getlength() failure in one, but
not the other, location.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/vpc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/block/vpc.c b/block/vpc.c
index 574879ba7c..468d10ec1c 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -219,6 +219,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
     uint64_t pagetable_size;
     int disk_type = VHD_DYNAMIC;
     int ret;
+    int64_t bs_size;

     bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
                                false, errp);
@@ -411,7 +412,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
             }
         }

-        if (s->free_data_block_offset > bdrv_getlength(bs->file->bs)) {
+        bs_size = bdrv_getlength(bs->file->bs);
+        if (bs_size < 0) {
+            error_setg_errno(errp, -bs_size, "unable to learn image size");
+            ret = bs_size;
+            goto fail;
+        }
+        if (s->free_data_block_offset > bs_size) {
             error_setg(errp, "block-vpc: free_data_block_offset points after "
                              "the end of file. The image has been truncated.");
             ret = -EINVAL;
-- 
2.13.4

[Qemu-devel] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

[Eric Blake]

This also requires changing the return type of get_cluster_offset()
and adjusting all callers.

Use osdep.h macros instead of open-coded rounding while in the
area.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/qcow.c | 64 ++++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 19 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index c08cdc4a7b..937023d447 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -347,16 +347,17 @@ static int qcow_reopen_prepare(BDRVReopenState *state,
  * 'compressed_size'. 'compressed_size' must be > 0 and <
  * cluster_size
  *
- * return 0 if not allocated.
+ * return 0 if not allocated, -errno on failure.
  */
-static uint64_t get_cluster_offset(BlockDriverState *bs,
-                                   uint64_t offset, int allocate,
-                                   int compressed_size,
-                                   int n_start, int n_end)
+static int64_t get_cluster_offset(BlockDriverState *bs,
+                                  uint64_t offset, int allocate,
+                                  int compressed_size,
+                                  int n_start, int n_end)
 {
     BDRVQcowState *s = bs->opaque;
     int min_index, i, j, l1_index, l2_index;
-    uint64_t l2_offset, *l2_table, cluster_offset, tmp;
+    int64_t l2_offset, cluster_offset;
+    uint64_t *l2_table, tmp;
     uint32_t min_count;
     int new_l2_table;

@@ -368,8 +369,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
             return 0;
         /* allocate a new l2 entry */
         l2_offset = bdrv_getlength(bs->file->bs);
+        if (l2_offset < 0) {
+            return l2_offset;
+        }
         /* round to cluster size */
-        l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 1);
+        l2_offset = QEMU_ALIGN_UP(l2_offset, s->cluster_size);
         /* update the L1 entry */
         s->l1_table[l1_index] = l2_offset;
         tmp = cpu_to_be64(l2_offset);
@@ -430,8 +434,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
             if (decompress_cluster(bs, cluster_offset) < 0)
                 return 0;
             cluster_offset = bdrv_getlength(bs->file->bs);
-            cluster_offset = (cluster_offset + s->cluster_size - 1) &
-                ~(s->cluster_size - 1);
+            if (cluster_offset < 0) {
+                return cluster_offset;
+            }
+            cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
             /* write the cluster content */
             if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache,
                             s->cluster_size) !=
@@ -439,12 +445,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
                 return -1;
         } else {
             cluster_offset = bdrv_getlength(bs->file->bs);
+            if (cluster_offset < 0) {
+                return cluster_offset;
+            }
             if (allocate == 1) {
+                int ret;
+
                 /* round to cluster size */
-                cluster_offset = (cluster_offset + s->cluster_size - 1) &
-                    ~(s->cluster_size - 1);
-                bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
-                              PREALLOC_MODE_OFF, NULL);
+                cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
+                ret = bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
+                                    PREALLOC_MODE_OFF, NULL);
+                if (ret < 0) {
+                    return ret;
+                }
                 /* if encrypted, we must initialize the cluster
                    content which won't be written */
                 if (bs->encrypted &&
@@ -491,11 +504,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
 {
     BDRVQcowState *s = bs->opaque;
     int index_in_cluster, n;
-    uint64_t cluster_offset;
+    int64_t cluster_offset;

     qemu_co_mutex_lock(&s->lock);
     cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
     qemu_co_mutex_unlock(&s->lock);
+    if (cluster_offset < 0) {
+        return cluster_offset;
+    }
     index_in_cluster = sector_num & (s->cluster_sectors - 1);
     n = s->cluster_sectors - index_in_cluster;
     if (n > nb_sectors)
@@ -567,7 +583,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
     BDRVQcowState *s = bs->opaque;
     int index_in_cluster;
     int ret = 0, n;
-    uint64_t cluster_offset;
+    int64_t cluster_offset;
     struct iovec hd_iov;
     QEMUIOVector hd_qiov;
     uint8_t *buf;
@@ -588,8 +604,10 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,

     while (nb_sectors != 0) {
         /* prepare next request */
-        cluster_offset = get_cluster_offset(bs, sector_num << 9,
-                                                 0, 0, 0, 0);
+        cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
+        if (cluster_offset < 0) {
+            return cluster_offset;
+        }
         index_in_cluster = sector_num & (s->cluster_sectors - 1);
         n = s->cluster_sectors - index_in_cluster;
         if (n > nb_sectors) {
@@ -670,7 +688,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
 {
     BDRVQcowState *s = bs->opaque;
     int index_in_cluster;
-    uint64_t cluster_offset;
+    int64_t cluster_offset;
     int ret = 0, n;
     struct iovec hd_iov;
     QEMUIOVector hd_qiov;
@@ -704,6 +722,10 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
         cluster_offset = get_cluster_offset(bs, sector_num << 9, 1, 0,
                                             index_in_cluster,
                                             index_in_cluster + n);
+        if (cluster_offset < 0) {
+            ret = cluster_offset;
+            break;
+        }
         if (!cluster_offset || (cluster_offset & 511) != 0) {
             ret = -EIO;
             break;
@@ -949,7 +971,7 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     z_stream strm;
     int ret, out_len;
     uint8_t *buf, *out_buf;
-    uint64_t cluster_offset;
+    int64_t cluster_offset;

     buf = qemu_blockalign(bs, s->cluster_size);
     if (bytes != s->cluster_size) {
@@ -1003,6 +1025,10 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     qemu_co_mutex_lock(&s->lock);
     cluster_offset = get_cluster_offset(bs, offset, 2, out_len, 0, 0);
     qemu_co_mutex_unlock(&s->lock);
+    if (cluster_offset < 0) {
+        ret = cluster_offset;
+        goto fail;
+    }
     if (cluster_offset == 0) {
         ret = -EIO;
         goto fail;
-- 
2.13.4

[Qemu-devel] [PATCH 3/4] qcow2: Drop debugging dump_refcounts()

[Eric Blake]

It's been #if 0'd since its introduction in 2006, commit 585f8587.
We can revive dead code if we need it, but in the meantime, it has
bit-rotted (for example, not checking for failure in bdrv_getlength()).

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/qcow2.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index d7c600b5a2..99407403ea 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3798,27 +3798,6 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
     return spec_info;
 }

-#if 0
-static void dump_refcounts(BlockDriverState *bs)
-{
-    BDRVQcow2State *s = bs->opaque;
-    int64_t nb_clusters, k, k1, size;
-    int refcount;
-
-    size = bdrv_getlength(bs->file->bs);
-    nb_clusters = size_to_clusters(s, size);
-    for(k = 0; k < nb_clusters;) {
-        k1 = k;
-        refcount = get_refcount(bs, k);
-        k++;
-        while (k < nb_clusters && get_refcount(bs, k) == refcount)
-            k++;
-        printf("%" PRId64 ": refcount=%d nb=%" PRId64 "\n", k, refcount,
-               k - k1);
-    }
-}
-#endif
-
 static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
                               int64_t pos)
 {
-- 
2.13.4

[Qemu-devel] [PATCH 4/4] qcow2: Check failure of bdrv_getlength()

[Eric Blake]

qcow2_co_pwritev_compressed() should not call bdrv_truncate()
if determining the size failed.

Reported-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/qcow2.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 99407403ea..40ba26c111 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3282,12 +3282,15 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     z_stream strm;
     int ret, out_len;
     uint8_t *buf, *out_buf;
-    uint64_t cluster_offset;
+    int64_t cluster_offset;

     if (bytes == 0) {
         /* align end of file to a sector boundary to ease reading with
            sector based I/Os */
         cluster_offset = bdrv_getlength(bs->file->bs);
+        if (cluster_offset < 0) {
+            return cluster_offset;
+        }
         return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, NULL);
     }

-- 
2.13.4

Re: [Qemu-devel] [PATCH 1/4] vpc: Check failure of bdrv_getlength()

[Philippe Mathieu-Daudé]

On 08/07/2017 05:30 PM, Eric Blake wrote:
> vpc_open() was checking for bdrv_getlength() failure in one, but
> not the other, location.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   block/vpc.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 574879ba7c..468d10ec1c 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -219,6 +219,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>       uint64_t pagetable_size;
>       int disk_type = VHD_DYNAMIC;
>       int ret;
> +    int64_t bs_size;
> 
>       bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
>                                  false, errp);
> @@ -411,7 +412,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>               }
>           }
> 
> -        if (s->free_data_block_offset > bdrv_getlength(bs->file->bs)) {
> +        bs_size = bdrv_getlength(bs->file->bs);
> +        if (bs_size < 0) {
> +            error_setg_errno(errp, -bs_size, "unable to learn image size");
> +            ret = bs_size;
> +            goto fail;
> +        }
> +        if (s->free_data_block_offset > bs_size) {
>               error_setg(errp, "block-vpc: free_data_block_offset points after "
>                                "the end of file. The image has been truncated.");
>               ret = -EINVAL;
> 

Re: [Qemu-devel] [PATCH 3/4] qcow2: Drop debugging dump_refcounts()

[Philippe Mathieu-Daudé]

On 08/07/2017 05:30 PM, Eric Blake wrote:
> It's been #if 0'd since its introduction in 2006, commit 585f8587.
> We can revive dead code if we need it, but in the meantime, it has
> bit-rotted (for example, not checking for failure in bdrv_getlength()).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> --- >   block/qcow2.c | 21 ---------------------
>   1 file changed, 21 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index d7c600b5a2..99407403ea 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3798,27 +3798,6 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>       return spec_info;
>   }
> 
> -#if 0
> -static void dump_refcounts(BlockDriverState *bs)
> -{
> -    BDRVQcow2State *s = bs->opaque;
> -    int64_t nb_clusters, k, k1, size;
> -    int refcount;
> -
> -    size = bdrv_getlength(bs->file->bs);
> -    nb_clusters = size_to_clusters(s, size);
> -    for(k = 0; k < nb_clusters;) {
> -        k1 = k;
> -        refcount = get_refcount(bs, k);
> -        k++;
> -        while (k < nb_clusters && get_refcount(bs, k) == refcount)
> -            k++;
> -        printf("%" PRId64 ": refcount=%d nb=%" PRId64 "\n", k, refcount,
> -               k - k1);
> -    }
> -}
> -#endif
> -
>   static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
>                                 int64_t pos)
>   {
> 

Re: [Qemu-devel] [PATCH 4/4] qcow2: Check failure of bdrv_getlength()

[Philippe Mathieu-Daudé]

On 08/07/2017 05:30 PM, Eric Blake wrote:
> qcow2_co_pwritev_compressed() should not call bdrv_truncate()
> if determining the size failed.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   block/qcow2.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 99407403ea..40ba26c111 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3282,12 +3282,15 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>       z_stream strm;
>       int ret, out_len;
>       uint8_t *buf, *out_buf;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
> 
>       if (bytes == 0) {
>           /* align end of file to a sector boundary to ease reading with
>              sector based I/Os */
>           cluster_offset = bdrv_getlength(bs->file->bs);
> +        if (cluster_offset < 0) {
> +            return cluster_offset;
> +        }
>           return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, NULL);
>       }
> 

Re: [Qemu-devel] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

[Philippe Mathieu-Daudé]

On 08/07/2017 05:30 PM, Eric Blake wrote:
> This also requires changing the return type of get_cluster_offset()
> and adjusting all callers.
> 
> Use osdep.h macros instead of open-coded rounding while in the
> area.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>   block/qcow.c | 64 ++++++++++++++++++++++++++++++++++++++++++------------------
>   1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index c08cdc4a7b..937023d447 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -347,16 +347,17 @@ static int qcow_reopen_prepare(BDRVReopenState *state,
>    * 'compressed_size'. 'compressed_size' must be > 0 and <
>    * cluster_size
>    *
> - * return 0 if not allocated.
> + * return 0 if not allocated, -errno on failure.
>    */
> -static uint64_t get_cluster_offset(BlockDriverState *bs,
> -                                   uint64_t offset, int allocate,
> -                                   int compressed_size,
> -                                   int n_start, int n_end)
> +static int64_t get_cluster_offset(BlockDriverState *bs,
> +                                  uint64_t offset, int allocate,
> +                                  int compressed_size,
> +                                  int n_start, int n_end)
>   {
>       BDRVQcowState *s = bs->opaque;
>       int min_index, i, j, l1_index, l2_index;
> -    uint64_t l2_offset, *l2_table, cluster_offset, tmp;
> +    int64_t l2_offset, cluster_offset;
> +    uint64_t *l2_table, tmp;
>       uint32_t min_count;
>       int new_l2_table;
> 
> @@ -368,8 +369,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>               return 0;
>           /* allocate a new l2 entry */
>           l2_offset = bdrv_getlength(bs->file->bs);
> +        if (l2_offset < 0) {
> +            return l2_offset;
> +        }
>           /* round to cluster size */
> -        l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 1);
> +        l2_offset = QEMU_ALIGN_UP(l2_offset, s->cluster_size);
>           /* update the L1 entry */
>           s->l1_table[l1_index] = l2_offset;
>           tmp = cpu_to_be64(l2_offset);
> @@ -430,8 +434,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>               if (decompress_cluster(bs, cluster_offset) < 0)
>                   return 0;
>               cluster_offset = bdrv_getlength(bs->file->bs);
> -            cluster_offset = (cluster_offset + s->cluster_size - 1) &
> -                ~(s->cluster_size - 1);
> +            if (cluster_offset < 0) {
> +                return cluster_offset;
> +            }
> +            cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
>               /* write the cluster content */
>               if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache,
>                               s->cluster_size) !=
> @@ -439,12 +445,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>                   return -1;
>           } else {
>               cluster_offset = bdrv_getlength(bs->file->bs);
> +            if (cluster_offset < 0) {
> +                return cluster_offset;
> +            }
>               if (allocate == 1) {
> +                int ret;
> +
>                   /* round to cluster size */
> -                cluster_offset = (cluster_offset + s->cluster_size - 1) &
> -                    ~(s->cluster_size - 1);
> -                bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
> -                              PREALLOC_MODE_OFF, NULL);
> +                cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
> +                ret = bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
> +                                    PREALLOC_MODE_OFF, NULL);
> +                if (ret < 0) {
> +                    return ret;
> +                }
>                   /* if encrypted, we must initialize the cluster
>                      content which won't be written */
>                   if (bs->encrypted &&
> @@ -491,11 +504,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
>   {
>       BDRVQcowState *s = bs->opaque;
>       int index_in_cluster, n;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
> 
>       qemu_co_mutex_lock(&s->lock);
>       cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
>       qemu_co_mutex_unlock(&s->lock);
> +    if (cluster_offset < 0) {
> +        return cluster_offset;
> +    }
>       index_in_cluster = sector_num & (s->cluster_sectors - 1);
>       n = s->cluster_sectors - index_in_cluster;
>       if (n > nb_sectors)
> @@ -567,7 +583,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
>       BDRVQcowState *s = bs->opaque;
>       int index_in_cluster;
>       int ret = 0, n;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
>       struct iovec hd_iov;
>       QEMUIOVector hd_qiov;
>       uint8_t *buf;
> @@ -588,8 +604,10 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
> 
>       while (nb_sectors != 0) {
>           /* prepare next request */
> -        cluster_offset = get_cluster_offset(bs, sector_num << 9,
> -                                                 0, 0, 0, 0);
> +        cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
> +        if (cluster_offset < 0) {
> +            return cluster_offset;
> +        }
>           index_in_cluster = sector_num & (s->cluster_sectors - 1);
>           n = s->cluster_sectors - index_in_cluster;
>           if (n > nb_sectors) {
> @@ -670,7 +688,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
>   {
>       BDRVQcowState *s = bs->opaque;
>       int index_in_cluster;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
>       int ret = 0, n;
>       struct iovec hd_iov;
>       QEMUIOVector hd_qiov;
> @@ -704,6 +722,10 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
>           cluster_offset = get_cluster_offset(bs, sector_num << 9, 1, 0,
>                                               index_in_cluster,
>                                               index_in_cluster + n);
> +        if (cluster_offset < 0) {
> +            ret = cluster_offset;
> +            break;
> +        }
>           if (!cluster_offset || (cluster_offset & 511) != 0) {
>               ret = -EIO;
>               break;
> @@ -949,7 +971,7 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>       z_stream strm;
>       int ret, out_len;
>       uint8_t *buf, *out_buf;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
> 
>       buf = qemu_blockalign(bs, s->cluster_size);
>       if (bytes != s->cluster_size) {
> @@ -1003,6 +1025,10 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>       qemu_co_mutex_lock(&s->lock);
>       cluster_offset = get_cluster_offset(bs, offset, 2, out_len, 0, 0);
>       qemu_co_mutex_unlock(&s->lock);
> +    if (cluster_offset < 0) {
> +        ret = cluster_offset;
> +        goto fail;
> +    }
>       if (cluster_offset == 0) {
>           ret = -EIO;
>           goto fail;
> 

Re: [Qemu-devel] [Qemu-block] [PATCH for-2.10 0/4] More blk_getlength() fixes

[John Snow]

On 08/07/2017 04:30 PM, Eric Blake wrote:
> Thanks again to Markus for catching these spots.
> 
> Eric Blake (4):
>   vpc: Check failure of bdrv_getlength()
>   qcow: Check failure of bdrv_getlength() and bdrv_truncate()
>   qcow2: Drop debugging dump_refcounts()
>   qcow2: Check failure of bdrv_getlength()
> 
>  block/qcow.c  | 64 +++++++++++++++++++++++++++++++++++++++++------------------
>  block/qcow2.c | 26 ++++--------------------
>  block/vpc.c   |  9 ++++++++-
>  3 files changed, 57 insertions(+), 42 deletions(-)
> 

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH 1/4] vpc: Check failure of bdrv_getlength()

[Jeff Cody]

On Mon, Aug 07, 2017 at 03:30:04PM -0500, Eric Blake wrote:
> vpc_open() was checking for bdrv_getlength() failure in one, but
> not the other, location.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/vpc.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 574879ba7c..468d10ec1c 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -219,6 +219,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>      uint64_t pagetable_size;
>      int disk_type = VHD_DYNAMIC;
>      int ret;
> +    int64_t bs_size;
> 
>      bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
>                                 false, errp);
> @@ -411,7 +412,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>              }
>          }
> 
> -        if (s->free_data_block_offset > bdrv_getlength(bs->file->bs)) {
> +        bs_size = bdrv_getlength(bs->file->bs);
> +        if (bs_size < 0) {
> +            error_setg_errno(errp, -bs_size, "unable to learn image size");
> +            ret = bs_size;
> +            goto fail;
> +        }
> +        if (s->free_data_block_offset > bs_size) {
>              error_setg(errp, "block-vpc: free_data_block_offset points after "
>                               "the end of file. The image has been truncated.");
>              ret = -EINVAL;
> -- 
> 2.13.4
> 
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

[Jeff Cody]

On Mon, Aug 07, 2017 at 03:30:05PM -0500, Eric Blake wrote:
> This also requires changing the return type of get_cluster_offset()
> and adjusting all callers.
> 
> Use osdep.h macros instead of open-coded rounding while in the
> area.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>



Reviewed-by: Jeff Cody <jcody@redhat.com>



> ---
>  block/qcow.c | 64 ++++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index c08cdc4a7b..937023d447 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -347,16 +347,17 @@ static int qcow_reopen_prepare(BDRVReopenState *state,
>   * 'compressed_size'. 'compressed_size' must be > 0 and <
>   * cluster_size
>   *
> - * return 0 if not allocated.
> + * return 0 if not allocated, -errno on failure.
>   */
> -static uint64_t get_cluster_offset(BlockDriverState *bs,
> -                                   uint64_t offset, int allocate,
> -                                   int compressed_size,
> -                                   int n_start, int n_end)
> +static int64_t get_cluster_offset(BlockDriverState *bs,
> +                                  uint64_t offset, int allocate,
> +                                  int compressed_size,
> +                                  int n_start, int n_end)
>  {
>      BDRVQcowState *s = bs->opaque;
>      int min_index, i, j, l1_index, l2_index;
> -    uint64_t l2_offset, *l2_table, cluster_offset, tmp;
> +    int64_t l2_offset, cluster_offset;
> +    uint64_t *l2_table, tmp;
>      uint32_t min_count;
>      int new_l2_table;
> 
> @@ -368,8 +369,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>              return 0;
>          /* allocate a new l2 entry */
>          l2_offset = bdrv_getlength(bs->file->bs);
> +        if (l2_offset < 0) {
> +            return l2_offset;
> +        }
>          /* round to cluster size */
> -        l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 1);
> +        l2_offset = QEMU_ALIGN_UP(l2_offset, s->cluster_size);
>          /* update the L1 entry */
>          s->l1_table[l1_index] = l2_offset;
>          tmp = cpu_to_be64(l2_offset);
> @@ -430,8 +434,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>              if (decompress_cluster(bs, cluster_offset) < 0)
>                  return 0;
>              cluster_offset = bdrv_getlength(bs->file->bs);
> -            cluster_offset = (cluster_offset + s->cluster_size - 1) &
> -                ~(s->cluster_size - 1);
> +            if (cluster_offset < 0) {
> +                return cluster_offset;
> +            }
> +            cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
>              /* write the cluster content */
>              if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache,
>                              s->cluster_size) !=
> @@ -439,12 +445,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>                  return -1;
>          } else {
>              cluster_offset = bdrv_getlength(bs->file->bs);
> +            if (cluster_offset < 0) {
> +                return cluster_offset;
> +            }
>              if (allocate == 1) {
> +                int ret;
> +
>                  /* round to cluster size */
> -                cluster_offset = (cluster_offset + s->cluster_size - 1) &
> -                    ~(s->cluster_size - 1);
> -                bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
> -                              PREALLOC_MODE_OFF, NULL);
> +                cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
> +                ret = bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
> +                                    PREALLOC_MODE_OFF, NULL);
> +                if (ret < 0) {
> +                    return ret;
> +                }
>                  /* if encrypted, we must initialize the cluster
>                     content which won't be written */
>                  if (bs->encrypted &&
> @@ -491,11 +504,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
>  {
>      BDRVQcowState *s = bs->opaque;
>      int index_in_cluster, n;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
> 
>      qemu_co_mutex_lock(&s->lock);
>      cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
>      qemu_co_mutex_unlock(&s->lock);
> +    if (cluster_offset < 0) {
> +        return cluster_offset;
> +    }
>      index_in_cluster = sector_num & (s->cluster_sectors - 1);
>      n = s->cluster_sectors - index_in_cluster;
>      if (n > nb_sectors)
> @@ -567,7 +583,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
>      BDRVQcowState *s = bs->opaque;
>      int index_in_cluster;
>      int ret = 0, n;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
>      struct iovec hd_iov;
>      QEMUIOVector hd_qiov;
>      uint8_t *buf;
> @@ -588,8 +604,10 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
> 
>      while (nb_sectors != 0) {
>          /* prepare next request */
> -        cluster_offset = get_cluster_offset(bs, sector_num << 9,
> -                                                 0, 0, 0, 0);
> +        cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
> +        if (cluster_offset < 0) {
> +            return cluster_offset;
> +        }
>          index_in_cluster = sector_num & (s->cluster_sectors - 1);
>          n = s->cluster_sectors - index_in_cluster;
>          if (n > nb_sectors) {
> @@ -670,7 +688,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
>  {
>      BDRVQcowState *s = bs->opaque;
>      int index_in_cluster;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
>      int ret = 0, n;
>      struct iovec hd_iov;
>      QEMUIOVector hd_qiov;
> @@ -704,6 +722,10 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
>          cluster_offset = get_cluster_offset(bs, sector_num << 9, 1, 0,
>                                              index_in_cluster,
>                                              index_in_cluster + n);
> +        if (cluster_offset < 0) {
> +            ret = cluster_offset;
> +            break;
> +        }
>          if (!cluster_offset || (cluster_offset & 511) != 0) {
>              ret = -EIO;
>              break;
> @@ -949,7 +971,7 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>      z_stream strm;
>      int ret, out_len;
>      uint8_t *buf, *out_buf;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
> 
>      buf = qemu_blockalign(bs, s->cluster_size);
>      if (bytes != s->cluster_size) {
> @@ -1003,6 +1025,10 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>      qemu_co_mutex_lock(&s->lock);
>      cluster_offset = get_cluster_offset(bs, offset, 2, out_len, 0, 0);
>      qemu_co_mutex_unlock(&s->lock);
> +    if (cluster_offset < 0) {
> +        ret = cluster_offset;
> +        goto fail;
> +    }
>      if (cluster_offset == 0) {
>          ret = -EIO;
>          goto fail;
> -- 
> 2.13.4
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH 3/4] qcow2: Drop debugging dump_refcounts()

[Jeff Cody]

On Mon, Aug 07, 2017 at 03:30:06PM -0500, Eric Blake wrote:
> It's been #if 0'd since its introduction in 2006, commit 585f8587.
> We can revive dead code if we need it, but in the meantime, it has
> bit-rotted (for example, not checking for failure in bdrv_getlength()).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>


Reviewed-by: Jeff Cody <jcody@redhat.com>


> ---
>  block/qcow2.c | 21 ---------------------
>  1 file changed, 21 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index d7c600b5a2..99407403ea 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3798,27 +3798,6 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
>      return spec_info;
>  }
> 
> -#if 0
> -static void dump_refcounts(BlockDriverState *bs)
> -{
> -    BDRVQcow2State *s = bs->opaque;
> -    int64_t nb_clusters, k, k1, size;
> -    int refcount;
> -
> -    size = bdrv_getlength(bs->file->bs);
> -    nb_clusters = size_to_clusters(s, size);
> -    for(k = 0; k < nb_clusters;) {
> -        k1 = k;
> -        refcount = get_refcount(bs, k);
> -        k++;
> -        while (k < nb_clusters && get_refcount(bs, k) == refcount)
> -            k++;
> -        printf("%" PRId64 ": refcount=%d nb=%" PRId64 "\n", k, refcount,
> -               k - k1);
> -    }
> -}
> -#endif
> -
>  static int qcow2_save_vmstate(BlockDriverState *bs, QEMUIOVector *qiov,
>                                int64_t pos)
>  {
> -- 
> 2.13.4
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH 4/4] qcow2: Check failure of bdrv_getlength()

[Jeff Cody]

On Mon, Aug 07, 2017 at 03:30:07PM -0500, Eric Blake wrote:
> qcow2_co_pwritev_compressed() should not call bdrv_truncate()
> if determining the size failed.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>



Reviewed-by: Jeff Cody <jcody@redhat.com>


> ---
>  block/qcow2.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 99407403ea..40ba26c111 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3282,12 +3282,15 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>      z_stream strm;
>      int ret, out_len;
>      uint8_t *buf, *out_buf;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
> 
>      if (bytes == 0) {
>          /* align end of file to a sector boundary to ease reading with
>             sector based I/Os */
>          cluster_offset = bdrv_getlength(bs->file->bs);
> +        if (cluster_offset < 0) {
> +            return cluster_offset;
> +        }
>          return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, NULL);
>      }
> 
> -- 
> 2.13.4
> 
> 

Re: [Qemu-devel] [Qemu-block] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

[Jeff Cody]

On Mon, Aug 07, 2017 at 03:30:05PM -0500, Eric Blake wrote:
> This also requires changing the return type of get_cluster_offset()
> and adjusting all callers.
> 
> Use osdep.h macros instead of open-coded rounding while in the
> area.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/qcow.c | 64 ++++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index c08cdc4a7b..937023d447 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -347,16 +347,17 @@ static int qcow_reopen_prepare(BDRVReopenState *state,
>   * 'compressed_size'. 'compressed_size' must be > 0 and <
>   * cluster_size
>   *
> - * return 0 if not allocated.
> + * return 0 if not allocated, -errno on failure.
>   */
> -static uint64_t get_cluster_offset(BlockDriverState *bs,
> -                                   uint64_t offset, int allocate,
> -                                   int compressed_size,
> -                                   int n_start, int n_end)
> +static int64_t get_cluster_offset(BlockDriverState *bs,
> +                                  uint64_t offset, int allocate,
> +                                  int compressed_size,
> +                                  int n_start, int n_end)
>  {
>      BDRVQcowState *s = bs->opaque;
>      int min_index, i, j, l1_index, l2_index;
> -    uint64_t l2_offset, *l2_table, cluster_offset, tmp;
> +    int64_t l2_offset, cluster_offset;
> +    uint64_t *l2_table, tmp;
>      uint32_t min_count;
>      int new_l2_table;
> 
> @@ -368,8 +369,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>              return 0;
>          /* allocate a new l2 entry */
>          l2_offset = bdrv_getlength(bs->file->bs);
> +        if (l2_offset < 0) {
> +            return l2_offset;
> +        }
>          /* round to cluster size */
> -        l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 1);
> +        l2_offset = QEMU_ALIGN_UP(l2_offset, s->cluster_size);
>          /* update the L1 entry */
>          s->l1_table[l1_index] = l2_offset;
>          tmp = cpu_to_be64(l2_offset);
> @@ -430,8 +434,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>              if (decompress_cluster(bs, cluster_offset) < 0)
>                  return 0;
>              cluster_offset = bdrv_getlength(bs->file->bs);
> -            cluster_offset = (cluster_offset + s->cluster_size - 1) &
> -                ~(s->cluster_size - 1);
> +            if (cluster_offset < 0) {
> +                return cluster_offset;
> +            }
> +            cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
>              /* write the cluster content */
>              if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache,
>                              s->cluster_size) !=
> @@ -439,12 +445,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>                  return -1;
>          } else {
>              cluster_offset = bdrv_getlength(bs->file->bs);
> +            if (cluster_offset < 0) {
> +                return cluster_offset;
> +            }
>              if (allocate == 1) {
> +                int ret;
> +
>                  /* round to cluster size */
> -                cluster_offset = (cluster_offset + s->cluster_size - 1) &
> -                    ~(s->cluster_size - 1);
> -                bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
> -                              PREALLOC_MODE_OFF, NULL);
> +                cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
> +                ret = bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
> +                                    PREALLOC_MODE_OFF, NULL);

My r-b stands, but do you think it is worth checking for
(cluster_offset + s->cluster_size) > INT64_MAX?

> +                if (ret < 0) {
> +                    return ret;
> +                }
>                  /* if encrypted, we must initialize the cluster
>                     content which won't be written */
>                  if (bs->encrypted &&
> @@ -491,11 +504,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
>  {
>      BDRVQcowState *s = bs->opaque;
>      int index_in_cluster, n;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
> 
>      qemu_co_mutex_lock(&s->lock);
>      cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
>      qemu_co_mutex_unlock(&s->lock);
> +    if (cluster_offset < 0) {
> +        return cluster_offset;
> +    }
>      index_in_cluster = sector_num & (s->cluster_sectors - 1);
>      n = s->cluster_sectors - index_in_cluster;
>      if (n > nb_sectors)
> @@ -567,7 +583,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
>      BDRVQcowState *s = bs->opaque;
>      int index_in_cluster;
>      int ret = 0, n;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
>      struct iovec hd_iov;
>      QEMUIOVector hd_qiov;
>      uint8_t *buf;
> @@ -588,8 +604,10 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
> 
>      while (nb_sectors != 0) {
>          /* prepare next request */
> -        cluster_offset = get_cluster_offset(bs, sector_num << 9,
> -                                                 0, 0, 0, 0);
> +        cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
> +        if (cluster_offset < 0) {
> +            return cluster_offset;
> +        }
>          index_in_cluster = sector_num & (s->cluster_sectors - 1);
>          n = s->cluster_sectors - index_in_cluster;
>          if (n > nb_sectors) {
> @@ -670,7 +688,7 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
>  {
>      BDRVQcowState *s = bs->opaque;
>      int index_in_cluster;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
>      int ret = 0, n;
>      struct iovec hd_iov;
>      QEMUIOVector hd_qiov;
> @@ -704,6 +722,10 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
>          cluster_offset = get_cluster_offset(bs, sector_num << 9, 1, 0,
>                                              index_in_cluster,
>                                              index_in_cluster + n);
> +        if (cluster_offset < 0) {
> +            ret = cluster_offset;
> +            break;
> +        }
>          if (!cluster_offset || (cluster_offset & 511) != 0) {
>              ret = -EIO;
>              break;
> @@ -949,7 +971,7 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>      z_stream strm;
>      int ret, out_len;
>      uint8_t *buf, *out_buf;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
> 
>      buf = qemu_blockalign(bs, s->cluster_size);
>      if (bytes != s->cluster_size) {
> @@ -1003,6 +1025,10 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>      qemu_co_mutex_lock(&s->lock);
>      cluster_offset = get_cluster_offset(bs, offset, 2, out_len, 0, 0);
>      qemu_co_mutex_unlock(&s->lock);
> +    if (cluster_offset < 0) {
> +        ret = cluster_offset;
> +        goto fail;
> +    }
>      if (cluster_offset == 0) {
>          ret = -EIO;
>          goto fail;
> -- 
> 2.13.4
> 
> 

Re: [Qemu-devel] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

[Kevin Wolf]

Am 07.08.2017 um 22:30 hat Eric Blake geschrieben:
> This also requires changing the return type of get_cluster_offset()
> and adjusting all callers.
> 
> Use osdep.h macros instead of open-coded rounding while in the
> area.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/qcow.c | 64 ++++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 45 insertions(+), 19 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index c08cdc4a7b..937023d447 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -347,16 +347,17 @@ static int qcow_reopen_prepare(BDRVReopenState *state,
>   * 'compressed_size'. 'compressed_size' must be > 0 and <
>   * cluster_size
>   *
> - * return 0 if not allocated.
> + * return 0 if not allocated, -errno on failure.
>   */
> -static uint64_t get_cluster_offset(BlockDriverState *bs,
> -                                   uint64_t offset, int allocate,
> -                                   int compressed_size,
> -                                   int n_start, int n_end)
> +static int64_t get_cluster_offset(BlockDriverState *bs,
> +                                  uint64_t offset, int allocate,
> +                                  int compressed_size,
> +                                  int n_start, int n_end)

qcow2 ended up with this prototype:

int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
                             unsigned int *bytes, uint64_t *cluster_offset)

Separating a full uin64_t offset by-reference parameter and an 0/-errno
status return code feels a little cleaner to me.

And in fact, it's not only cleaner, but bit 63 is QCOW_OFLAG_COMPRESSED,
so this patch breaks compressed images.

>  {
>      BDRVQcowState *s = bs->opaque;
>      int min_index, i, j, l1_index, l2_index;
> -    uint64_t l2_offset, *l2_table, cluster_offset, tmp;
> +    int64_t l2_offset, cluster_offset;
> +    uint64_t *l2_table, tmp;
>      uint32_t min_count;
>      int new_l2_table;
> 
> @@ -368,8 +369,11 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>              return 0;
>          /* allocate a new l2 entry */
>          l2_offset = bdrv_getlength(bs->file->bs);
> +        if (l2_offset < 0) {
> +            return l2_offset;
> +        }
>          /* round to cluster size */
> -        l2_offset = (l2_offset + s->cluster_size - 1) & ~(s->cluster_size - 1);
> +        l2_offset = QEMU_ALIGN_UP(l2_offset, s->cluster_size);
>          /* update the L1 entry */
>          s->l1_table[l1_index] = l2_offset;
>          tmp = cpu_to_be64(l2_offset);
> @@ -430,8 +434,10 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>              if (decompress_cluster(bs, cluster_offset) < 0)
>                  return 0;
>              cluster_offset = bdrv_getlength(bs->file->bs);
> -            cluster_offset = (cluster_offset + s->cluster_size - 1) &
> -                ~(s->cluster_size - 1);
> +            if (cluster_offset < 0) {
> +                return cluster_offset;
> +            }
> +            cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
>              /* write the cluster content */
>              if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache,
>                              s->cluster_size) !=
> @@ -439,12 +445,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
>                  return -1;
>          } else {
>              cluster_offset = bdrv_getlength(bs->file->bs);
> +            if (cluster_offset < 0) {
> +                return cluster_offset;
> +            }
>              if (allocate == 1) {
> +                int ret;
> +
>                  /* round to cluster size */
> -                cluster_offset = (cluster_offset + s->cluster_size - 1) &
> -                    ~(s->cluster_size - 1);
> -                bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
> -                              PREALLOC_MODE_OFF, NULL);
> +                cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
> +                ret = bdrv_truncate(bs->file, cluster_offset + s->cluster_size,
> +                                    PREALLOC_MODE_OFF, NULL);
> +                if (ret < 0) {
> +                    return ret;
> +                }
>                  /* if encrypted, we must initialize the cluster
>                     content which won't be written */
>                  if (bs->encrypted &&
> @@ -491,11 +504,14 @@ static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
>  {
>      BDRVQcowState *s = bs->opaque;
>      int index_in_cluster, n;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
> 
>      qemu_co_mutex_lock(&s->lock);
>      cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
>      qemu_co_mutex_unlock(&s->lock);
> +    if (cluster_offset < 0) {
> +        return cluster_offset;
> +    }
>      index_in_cluster = sector_num & (s->cluster_sectors - 1);
>      n = s->cluster_sectors - index_in_cluster;
>      if (n > nb_sectors)
> @@ -567,7 +583,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
>      BDRVQcowState *s = bs->opaque;
>      int index_in_cluster;
>      int ret = 0, n;
> -    uint64_t cluster_offset;
> +    int64_t cluster_offset;
>      struct iovec hd_iov;
>      QEMUIOVector hd_qiov;
>      uint8_t *buf;
> @@ -588,8 +604,10 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
> 
>      while (nb_sectors != 0) {
>          /* prepare next request */
> -        cluster_offset = get_cluster_offset(bs, sector_num << 9,
> -                                                 0, 0, 0, 0);
> +        cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
> +        if (cluster_offset < 0) {
> +            return cluster_offset;
> +        }

Oops, we're still holding s->lock. Combined with the bug that compressed
clusters are treated as errors now, this makes compressed images result
in an assertion failure:

qemu-system-x86_64: util/qemu-coroutine.c:138: qemu_aio_coroutine_enter: Assertion `!co->locks_held' failed.

>          index_in_cluster = sector_num & (s->cluster_sectors - 1);
>          n = s->cluster_sectors - index_in_cluster;
>          if (n > nb_sectors) {

Kevin

Re: [Qemu-devel] [PATCH 1/4] vpc: Check failure of bdrv_getlength()

[Kevin Wolf]

Am 07.08.2017 um 22:30 hat Eric Blake geschrieben:
> vpc_open() was checking for bdrv_getlength() failure in one, but
> not the other, location.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  block/vpc.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/block/vpc.c b/block/vpc.c
> index 574879ba7c..468d10ec1c 100644
> --- a/block/vpc.c
> +++ b/block/vpc.c
> @@ -219,6 +219,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>      uint64_t pagetable_size;
>      int disk_type = VHD_DYNAMIC;
>      int ret;
> +    int64_t bs_size;
> 
>      bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file,
>                                 false, errp);
> @@ -411,7 +412,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
>              }
>          }
> 
> -        if (s->free_data_block_offset > bdrv_getlength(bs->file->bs)) {
> +        bs_size = bdrv_getlength(bs->file->bs);
> +        if (bs_size < 0) {
> +            error_setg_errno(errp, -bs_size, "unable to learn image size");

I would start the error message with a capital letter for consistency
with other messages in this function. (It has obviously nothing to do
with my general preference for that style.)

> +            ret = bs_size;
> +            goto fail;
> +        }
> +        if (s->free_data_block_offset > bs_size) {
>              error_setg(errp, "block-vpc: free_data_block_offset points after "
>                               "the end of file. The image has been truncated.");
>              ret = -EINVAL;

Kevin

Re: [Qemu-devel] [PATCH 3/4] qcow2: Drop debugging dump_refcounts()

[Kevin Wolf]

Am 07.08.2017 um 22:30 hat Eric Blake geschrieben:
> It's been #if 0'd since its introduction in 2006, commit 585f8587.
> We can revive dead code if we need it, but in the meantime, it has
> bit-rotted (for example, not checking for failure in bdrv_getlength()).
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 4/4] qcow2: Check failure of bdrv_getlength()

[Kevin Wolf]

Am 07.08.2017 um 22:30 hat Eric Blake geschrieben:
> qcow2_co_pwritev_compressed() should not call bdrv_truncate()
> if determining the size failed.
> 
> Reported-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 2/4] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1021 bytes --]

On 08/08/2017 03:28 AM, Kevin Wolf wrote:
> Am 07.08.2017 um 22:30 hat Eric Blake geschrieben:
>> This also requires changing the return type of get_cluster_offset()
>> and adjusting all callers.
>>
>> Use osdep.h macros instead of open-coded rounding while in the
>> area.
>>
>> Reported-by: Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---

> qcow2 ended up with this prototype:
> 
> int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>                              unsigned int *bytes, uint64_t *cluster_offset)
> 
> Separating a full uin64_t offset by-reference parameter and an 0/-errno
> status return code feels a little cleaner to me.
> 
> And in fact, it's not only cleaner, but bit 63 is QCOW_OFLAG_COMPRESSED,
> so this patch breaks compressed images.

Looks like I'll be sending a v2, then.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]