[Qemu-devel] [PATCH for-2.10 v2 0/5] More bdrv_getlength() fixes

[Qemu-devel] [PATCH for-2.10 v2 0/5] More bdrv_getlength() fixes

[Eric Blake]

We already have a lot of bdrv_getlength() fixes in -rc2; so I think
this is still okay for -rc3.

v1 was here (with a typo'd subject line):
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01226.html

Since v1:
- patch 1: fix error message capitalization (Kevin, R-b kept)
- fix locking bug in original patch 2 (Kevin)
- split original patch 2 into two parts: signature update, and
added error checking (Kevin)
- check for unlikely integer overflow before bdrv_truncate (Jeff)

001/5:[0002] [FC] 'vpc: Check failure of bdrv_getlength()'
002/5:[down] 'qcow: Change signature of get_cluster_offset()'
003/5:[0048] [FC] 'qcow: Check failure of bdrv_getlength() and bdrv_truncate()'
004/5:[----] [--] 'qcow2: Drop debugging dump_refcounts()'
005/5:[----] [--] 'qcow2: Check failure of bdrv_getlength()'

Eric Blake (5):
  vpc: Check failure of bdrv_getlength()
  qcow: Change signature of get_cluster_offset()
  qcow: Check failure of bdrv_getlength() and bdrv_truncate()
  qcow2: Drop debugging dump_refcounts()
  qcow2: Check failure of bdrv_getlength()

 block/qcow.c  | 165 ++++++++++++++++++++++++++++++++++------------------------
 block/qcow2.c |  26 ++-------
 block/vpc.c   |   9 +++-
 3 files changed, 110 insertions(+), 90 deletions(-)

-- 
2.13.4

[Qemu-devel] [PATCH v2 1/5] 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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>

---
v2: error message tweak [Kevin]
---
 block/vpc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/block/vpc.c b/block/vpc.c
index 574879ba7c..82911ebead 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 v2 2/5] qcow: Change signature of get_cluster_offset()

[Eric Blake]

The old signature has an ambiguous meaning for a return of 0:
either no allocation was requested or necessary, or an error
occurred (but any errno associated with the error is lost to
the caller, which then has to assume EIO).

Better is to follow the example of qcow2, by changing the
signature to have a separate return value that cleanly
distinguishes between failure and success, along with a
parameter that cleanly holds a 64-bit value.  Then update all
callers.

While auditing that all return paths return a negative errno
(rather than -1), I also simplified places where we can pass
NULL rather than a local Error that just gets thrown away.

Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 block/qcow.c | 135 +++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 76 insertions(+), 59 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index c08cdc4a7b..d07bef6306 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -347,19 +347,21 @@ 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, 1 if *result is assigned, and negative
+ * 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 int get_cluster_offset(BlockDriverState *bs,
+                              uint64_t offset, int allocate,
+                              int compressed_size,
+                              int n_start, int n_end, uint64_t *result)
 {
     BDRVQcowState *s = bs->opaque;
-    int min_index, i, j, l1_index, l2_index;
+    int min_index, i, j, l1_index, l2_index, ret;
     uint64_t l2_offset, *l2_table, cluster_offset, tmp;
     uint32_t min_count;
     int new_l2_table;

+    *result = 0;
     l1_index = offset >> (s->l2_bits + s->cluster_bits);
     l2_offset = s->l1_table[l1_index];
     new_l2_table = 0;
@@ -373,10 +375,12 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
         /* update the L1 entry */
         s->l1_table[l1_index] = l2_offset;
         tmp = cpu_to_be64(l2_offset);
-        if (bdrv_pwrite_sync(bs->file,
-                s->l1_table_offset + l1_index * sizeof(tmp),
-                &tmp, sizeof(tmp)) < 0)
-            return 0;
+        ret = bdrv_pwrite_sync(bs->file,
+                               s->l1_table_offset + l1_index * sizeof(tmp),
+                               &tmp, sizeof(tmp));
+        if (ret < 0) {
+            return ret;
+        }
         new_l2_table = 1;
     }
     for(i = 0; i < L2_CACHE_SIZE; i++) {
@@ -403,14 +407,17 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
     l2_table = s->l2_cache + (min_index << s->l2_bits);
     if (new_l2_table) {
         memset(l2_table, 0, s->l2_size * sizeof(uint64_t));
-        if (bdrv_pwrite_sync(bs->file, l2_offset, l2_table,
-                s->l2_size * sizeof(uint64_t)) < 0)
-            return 0;
+        ret = bdrv_pwrite_sync(bs->file, l2_offset, l2_table,
+                               s->l2_size * sizeof(uint64_t));
+        if (ret < 0) {
+            return ret;
+        }
     } else {
-        if (bdrv_pread(bs->file, l2_offset, l2_table,
-                       s->l2_size * sizeof(uint64_t)) !=
-            s->l2_size * sizeof(uint64_t))
-            return 0;
+        ret = bdrv_pread(bs->file, l2_offset, l2_table,
+                         s->l2_size * sizeof(uint64_t));
+        if (ret < 0) {
+            return ret;
+        }
     }
     s->l2_cache_offsets[min_index] = l2_offset;
     s->l2_cache_counts[min_index] = 1;
@@ -427,16 +434,18 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
             /* if the cluster is already compressed, we must
                decompress it in the case it is not completely
                overwritten */
-            if (decompress_cluster(bs, cluster_offset) < 0)
-                return 0;
+            if (decompress_cluster(bs, cluster_offset) < 0) {
+                return -EIO;
+            }
             cluster_offset = bdrv_getlength(bs->file->bs);
             cluster_offset = (cluster_offset + s->cluster_size - 1) &
                 ~(s->cluster_size - 1);
             /* write the cluster content */
-            if (bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache,
-                            s->cluster_size) !=
-                s->cluster_size)
-                return -1;
+            ret = bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache,
+                              s->cluster_size);
+            if (ret < 0) {
+                return ret;
+            }
         } else {
             cluster_offset = bdrv_getlength(bs->file->bs);
             if (allocate == 1) {
@@ -454,20 +463,19 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
                     start_sect = (offset & ~(s->cluster_size - 1)) >> 9;
                     for(i = 0; i < s->cluster_sectors; i++) {
                         if (i < n_start || i >= n_end) {
-                            Error *err = NULL;
                             memset(s->cluster_data, 0x00, 512);
                             if (qcrypto_block_encrypt(s->crypto, start_sect + i,
                                                       s->cluster_data,
                                                       BDRV_SECTOR_SIZE,
-                                                      &err) < 0) {
-                                error_free(err);
-                                errno = EIO;
-                                return -1;
+                                                      NULL) < 0) {
+                                return -EIO;
+                            }
+                            ret = bdrv_pwrite(bs->file,
+                                              cluster_offset + i * 512,
+                                              s->cluster_data, 512);
+                            if (ret < 0) {
+                                return ret;
                             }
-                            if (bdrv_pwrite(bs->file,
-                                            cluster_offset + i * 512,
-                                            s->cluster_data, 512) != 512)
-                                return -1;
                         }
                     }
                 }
@@ -479,23 +487,29 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
         /* update L2 table */
         tmp = cpu_to_be64(cluster_offset);
         l2_table[l2_index] = tmp;
-        if (bdrv_pwrite_sync(bs->file, l2_offset + l2_index * sizeof(tmp),
-                &tmp, sizeof(tmp)) < 0)
-            return 0;
+        ret = bdrv_pwrite_sync(bs->file, l2_offset + l2_index * sizeof(tmp),
+                               &tmp, sizeof(tmp));
+        if (ret < 0) {
+            return ret;
+        }
     }
-    return cluster_offset;
+    *result = cluster_offset;
+    return 1;
 }

 static int64_t coroutine_fn qcow_co_get_block_status(BlockDriverState *bs,
         int64_t sector_num, int nb_sectors, int *pnum, BlockDriverState **file)
 {
     BDRVQcowState *s = bs->opaque;
-    int index_in_cluster, n;
+    int index_in_cluster, n, ret;
     uint64_t cluster_offset;

     qemu_co_mutex_lock(&s->lock);
-    cluster_offset = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0);
+    ret = get_cluster_offset(bs, sector_num << 9, 0, 0, 0, 0, &cluster_offset);
     qemu_co_mutex_unlock(&s->lock);
+    if (ret < 0) {
+        return ret;
+    }
     index_in_cluster = sector_num & (s->cluster_sectors - 1);
     n = s->cluster_sectors - index_in_cluster;
     if (n > nb_sectors)
@@ -572,7 +586,6 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
     QEMUIOVector hd_qiov;
     uint8_t *buf;
     void *orig_buf;
-    Error *err = NULL;

     if (qiov->niov > 1) {
         buf = orig_buf = qemu_try_blockalign(bs, qiov->size);
@@ -588,8 +601,11 @@ 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);
+        ret = get_cluster_offset(bs, sector_num << 9,
+                                 0, 0, 0, 0, &cluster_offset);
+        if (ret < 0) {
+            break;
+        }
         index_in_cluster = sector_num & (s->cluster_sectors - 1);
         n = s->cluster_sectors - index_in_cluster;
         if (n > nb_sectors) {
@@ -606,7 +622,7 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
                 ret = bdrv_co_readv(bs->backing, sector_num, n, &hd_qiov);
                 qemu_co_mutex_lock(&s->lock);
                 if (ret < 0) {
-                    goto fail;
+                    break;
                 }
             } else {
                 /* Note: in this case, no need to wait */
@@ -615,13 +631,15 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
         } else if (cluster_offset & QCOW_OFLAG_COMPRESSED) {
             /* add AIO support for compressed blocks ? */
             if (decompress_cluster(bs, cluster_offset) < 0) {
-                goto fail;
+                ret = -EIO;
+                break;
             }
             memcpy(buf,
                    s->cluster_cache + index_in_cluster * 512, 512 * n);
         } else {
             if ((cluster_offset & 511) != 0) {
-                goto fail;
+                ret = -EIO;
+                break;
             }
             hd_iov.iov_base = (void *)buf;
             hd_iov.iov_len = n * 512;
@@ -637,8 +655,9 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
             if (bs->encrypted) {
                 assert(s->crypto);
                 if (qcrypto_block_decrypt(s->crypto, sector_num, buf,
-                                          n * BDRV_SECTOR_SIZE, &err) < 0) {
-                    goto fail;
+                                          n * BDRV_SECTOR_SIZE, NULL) < 0) {
+                    ret = -EIO;
+                    break;
                 }
             }
         }
@@ -649,7 +668,6 @@ static coroutine_fn int qcow_co_readv(BlockDriverState *bs, int64_t sector_num,
         buf += n * 512;
     }

-done:
     qemu_co_mutex_unlock(&s->lock);

     if (qiov->niov > 1) {
@@ -658,11 +676,6 @@ done:
     }

     return ret;
-
-fail:
-    error_free(err);
-    ret = -EIO;
-    goto done;
 }

 static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
@@ -701,19 +714,20 @@ static coroutine_fn int qcow_co_writev(BlockDriverState *bs, int64_t sector_num,
         if (n > nb_sectors) {
             n = nb_sectors;
         }
-        cluster_offset = get_cluster_offset(bs, sector_num << 9, 1, 0,
-                                            index_in_cluster,
-                                            index_in_cluster + n);
+        ret = get_cluster_offset(bs, sector_num << 9, 1, 0,
+                                 index_in_cluster,
+                                 index_in_cluster + n, &cluster_offset);
+        if (ret < 0) {
+            break;
+        }
         if (!cluster_offset || (cluster_offset & 511) != 0) {
             ret = -EIO;
             break;
         }
         if (bs->encrypted) {
-            Error *err = NULL;
             assert(s->crypto);
             if (qcrypto_block_encrypt(s->crypto, sector_num, buf,
-                                      n * BDRV_SECTOR_SIZE, &err) < 0) {
-                error_free(err);
+                                      n * BDRV_SECTOR_SIZE, NULL) < 0) {
                 ret = -EIO;
                 break;
             }
@@ -1001,8 +1015,11 @@ qcow_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
         goto success;
     }
     qemu_co_mutex_lock(&s->lock);
-    cluster_offset = get_cluster_offset(bs, offset, 2, out_len, 0, 0);
+    ret = get_cluster_offset(bs, offset, 2, out_len, 0, 0, &cluster_offset);
     qemu_co_mutex_unlock(&s->lock);
+    if (ret < 0) {
+        goto fail;
+    }
     if (cluster_offset == 0) {
         ret = -EIO;
         goto fail;
-- 
2.13.4

[Qemu-devel] [PATCH v2 3/5] qcow: Check failure of bdrv_getlength() and bdrv_truncate()

[Eric Blake]

Omitting the check for whether bdrv_getlength() and bdrv_truncate()
failed meant that it was theoretically possible to return an
incorrect offset to the caller.  More likely, conditions for either
of these functions to fail would also cause one of our other calls
(such as bdrv_pread() or bdrv_pwrite_sync()) to also fail, but
auditing that we are safe is difficult compared to just patching
things to always forward on the error rather than ignoring it.

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>

---
v2: Rebase to new signature to not corrupt compressed clusters, and
fix lock handling [Kevin], check for truncate overflow [Jeff]
---
 block/qcow.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/block/qcow.c b/block/qcow.c
index d07bef6306..f450b00cfc 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -357,7 +357,8 @@ static int get_cluster_offset(BlockDriverState *bs,
 {
     BDRVQcowState *s = bs->opaque;
     int min_index, i, j, l1_index, l2_index, ret;
-    uint64_t l2_offset, *l2_table, cluster_offset, tmp;
+    int64_t l2_offset;
+    uint64_t *l2_table, cluster_offset, tmp;
     uint32_t min_count;
     int new_l2_table;

@@ -370,8 +371,11 @@ static int 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);
@@ -438,8 +442,10 @@ static int get_cluster_offset(BlockDriverState *bs,
                 return -EIO;
             }
             cluster_offset = bdrv_getlength(bs->file->bs);
-            cluster_offset = (cluster_offset + s->cluster_size - 1) &
-                ~(s->cluster_size - 1);
+            if ((int64_t) cluster_offset < 0) {
+                return cluster_offset;
+            }
+            cluster_offset = QEMU_ALIGN_UP(cluster_offset, s->cluster_size);
             /* write the cluster content */
             ret = bdrv_pwrite(bs->file, cluster_offset, s->cluster_cache,
                               s->cluster_size);
@@ -448,12 +454,20 @@ static int get_cluster_offset(BlockDriverState *bs,
             }
         } else {
             cluster_offset = bdrv_getlength(bs->file->bs);
+            if ((int64_t) cluster_offset < 0) {
+                return cluster_offset;
+            }
             if (allocate == 1) {
                 /* 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);
+                if (cluster_offset + s->cluster_size > INT64_MAX) {
+                    return -E2BIG;
+                }
+                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 &&
-- 
2.13.4

[Qemu-devel] [PATCH v2 4/5] 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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: John Snow <jsnow@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 v2 5/5] 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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: John Snow <jsnow@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 for-2.10 v2 0/5] More bdrv_getlength() fixes

[Kevin Wolf]

Am 09.08.2017 um 22:38 hat Eric Blake geschrieben:
> We already have a lot of bdrv_getlength() fixes in -rc2; so I think
> this is still okay for -rc3.
> 
> v1 was here (with a typo'd subject line):
> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01226.html
> 
> Since v1:
> - patch 1: fix error message capitalization (Kevin, R-b kept)
> - fix locking bug in original patch 2 (Kevin)
> - split original patch 2 into two parts: signature update, and
> added error checking (Kevin)
> - check for unlikely integer overflow before bdrv_truncate (Jeff)
> 
> 001/5:[0002] [FC] 'vpc: Check failure of bdrv_getlength()'
> 002/5:[down] 'qcow: Change signature of get_cluster_offset()'
> 003/5:[0048] [FC] 'qcow: Check failure of bdrv_getlength() and bdrv_truncate()'
> 004/5:[----] [--] 'qcow2: Drop debugging dump_refcounts()'
> 005/5:[----] [--] 'qcow2: Check failure of bdrv_getlength()'

Looks good to me, but as the bug is far from being critical, I'd rather
apply the more complex qcow1 patches only to block-next. The vpc and
qcow2 parts seems a lot less risky, so 2.10 should be okay for them.

What do you think?

Kevin

Re: [Qemu-devel] [PATCH for-2.10 v2 0/5] More bdrv_getlength() fixes

[Eric Blake]

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

On 08/10/2017 08:02 AM, Kevin Wolf wrote:
> Am 09.08.2017 um 22:38 hat Eric Blake geschrieben:
>> We already have a lot of bdrv_getlength() fixes in -rc2; so I think
>> this is still okay for -rc3.
>>
>> v1 was here (with a typo'd subject line):
>> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01226.html
>>
>> Since v1:
>> - patch 1: fix error message capitalization (Kevin, R-b kept)
>> - fix locking bug in original patch 2 (Kevin)
>> - split original patch 2 into two parts: signature update, and
>> added error checking (Kevin)
>> - check for unlikely integer overflow before bdrv_truncate (Jeff)
>>
>> 001/5:[0002] [FC] 'vpc: Check failure of bdrv_getlength()'
>> 002/5:[down] 'qcow: Change signature of get_cluster_offset()'
>> 003/5:[0048] [FC] 'qcow: Check failure of bdrv_getlength() and bdrv_truncate()'
>> 004/5:[----] [--] 'qcow2: Drop debugging dump_refcounts()'
>> 005/5:[----] [--] 'qcow2: Check failure of bdrv_getlength()'
> 
> Looks good to me, but as the bug is far from being critical, I'd rather
> apply the more complex qcow1 patches only to block-next. The vpc and
> qcow2 parts seems a lot less risky, so 2.10 should be okay for them.
> 
> What do you think?

The argument for NOT doing the qcow changes (patches 2 and 3): the only
place where we are not checking for failures is part of
get_cluster_offset() - but in all likelihood, if we were unable to
determine or change the length of the backing file, we will have nearby
problems that will ultimately cause failure soon enough.  Furthermore,
it's not a regression (we've had several releases with the problem), and
qcow is not a good format (it's painfully slow, and we strongly
recommend qcow2 instead) - so no one will be hitting any actual bugs in
practice.

I'll trust your judgment as maintainer, so taking just 1, 4, and 5 in
2.10 is fine.

-- 
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 --]

Re: [Qemu-devel] [PATCH for-2.10 v2 0/5] More bdrv_getlength() fixes

[Kevin Wolf]

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

Am 10.08.2017 um 17:08 hat Eric Blake geschrieben:
> On 08/10/2017 08:02 AM, Kevin Wolf wrote:
> > Am 09.08.2017 um 22:38 hat Eric Blake geschrieben:
> >> We already have a lot of bdrv_getlength() fixes in -rc2; so I think
> >> this is still okay for -rc3.
> >>
> >> v1 was here (with a typo'd subject line):
> >> https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01226.html
> >>
> >> Since v1:
> >> - patch 1: fix error message capitalization (Kevin, R-b kept)
> >> - fix locking bug in original patch 2 (Kevin)
> >> - split original patch 2 into two parts: signature update, and
> >> added error checking (Kevin)
> >> - check for unlikely integer overflow before bdrv_truncate (Jeff)
> >>
> >> 001/5:[0002] [FC] 'vpc: Check failure of bdrv_getlength()'
> >> 002/5:[down] 'qcow: Change signature of get_cluster_offset()'
> >> 003/5:[0048] [FC] 'qcow: Check failure of bdrv_getlength() and bdrv_truncate()'
> >> 004/5:[----] [--] 'qcow2: Drop debugging dump_refcounts()'
> >> 005/5:[----] [--] 'qcow2: Check failure of bdrv_getlength()'
> > 
> > Looks good to me, but as the bug is far from being critical, I'd rather
> > apply the more complex qcow1 patches only to block-next. The vpc and
> > qcow2 parts seems a lot less risky, so 2.10 should be okay for them.
> > 
> > What do you think?
> 
> The argument for NOT doing the qcow changes (patches 2 and 3): the only
> place where we are not checking for failures is part of
> get_cluster_offset() - but in all likelihood, if we were unable to
> determine or change the length of the backing file, we will have nearby
> problems that will ultimately cause failure soon enough.  Furthermore,
> it's not a regression (we've had several releases with the problem), and
> qcow is not a good format (it's painfully slow, and we strongly
> recommend qcow2 instead) - so no one will be hitting any actual bugs in
> practice.
> 
> I'll trust your judgment as maintainer, so taking just 1, 4, and 5 in
> 2.10 is fine.

Thanks, applied the patches to block and block-next, respectively.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]