[Qemu-devel] [PATCH v4 0/4] qemu-img: fix bugs when cluster size is larger than the default value

[Qemu-devel] [PATCH v4 0/4] qemu-img: fix bugs when cluster size is larger than the default value

[Hu Tao]

This series fixes several bugs of qcow2 when doing preallocation with a
cluster_size larger than the default value.

v4:
  - qemu-iotests: use _filter_testdir directly.
  - remove n_end in qcow2_co_writev() that is never used.

Hu Tao (4):
  qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()
  qcow2: fix offset overflow in qcow2_alloc_clusters_at()
  qcow2: check for NULL l2meta
  qemu-iotests: add test for qcow2 preallocation with different cluster
    sizes

 block/qcow2-cluster.c      | 14 +++++------
 block/qcow2-refcount.c     |  8 +++++-
 block/qcow2.c              | 36 +++++++++++++-------------
 block/qcow2.h              |  2 +-
 tests/qemu-iotests/079     | 63 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/079.out | 32 +++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 trace-events               |  2 +-
 8 files changed, 128 insertions(+), 30 deletions(-)
 create mode 100755 tests/qemu-iotests/079
 create mode 100644 tests/qemu-iotests/079.out

-- 
1.8.5.2.229.g4448466

[Qemu-devel] [PATCH v4 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()

[Hu Tao]

n_start can be actually calculated from offset. The number of
sectors to be allocated(n_end - n_start) can be passed in in
num. By removing n_start and n_end, we can save two parameters.

The side effect is there is a bug in qcow2.c:preallocate() that
passes incorrect n_start to qcow2_alloc_cluster_offset() is
fixed. The bug can be triggerred by a larger cluster size than
the default value(65536), for example:

./qemu-img create -f qcow2 \
  -o 'cluster_size=131072,preallocation=metadata' file.img 4G

Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2-cluster.c | 14 ++++++--------
 block/qcow2.c         | 11 +++--------
 block/qcow2.h         |  2 +-
 trace-events          |  2 +-
 4 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 8534084..c57f39d 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1182,7 +1182,7 @@ fail:
  * Return 0 on success and -errno in error cases
  */
 int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
-    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m)
+    int *num, uint64_t *host_offset, QCowL2Meta **m)
 {
     BDRVQcowState *s = bs->opaque;
     uint64_t start, remaining;
@@ -1190,15 +1190,13 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
     uint64_t cur_bytes;
     int ret;
 
-    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset,
-                                      n_start, n_end);
+    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *num);
 
-    assert(n_start * BDRV_SECTOR_SIZE == offset_into_cluster(s, offset));
-    offset = start_of_cluster(s, offset);
+    assert((offset & ~BDRV_SECTOR_MASK) == 0);
 
 again:
-    start = offset + (n_start << BDRV_SECTOR_BITS);
-    remaining = (n_end - n_start) << BDRV_SECTOR_BITS;
+    start = offset;
+    remaining = *num << BDRV_SECTOR_BITS;
     cluster_offset = 0;
     *host_offset = 0;
     cur_bytes = 0;
@@ -1284,7 +1282,7 @@ again:
         }
     }
 
-    *num = (n_end - n_start) - (remaining >> BDRV_SECTOR_BITS);
+    *num -= remaining >> BDRV_SECTOR_BITS;
     assert(*num > 0);
     assert(*host_offset != 0);
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 8ec9db1..0a310cc 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -992,7 +992,6 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
 {
     BDRVQcowState *s = bs->opaque;
     int index_in_cluster;
-    int n_end;
     int ret;
     int cur_nr_sectors; /* number of sectors in current iteration */
     uint64_t cluster_offset;
@@ -1016,14 +1015,10 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
 
         trace_qcow2_writev_start_part(qemu_coroutine_self());
         index_in_cluster = sector_num & (s->cluster_sectors - 1);
-        n_end = index_in_cluster + remaining_sectors;
-        if (s->crypt_method &&
-            n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors) {
-            n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
-        }
+        cur_nr_sectors = remaining_sectors;
 
         ret = qcow2_alloc_cluster_offset(bs, sector_num << 9,
-            index_in_cluster, n_end, &cur_nr_sectors, &cluster_offset, &l2meta);
+            &cur_nr_sectors, &cluster_offset, &l2meta);
         if (ret < 0) {
             goto fail;
         }
@@ -1400,7 +1395,7 @@ static int preallocate(BlockDriverState *bs)
 
     while (nb_sectors) {
         num = MIN(nb_sectors, INT_MAX >> 9);
-        ret = qcow2_alloc_cluster_offset(bs, offset, 0, num, &num,
+        ret = qcow2_alloc_cluster_offset(bs, offset, &num,
                                          &host_offset, &meta);
         if (ret < 0) {
             return ret;
diff --git a/block/qcow2.h b/block/qcow2.h
index 303eb26..84e1344 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -468,7 +468,7 @@ void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
 int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
     int *num, uint64_t *cluster_offset);
 int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
-    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m);
+    int *num, uint64_t *host_offset, QCowL2Meta **m);
 uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
                                          uint64_t offset,
                                          int compressed_size);
diff --git a/trace-events b/trace-events
index 9f4456a..9b4e586 100644
--- a/trace-events
+++ b/trace-events
@@ -494,7 +494,7 @@ qcow2_writev_done_part(void *co, int cur_nr_sectors) "co %p cur_nr_sectors %d"
 qcow2_writev_data(void *co, uint64_t offset) "co %p offset %" PRIx64
 
 # block/qcow2-cluster.c
-qcow2_alloc_clusters_offset(void *co, uint64_t offset, int n_start, int n_end) "co %p offet %" PRIx64 " n_start %d n_end %d"
+qcow2_alloc_clusters_offset(void *co, uint64_t offset, int num) "co %p offet %" PRIx64 " num %d"
 qcow2_handle_copied(void *co, uint64_t guest_offset, uint64_t host_offset, uint64_t bytes) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " bytes %" PRIx64
 qcow2_handle_alloc(void *co, uint64_t guest_offset, uint64_t host_offset, uint64_t bytes) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " bytes %" PRIx64
 qcow2_do_alloc_clusters_offset(void *co, uint64_t guest_offset, uint64_t host_offset, int nb_clusters) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " nb_clusters %d"
-- 
1.8.5.2.229.g4448466

[Qemu-devel] [PATCH v4 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at()

[Hu Tao]

When cluster size is big enough it can lead offset overflow
in qcow2_alloc_clusters_at(). This patch fixes it.

The allocation each time is stopped at L2 table boundary
(see handle_alloc()), so the possible maximum bytes could be

  2^(cluster_bits - 3 + cluster_bits)

so int is safe for cluster_bits<=17, unsafe otherwise.

Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2-refcount.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index c974abe..8712d8b 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -676,7 +676,13 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
     BDRVQcowState *s = bs->opaque;
     uint64_t cluster_index;
     uint64_t old_free_cluster_index;
-    int i, refcount, ret;
+    uint64_t i;
+    int refcount, ret;
+
+    assert(nb_clusters >= 0);
+    if (nb_clusters == 0) {
+        return 0;
+    }
 
     /* Check how many clusters there are free */
     cluster_index = offset >> s->cluster_bits;
-- 
1.8.5.2.229.g4448466

[Qemu-devel] [PATCH v4 3/4] qcow2: check for NULL l2meta

[Hu Tao]

In case of do preallocating metadata with a large cluster size,
qcow2_alloc_cluster_offset() can allocate nothing and returns
a NULL l2meta. This patch checks for it and link2 l2 with only
valid l2meta.

Replace 9 and 512 with BDRV_SECTOR_BITS, BDRV_SECTOR_SIZE
respectively while at the function.

Reviewed-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 0a310cc..f989247 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1390,22 +1390,24 @@ static int preallocate(BlockDriverState *bs)
     int ret;
     QCowL2Meta *meta;
 
-    nb_sectors = bdrv_getlength(bs) >> 9;
+    nb_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
     offset = 0;
 
     while (nb_sectors) {
-        num = MIN(nb_sectors, INT_MAX >> 9);
+        num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS);
         ret = qcow2_alloc_cluster_offset(bs, offset, &num,
                                          &host_offset, &meta);
         if (ret < 0) {
             return ret;
         }
 
-        ret = qcow2_alloc_cluster_link_l2(bs, meta);
-        if (ret < 0) {
-            qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters,
-                                    QCOW2_DISCARD_NEVER);
-            return ret;
+        if (meta) {
+            ret = qcow2_alloc_cluster_link_l2(bs, meta);
+            if (ret < 0) {
+                qcow2_free_any_clusters(bs, meta->alloc_offset,
+                                        meta->nb_clusters, QCOW2_DISCARD_NEVER);
+                return ret;
+            }
         }
 
         /* There are no dependent requests, but we need to remove our request
@@ -1417,7 +1419,7 @@ static int preallocate(BlockDriverState *bs)
         /* TODO Preallocate data if requested */
 
         nb_sectors -= num;
-        offset += num << 9;
+        offset += num << BDRV_SECTOR_BITS;
     }
 
     /*
@@ -1426,9 +1428,10 @@ static int preallocate(BlockDriverState *bs)
      * EOF). Extend the image to the last allocated sector.
      */
     if (host_offset != 0) {
-        uint8_t buf[512];
-        memset(buf, 0, 512);
-        ret = bdrv_write(bs->file, (host_offset >> 9) + num - 1, buf, 1);
+        uint8_t buf[BDRV_SECTOR_SIZE];
+        memset(buf, 0, BDRV_SECTOR_SIZE);
+        ret = bdrv_write(bs->file, (host_offset >> BDRV_SECTOR_BITS) + num - 1,
+                         buf, 1);
         if (ret < 0) {
             return ret;
         }
-- 
1.8.5.2.229.g4448466

[Qemu-devel] [PATCH v4 4/4] qemu-iotests: add test for qcow2 preallocation with different cluster sizes

[Hu Tao]

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 tests/qemu-iotests/079     | 63 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/079.out | 32 +++++++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 96 insertions(+)
 create mode 100755 tests/qemu-iotests/079
 create mode 100644 tests/qemu-iotests/079.out

diff --git a/tests/qemu-iotests/079 b/tests/qemu-iotests/079
new file mode 100755
index 0000000..2142bbb
--- /dev/null
+++ b/tests/qemu-iotests/079
@@ -0,0 +1,63 @@
+#!/bin/bash
+#
+# Test qcow2 preallocation with different cluster_sizes
+#
+# Copyright (C) 2014 Fujitsu.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=hutao@cn.fujitsu.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt qcow2
+_supported_proto file
+_supported_os Linux
+
+function test_qemu_img()
+{
+    echo qemu-img "$@" | _filter_testdir
+    $QEMU_IMG "$@" 2>&1 | _filter_testdir
+    echo
+}
+
+echo "=== Check option preallocation and cluster_size ==="
+echo
+cluster_sizes="16384 32768 65536 131072 262144 524288 1048576 2097152 4194304"
+
+for s in $cluster_sizes; do
+    test_qemu_img create -f $IMGFMT -o preallocation=metadata,cluster_size=$s "$TEST_IMG" 4G
+done
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/079.out b/tests/qemu-iotests/079.out
new file mode 100644
index 0000000..ef4b8c9
--- /dev/null
+++ b/tests/qemu-iotests/079.out
@@ -0,0 +1,32 @@
+QA output created by 079
+=== Check option preallocation and cluster_size ===
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=16384 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=16384 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=32768 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=32768 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=65536 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=65536 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=131072 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=131072 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=262144 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=262144 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=524288 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=524288 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=1048576 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=1048576 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=2097152 TEST_DIR/t.qcow2 4G
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=2097152 preallocation='metadata' lazy_refcounts=off
+
+qemu-img create -f qcow2 -o preallocation=metadata,cluster_size=4194304 TEST_DIR/t.qcow2 4G
+qemu-img: TEST_DIR/t.qcow2: Cluster size must be a power of two between 512 and 2048k
+Formatting 'TEST_DIR/t.qcow2', fmt=qcow2 size=4294967296 encryption=off cluster_size=4194304 preallocation='metadata' lazy_refcounts=off
+
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index cc750c9..3bb22c2 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -79,3 +79,4 @@
 070 rw auto
 073 rw auto
 074 rw auto
+079 rw auto
-- 
1.8.5.2.229.g4448466

Re: [Qemu-devel] [PATCH v4 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()

[Kevin Wolf]

Am 23.01.2014 um 04:04 hat Hu Tao geschrieben:
> n_start can be actually calculated from offset. The number of
> sectors to be allocated(n_end - n_start) can be passed in in
> num. By removing n_start and n_end, we can save two parameters.
> 
> The side effect is there is a bug in qcow2.c:preallocate() that
> passes incorrect n_start to qcow2_alloc_cluster_offset() is
> fixed. The bug can be triggerred by a larger cluster size than
> the default value(65536), for example:
> 
> ./qemu-img create -f qcow2 \
>   -o 'cluster_size=131072,preallocation=metadata' file.img 4G
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  block/qcow2-cluster.c | 14 ++++++--------
>  block/qcow2.c         | 11 +++--------
>  block/qcow2.h         |  2 +-
>  trace-events          |  2 +-
>  4 files changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 8534084..c57f39d 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1182,7 +1182,7 @@ fail:
>   * Return 0 on success and -errno in error cases
>   */
>  int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> -    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m)
> +    int *num, uint64_t *host_offset, QCowL2Meta **m)
>  {
>      BDRVQcowState *s = bs->opaque;
>      uint64_t start, remaining;
> @@ -1190,15 +1190,13 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
>      uint64_t cur_bytes;
>      int ret;
>  
> -    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset,
> -                                      n_start, n_end);
> +    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *num);
>  
> -    assert(n_start * BDRV_SECTOR_SIZE == offset_into_cluster(s, offset));
> -    offset = start_of_cluster(s, offset);
> +    assert((offset & ~BDRV_SECTOR_MASK) == 0);
>  
>  again:
> -    start = offset + (n_start << BDRV_SECTOR_BITS);
> -    remaining = (n_end - n_start) << BDRV_SECTOR_BITS;
> +    start = offset;
> +    remaining = *num << BDRV_SECTOR_BITS;
>      cluster_offset = 0;
>      *host_offset = 0;
>      cur_bytes = 0;
> @@ -1284,7 +1282,7 @@ again:
>          }
>      }
>  
> -    *num = (n_end - n_start) - (remaining >> BDRV_SECTOR_BITS);
> +    *num -= remaining >> BDRV_SECTOR_BITS;
>      assert(*num > 0);
>      assert(*host_offset != 0);
>  
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 8ec9db1..0a310cc 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -992,7 +992,6 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
>  {
>      BDRVQcowState *s = bs->opaque;
>      int index_in_cluster;
> -    int n_end;
>      int ret;
>      int cur_nr_sectors; /* number of sectors in current iteration */
>      uint64_t cluster_offset;
> @@ -1016,14 +1015,10 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
>  
>          trace_qcow2_writev_start_part(qemu_coroutine_self());
>          index_in_cluster = sector_num & (s->cluster_sectors - 1);
> -        n_end = index_in_cluster + remaining_sectors;
> -        if (s->crypt_method &&
> -            n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors) {
> -            n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
> -        }
> +        cur_nr_sectors = remaining_sectors;

You still need to limit cur_nr_sectors for the encrypted case, otherwise
you get a buffer overflow of cluster_data later in the function. My
complaint in v3 was not that you have the limiting, but that applying it
to n_end doesn't have any effect any more, you need to apply it to
cur_nr_sectors.

Kevin

Re: [Qemu-devel] [PATCH v4 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()

[Benoît Canet]

Le Thursday 23 Jan 2014 à 11:04:05 (+0800), Hu Tao a écrit :
> n_start can be actually calculated from offset. The number of
> sectors to be allocated(n_end - n_start) can be passed in in
> num. By removing n_start and n_end, we can save two parameters.
> 
> The side effect is there is a bug in qcow2.c:preallocate() that
> passes incorrect n_start to qcow2_alloc_cluster_offset() is
> fixed. The bug can be triggerred by a larger cluster size than
> the default value(65536), for example:
> 
> ./qemu-img create -f qcow2 \
>   -o 'cluster_size=131072,preallocation=metadata' file.img 4G
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  block/qcow2-cluster.c | 14 ++++++--------
>  block/qcow2.c         | 11 +++--------
>  block/qcow2.h         |  2 +-
>  trace-events          |  2 +-
>  4 files changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 8534084..c57f39d 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1182,7 +1182,7 @@ fail:
>   * Return 0 on success and -errno in error cases
>   */
>  int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> -    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m)
> +    int *num, uint64_t *host_offset, QCowL2Meta **m)
>  {
>      BDRVQcowState *s = bs->opaque;
>      uint64_t start, remaining;
> @@ -1190,15 +1190,13 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
>      uint64_t cur_bytes;
>      int ret;
>  
> -    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset,
> -                                      n_start, n_end);
> +    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *num);
>  
> -    assert(n_start * BDRV_SECTOR_SIZE == offset_into_cluster(s, offset));
> -    offset = start_of_cluster(s, offset);
> +    assert((offset & ~BDRV_SECTOR_MASK) == 0);

Why replace something that would round gently an unaligned offset
(start_of_cluster) by an assert that would make QEMU exit ?

Best regards

Benoît

>  
>  again:
> -    start = offset + (n_start << BDRV_SECTOR_BITS);
> -    remaining = (n_end - n_start) << BDRV_SECTOR_BITS;
> +    start = offset;
> +    remaining = *num << BDRV_SECTOR_BITS;
>      cluster_offset = 0;
>      *host_offset = 0;
>      cur_bytes = 0;
> @@ -1284,7 +1282,7 @@ again:
>          }
>      }
>  
> -    *num = (n_end - n_start) - (remaining >> BDRV_SECTOR_BITS);
> +    *num -= remaining >> BDRV_SECTOR_BITS;
>      assert(*num > 0);
>      assert(*host_offset != 0);
>  
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 8ec9db1..0a310cc 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -992,7 +992,6 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
>  {
>      BDRVQcowState *s = bs->opaque;
>      int index_in_cluster;
> -    int n_end;
>      int ret;
>      int cur_nr_sectors; /* number of sectors in current iteration */
>      uint64_t cluster_offset;
> @@ -1016,14 +1015,10 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
>  
>          trace_qcow2_writev_start_part(qemu_coroutine_self());
>          index_in_cluster = sector_num & (s->cluster_sectors - 1);
> -        n_end = index_in_cluster + remaining_sectors;
> -        if (s->crypt_method &&
> -            n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors) {
> -            n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
> -        }
> +        cur_nr_sectors = remaining_sectors;
>  
>          ret = qcow2_alloc_cluster_offset(bs, sector_num << 9,
> -            index_in_cluster, n_end, &cur_nr_sectors, &cluster_offset, &l2meta);
> +            &cur_nr_sectors, &cluster_offset, &l2meta);
>          if (ret < 0) {
>              goto fail;
>          }
> @@ -1400,7 +1395,7 @@ static int preallocate(BlockDriverState *bs)
>  
>      while (nb_sectors) {
>          num = MIN(nb_sectors, INT_MAX >> 9);
> -        ret = qcow2_alloc_cluster_offset(bs, offset, 0, num, &num,
> +        ret = qcow2_alloc_cluster_offset(bs, offset, &num,
>                                           &host_offset, &meta);
>          if (ret < 0) {
>              return ret;
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 303eb26..84e1344 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -468,7 +468,7 @@ void qcow2_encrypt_sectors(BDRVQcowState *s, int64_t sector_num,
>  int qcow2_get_cluster_offset(BlockDriverState *bs, uint64_t offset,
>      int *num, uint64_t *cluster_offset);
>  int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> -    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m);
> +    int *num, uint64_t *host_offset, QCowL2Meta **m);
>  uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
>                                           uint64_t offset,
>                                           int compressed_size);
> diff --git a/trace-events b/trace-events
> index 9f4456a..9b4e586 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -494,7 +494,7 @@ qcow2_writev_done_part(void *co, int cur_nr_sectors) "co %p cur_nr_sectors %d"
>  qcow2_writev_data(void *co, uint64_t offset) "co %p offset %" PRIx64
>  
>  # block/qcow2-cluster.c
> -qcow2_alloc_clusters_offset(void *co, uint64_t offset, int n_start, int n_end) "co %p offet %" PRIx64 " n_start %d n_end %d"
> +qcow2_alloc_clusters_offset(void *co, uint64_t offset, int num) "co %p offet %" PRIx64 " num %d"
>  qcow2_handle_copied(void *co, uint64_t guest_offset, uint64_t host_offset, uint64_t bytes) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " bytes %" PRIx64
>  qcow2_handle_alloc(void *co, uint64_t guest_offset, uint64_t host_offset, uint64_t bytes) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " bytes %" PRIx64
>  qcow2_do_alloc_clusters_offset(void *co, uint64_t guest_offset, uint64_t host_offset, int nb_clusters) "co %p guest_offet %" PRIx64 " host_offset %" PRIx64 " nb_clusters %d"
> -- 
> 1.8.5.2.229.g4448466
> 
> 

Re: [Qemu-devel] [PATCH v4 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at()

[Benoît Canet]

Le Thursday 23 Jan 2014 à 11:04:06 (+0800), Hu Tao a écrit :
> When cluster size is big enough it can lead offset overflow

Maybe "it can lead to an offset overflow in"
> in qcow2_alloc_clusters_at(). This patch fixes it.
> 
> The allocation each time is stopped at L2 table boundary
"The allocation is stopped each time at"

> (see handle_alloc()), so the possible maximum bytes could be
> 
>   2^(cluster_bits - 3 + cluster_bits)

So if understand cluster_bits - 3 is used to compute the number of entry by L2
and the additional cluster_bits is to take into account each clusters referenced
by the L2 entries.
It makes sense.

> 
> so int is safe for cluster_bits<=17, unsafe otherwise.
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  block/qcow2-refcount.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index c974abe..8712d8b 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -676,7 +676,13 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
>      BDRVQcowState *s = bs->opaque;
>      uint64_t cluster_index;
>      uint64_t old_free_cluster_index;
> -    int i, refcount, ret;
> +    uint64_t i;
> +    int refcount, ret;
> +


> +    assert(nb_clusters >= 0);
> +    if (nb_clusters == 0) {
> +        return 0;
> +    }
^
Adding a a line on the commit message about this assertion and chunk of code
would be helpful.

Best regards

Benoît

>  
>      /* Check how many clusters there are free */
>      cluster_index = offset >> s->cluster_bits;
> -- 
> 1.8.5.2.229.g4448466
> 
> 

Re: [Qemu-devel] [PATCH v4 3/4] qcow2: check for NULL l2meta

[Benoît Canet]

Le Thursday 23 Jan 2014 à 11:04:07 (+0800), Hu Tao a écrit :
> In case of do preallocating metadata with a large cluster size,
In the case of a metadata preallocation with

> qcow2_alloc_cluster_offset() can allocate nothing and returns
> a NULL l2meta. This patch checks for it and link2 l2 with only
> valid l2meta.
> 
> Replace 9 and 512 with BDRV_SECTOR_BITS, BDRV_SECTOR_SIZE
> respectively while at the function.
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>  block/qcow2.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 0a310cc..f989247 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1390,22 +1390,24 @@ static int preallocate(BlockDriverState *bs)
>      int ret;
>      QCowL2Meta *meta;
>  
> -    nb_sectors = bdrv_getlength(bs) >> 9;
> +    nb_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
>      offset = 0;
>  
>      while (nb_sectors) {
> -        num = MIN(nb_sectors, INT_MAX >> 9);
> +        num = MIN(nb_sectors, INT_MAX >> BDRV_SECTOR_BITS);
>          ret = qcow2_alloc_cluster_offset(bs, offset, &num,
>                                           &host_offset, &meta);
>          if (ret < 0) {
>              return ret;
>          }
>  
> -        ret = qcow2_alloc_cluster_link_l2(bs, meta);
> -        if (ret < 0) {
> -            qcow2_free_any_clusters(bs, meta->alloc_offset, meta->nb_clusters,
> -                                    QCOW2_DISCARD_NEVER);
> -            return ret;
> +        if (meta) {
> +            ret = qcow2_alloc_cluster_link_l2(bs, meta);
> +            if (ret < 0) {
> +                qcow2_free_any_clusters(bs, meta->alloc_offset,
> +                                        meta->nb_clusters, QCOW2_DISCARD_NEVER);
> +                return ret;
> +            }
>          }

Maybe if (meta) could include the following line to remove another extra test.
QLIST_REMOVE(meta, next_in_flight);

>  
>          /* There are no dependent requests, but we need to remove our request
> @@ -1417,7 +1419,7 @@ static int preallocate(BlockDriverState *bs)
>          /* TODO Preallocate data if requested */
>  
>          nb_sectors -= num;
> -        offset += num << 9;
> +        offset += num << BDRV_SECTOR_BITS;
>      }
>  
>      /*
> @@ -1426,9 +1428,10 @@ static int preallocate(BlockDriverState *bs)
>       * EOF). Extend the image to the last allocated sector.
>       */
>      if (host_offset != 0) {
> -        uint8_t buf[512];
> -        memset(buf, 0, 512);
> -        ret = bdrv_write(bs->file, (host_offset >> 9) + num - 1, buf, 1);
> +        uint8_t buf[BDRV_SECTOR_SIZE];
> +        memset(buf, 0, BDRV_SECTOR_SIZE);
> +        ret = bdrv_write(bs->file, (host_offset >> BDRV_SECTOR_BITS) + num - 1,
> +                         buf, 1);
>          if (ret < 0) {
>              return ret;
>          }
> -- 
> 1.8.5.2.229.g4448466
> 
> 

Re: [Qemu-devel] [PATCH v4 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()

[Hu Tao]

On Thu, Jan 23, 2014 at 03:29:04PM +0100, Kevin Wolf wrote:
> Am 23.01.2014 um 04:04 hat Hu Tao geschrieben:
> > n_start can be actually calculated from offset. The number of
> > sectors to be allocated(n_end - n_start) can be passed in in
> > num. By removing n_start and n_end, we can save two parameters.
> > 
> > The side effect is there is a bug in qcow2.c:preallocate() that
> > passes incorrect n_start to qcow2_alloc_cluster_offset() is
> > fixed. The bug can be triggerred by a larger cluster size than
> > the default value(65536), for example:
> > 
> > ./qemu-img create -f qcow2 \
> >   -o 'cluster_size=131072,preallocation=metadata' file.img 4G
> > 
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  block/qcow2-cluster.c | 14 ++++++--------
> >  block/qcow2.c         | 11 +++--------
> >  block/qcow2.h         |  2 +-
> >  trace-events          |  2 +-
> >  4 files changed, 11 insertions(+), 18 deletions(-)
> > 
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index 8534084..c57f39d 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -1182,7 +1182,7 @@ fail:
> >   * Return 0 on success and -errno in error cases
> >   */
> >  int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> > -    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m)
> > +    int *num, uint64_t *host_offset, QCowL2Meta **m)
> >  {
> >      BDRVQcowState *s = bs->opaque;
> >      uint64_t start, remaining;
> > @@ -1190,15 +1190,13 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> >      uint64_t cur_bytes;
> >      int ret;
> >  
> > -    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset,
> > -                                      n_start, n_end);
> > +    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *num);
> >  
> > -    assert(n_start * BDRV_SECTOR_SIZE == offset_into_cluster(s, offset));
> > -    offset = start_of_cluster(s, offset);
> > +    assert((offset & ~BDRV_SECTOR_MASK) == 0);
> >  
> >  again:
> > -    start = offset + (n_start << BDRV_SECTOR_BITS);
> > -    remaining = (n_end - n_start) << BDRV_SECTOR_BITS;
> > +    start = offset;
> > +    remaining = *num << BDRV_SECTOR_BITS;
> >      cluster_offset = 0;
> >      *host_offset = 0;
> >      cur_bytes = 0;
> > @@ -1284,7 +1282,7 @@ again:
> >          }
> >      }
> >  
> > -    *num = (n_end - n_start) - (remaining >> BDRV_SECTOR_BITS);
> > +    *num -= remaining >> BDRV_SECTOR_BITS;
> >      assert(*num > 0);
> >      assert(*host_offset != 0);
> >  
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 8ec9db1..0a310cc 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -992,7 +992,6 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
> >  {
> >      BDRVQcowState *s = bs->opaque;
> >      int index_in_cluster;
> > -    int n_end;
> >      int ret;
> >      int cur_nr_sectors; /* number of sectors in current iteration */
> >      uint64_t cluster_offset;
> > @@ -1016,14 +1015,10 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
> >  
> >          trace_qcow2_writev_start_part(qemu_coroutine_self());
> >          index_in_cluster = sector_num & (s->cluster_sectors - 1);
> > -        n_end = index_in_cluster + remaining_sectors;
> > -        if (s->crypt_method &&
> > -            n_end > QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors) {
> > -            n_end = QCOW_MAX_CRYPT_CLUSTERS * s->cluster_sectors;
> > -        }
> > +        cur_nr_sectors = remaining_sectors;
> 
> You still need to limit cur_nr_sectors for the encrypted case, otherwise
> you get a buffer overflow of cluster_data later in the function. My
> complaint in v3 was not that you have the limiting, but that applying it
> to n_end doesn't have any effect any more, you need to apply it to
> cur_nr_sectors.

Thanks! I didn't understand you completely:-P.

Re: [Qemu-devel] [PATCH v4 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()

[Hu Tao]

On Thu, Jan 23, 2014 at 06:02:08PM +0100, Benoît Canet wrote:
> Le Thursday 23 Jan 2014 à 11:04:05 (+0800), Hu Tao a écrit :
> > n_start can be actually calculated from offset. The number of
> > sectors to be allocated(n_end - n_start) can be passed in in
> > num. By removing n_start and n_end, we can save two parameters.
> > 
> > The side effect is there is a bug in qcow2.c:preallocate() that
> > passes incorrect n_start to qcow2_alloc_cluster_offset() is
> > fixed. The bug can be triggerred by a larger cluster size than
> > the default value(65536), for example:
> > 
> > ./qemu-img create -f qcow2 \
> >   -o 'cluster_size=131072,preallocation=metadata' file.img 4G
> > 
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  block/qcow2-cluster.c | 14 ++++++--------
> >  block/qcow2.c         | 11 +++--------
> >  block/qcow2.h         |  2 +-
> >  trace-events          |  2 +-
> >  4 files changed, 11 insertions(+), 18 deletions(-)
> > 
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index 8534084..c57f39d 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -1182,7 +1182,7 @@ fail:
> >   * Return 0 on success and -errno in error cases
> >   */
> >  int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> > -    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m)
> > +    int *num, uint64_t *host_offset, QCowL2Meta **m)
> >  {
> >      BDRVQcowState *s = bs->opaque;
> >      uint64_t start, remaining;
> > @@ -1190,15 +1190,13 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> >      uint64_t cur_bytes;
> >      int ret;
> >  
> > -    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset,
> > -                                      n_start, n_end);
> > +    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *num);
> >  
> > -    assert(n_start * BDRV_SECTOR_SIZE == offset_into_cluster(s, offset));
> > -    offset = start_of_cluster(s, offset);
> > +    assert((offset & ~BDRV_SECTOR_MASK) == 0);
> 
> Why replace something that would round gently an unaligned offset
> (start_of_cluster) by an assert that would make QEMU exit ?

It is equivalent to the removed assert().

Re: [Qemu-devel] [PATCH v4 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at()

[Hu Tao]

On Thu, Jan 23, 2014 at 06:13:48PM +0100, Benoît Canet wrote:
> Le Thursday 23 Jan 2014 à 11:04:06 (+0800), Hu Tao a écrit :
> > When cluster size is big enough it can lead offset overflow
> 
> Maybe "it can lead to an offset overflow in"
> > in qcow2_alloc_clusters_at(). This patch fixes it.

Sure.

> > 
> > The allocation each time is stopped at L2 table boundary
> "The allocation is stopped each time at"

Sure.

> 
> > (see handle_alloc()), so the possible maximum bytes could be
> > 
> >   2^(cluster_bits - 3 + cluster_bits)
> 
> So if understand cluster_bits - 3 is used to compute the number of entry by L2
> and the additional cluster_bits is to take into account each clusters referenced
> by the L2 entries.

Exactly. This is clearer than just one calculation. Do you mind I
put the sentence in commit message?

> It makes sense.
> 
> > 
> > so int is safe for cluster_bits<=17, unsafe otherwise.
> > 
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > ---
> >  block/qcow2-refcount.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> > index c974abe..8712d8b 100644
> > --- a/block/qcow2-refcount.c
> > +++ b/block/qcow2-refcount.c
> > @@ -676,7 +676,13 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
> >      BDRVQcowState *s = bs->opaque;
> >      uint64_t cluster_index;
> >      uint64_t old_free_cluster_index;
> > -    int i, refcount, ret;
> > +    uint64_t i;
> > +    int refcount, ret;
> > +
> 
> 
> > +    assert(nb_clusters >= 0);
> > +    if (nb_clusters == 0) {
> > +        return 0;
> > +    }
> ^
> Adding a a line on the commit message about this assertion and chunk of code
> would be helpful.

How about

  Add an assert to guard the comparation between signed and unsigned

?

> 
> Best regards
> 
> Benoît
> 
> >  
> >      /* Check how many clusters there are free */
> >      cluster_index = offset >> s->cluster_bits;
> > -- 
> > 1.8.5.2.229.g4448466
> > 
> > 

Re: [Qemu-devel] [PATCH v4 4/4] qemu-iotests: add test for qcow2 preallocation with different cluster sizes

[Max Reitz]

On 23.01.2014 04:04, Hu Tao wrote:
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>   tests/qemu-iotests/079     | 63 ++++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/079.out | 32 +++++++++++++++++++++++
>   tests/qemu-iotests/group   |  1 +
>   3 files changed, 96 insertions(+)
>   create mode 100755 tests/qemu-iotests/079
>   create mode 100644 tests/qemu-iotests/079.out

Reviewed-by: Max Reitz <mreitz@redhat.com>

Re: [Qemu-devel] [PATCH v4 2/4] qcow2: fix offset overflow in qcow2_alloc_clusters_at()

[Benoît Canet]

Le Friday 24 Jan 2014 à 18:01:20 (+0800), Hu Tao a écrit :
> On Thu, Jan 23, 2014 at 06:13:48PM +0100, Benoît Canet wrote:
> > Le Thursday 23 Jan 2014 à 11:04:06 (+0800), Hu Tao a écrit :
> > > When cluster size is big enough it can lead offset overflow
> > 
> > Maybe "it can lead to an offset overflow in"
> > > in qcow2_alloc_clusters_at(). This patch fixes it.
> 
> Sure.
> 
> > > 
> > > The allocation each time is stopped at L2 table boundary
> > "The allocation is stopped each time at"
> 
> Sure.
> 
> > 
> > > (see handle_alloc()), so the possible maximum bytes could be
> > > 
> > >   2^(cluster_bits - 3 + cluster_bits)
> > 
> > So if understand cluster_bits - 3 is used to compute the number of entry by L2
> > and the additional cluster_bits is to take into account each clusters referenced
> > by the L2 entries.
> 
> Exactly. This is clearer than just one calculation. Do you mind I
> put the sentence in commit message?
yes It would be fine for future reference.

> 
> > It makes sense.
> > 
> > > 
> > > so int is safe for cluster_bits<=17, unsafe otherwise.
> > > 
> > > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > ---
> > >  block/qcow2-refcount.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> > > index c974abe..8712d8b 100644
> > > --- a/block/qcow2-refcount.c
> > > +++ b/block/qcow2-refcount.c
> > > @@ -676,7 +676,13 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
> > >      BDRVQcowState *s = bs->opaque;
> > >      uint64_t cluster_index;
> > >      uint64_t old_free_cluster_index;
> > > -    int i, refcount, ret;
> > > +    uint64_t i;
> > > +    int refcount, ret;
> > > +
> > 
> > 
> > > +    assert(nb_clusters >= 0);
> > > +    if (nb_clusters == 0) {
> > > +        return 0;
> > > +    }
> > ^
> > Adding a a line on the commit message about this assertion and chunk of code
> > would be helpful.
> 
> How about
> 
>   Add an assert to guard the comparation between signed and unsigned
yes
> 
> ?
> 
> > 
> > Best regards
> > 
> > Benoît
> > 
> > >  
> > >      /* Check how many clusters there are free */
> > >      cluster_index = offset >> s->cluster_bits;
> > > -- 
> > > 1.8.5.2.229.g4448466
> > > 
> > > 
> 

Re: [Qemu-devel] [PATCH v4 1/4] qcow2: remove n_start and n_end of qcow2_alloc_cluster_offset()

[Benoît Canet]

Le Friday 24 Jan 2014 à 17:32:40 (+0800), Hu Tao a écrit :
> On Thu, Jan 23, 2014 at 06:02:08PM +0100, Benoît Canet wrote:
> > Le Thursday 23 Jan 2014 à 11:04:05 (+0800), Hu Tao a écrit :
> > > n_start can be actually calculated from offset. The number of
> > > sectors to be allocated(n_end - n_start) can be passed in in
> > > num. By removing n_start and n_end, we can save two parameters.
> > > 
> > > The side effect is there is a bug in qcow2.c:preallocate() that
> > > passes incorrect n_start to qcow2_alloc_cluster_offset() is
> > > fixed. The bug can be triggerred by a larger cluster size than
> > > the default value(65536), for example:
> > > 
> > > ./qemu-img create -f qcow2 \
> > >   -o 'cluster_size=131072,preallocation=metadata' file.img 4G
> > > 
> > > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > > ---
> > >  block/qcow2-cluster.c | 14 ++++++--------
> > >  block/qcow2.c         | 11 +++--------
> > >  block/qcow2.h         |  2 +-
> > >  trace-events          |  2 +-
> > >  4 files changed, 11 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > > index 8534084..c57f39d 100644
> > > --- a/block/qcow2-cluster.c
> > > +++ b/block/qcow2-cluster.c
> > > @@ -1182,7 +1182,7 @@ fail:
> > >   * Return 0 on success and -errno in error cases
> > >   */
> > >  int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> > > -    int n_start, int n_end, int *num, uint64_t *host_offset, QCowL2Meta **m)
> > > +    int *num, uint64_t *host_offset, QCowL2Meta **m)
> > >  {
> > >      BDRVQcowState *s = bs->opaque;
> > >      uint64_t start, remaining;
> > > @@ -1190,15 +1190,13 @@ int qcow2_alloc_cluster_offset(BlockDriverState *bs, uint64_t offset,
> > >      uint64_t cur_bytes;
> > >      int ret;
> > >  
> > > -    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset,
> > > -                                      n_start, n_end);
> > > +    trace_qcow2_alloc_clusters_offset(qemu_coroutine_self(), offset, *num);
> > >  
> > > -    assert(n_start * BDRV_SECTOR_SIZE == offset_into_cluster(s, offset));
> > > -    offset = start_of_cluster(s, offset);
> > > +    assert((offset & ~BDRV_SECTOR_MASK) == 0);
> > 
> > Why replace something that would round gently an unaligned offset
> > (start_of_cluster) by an assert that would make QEMU exit ?
> 
> It is equivalent to the removed assert().
Oh sorry I didn't see the removed assert() when reviewing :(


> 
>