[Qemu-devel] [PATCH v4 00/10] Clean up around bdrv_getlength()

[Qemu-devel] [PATCH v4 00/10] Clean up around bdrv_getlength()

[Markus Armbruster]

Issues addressed in this series:

* BlockDriver method bdrv_getlength() generally returns -errno, but
  some implementations return -1 instead.  Fix them [PATCH 1].

* Frequent conversions between sectors and bytes complicate the code
  needlessly.  Clean up some [PATCH 2-7].

* bdrv_getlength() always returns a multiple of BDRV_SECTOR_SIZE, but
  some places appear to be confused about that, and align the result
  up or down.  Don't [PATCH 8].

* bdrv_get_geometry() hides errors.  Don't use it in places where
  errors should be detected [PATCH 9+10].

Issues not addressed:

* We want to move away from counting in arbitrary units of 512 bytes
  we call "sector", even though it's not really related to either
  guest or host sector size.  My patches mostly move sideways:

  - Sector-based bdrv_get_geometry() gets partly replaced by new
    bdrv_nb_sectors(), still sector-based.

  - Some sector-based places get converted from bdrv_getlength() to
    bdrv_nb_sectors().  At least, this de-duplicates the conversion
    from bytes to sectors.

  - Two places get converted from bdrv_get_geometry() to
    bdrv_getlength().  Two baby steps forward.

* There are quite a few literals left in the code where
  BDRV_SECTOR_SIZE, BDRV_SECTOR_BITS or BDRV_SECTOR_MASK should be
  used instead.

* Error handling is missing in places, but it's not always obvious
  whether errors can actually happen, and if yes, how to handle them.

* Several calls of bdrv_get_geometry() remain in hw/.  I wanted to
  replace them all, but ran out of steam.

v4:
* Trivially rebased
* Set ret on error paths in img_compare() and img_rebase() in PATCH 10
  [Benoît]
v3:
* Trivially rebased
* Correct silly g_new() vs. g_new0() mistake in PATCH 09 [Max]
v2:
* Trivially rebased
* Correct silly bdrv_getlength() vs. bdrv_nb_sectors() mistake in
  PATCH 03
* Split PATCH 03 into 03-07 [Kevin]
* Conversion of bs_sectors to array in PATCH 05 had a subscript off by
  one, fix [Kevin]
* Split PATCH 05 into 09-10 [Kevin]

Markus Armbruster (10):
  raw-posix: Fix raw_getlength() to always return -errno on error
  block: New bdrv_nb_sectors()
  block: Use bdrv_nb_sectors() in bdrv_make_zero()
  block: Use bdrv_nb_sectors() in bdrv_aligned_preadv()
  block: Use bdrv_nb_sectors() in bdrv_co_get_block_status()
  block: Use bdrv_nb_sectors() in img_convert()
  block: Use bdrv_nb_sectors() where sectors, not bytes are wanted
  block: Drop superfluous aligning of bdrv_getlength()'s value
  qemu-img: Make img_convert() get image size just once per image
  block: Avoid bdrv_get_geometry() where errors should be detected

 block-migration.c     |  9 +++--
 block.c               | 81 +++++++++++++++++++++++-------------------
 block/qapi.c          | 14 +++++---
 block/qcow2.c         |  3 +-
 block/raw-posix.c     | 28 +++++++++++----
 block/vmdk.c          |  5 ++-
 include/block/block.h |  1 +
 qemu-img.c            | 98 +++++++++++++++++++++++++++++++++++----------------
 8 files changed, 152 insertions(+), 87 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH v4 01/10] raw-posix: Fix raw_getlength() to always return -errno on error

[Markus Armbruster]

We got a merry mix of -1 and -errno here.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 block/raw-posix.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index b7f0f26..b9db078 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1097,12 +1097,12 @@ static int64_t raw_getlength(BlockDriverState *bs)
     struct stat st;
 
     if (fstat(fd, &st))
-        return -1;
+        return -errno;
     if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
         struct disklabel dl;
 
         if (ioctl(fd, DIOCGDINFO, &dl))
-            return -1;
+            return -errno;
         return (uint64_t)dl.d_secsize *
             dl.d_partitions[DISKPART(st.st_rdev)].p_size;
     } else
@@ -1116,7 +1116,7 @@ static int64_t raw_getlength(BlockDriverState *bs)
     struct stat st;
 
     if (fstat(fd, &st))
-        return -1;
+        return -errno;
     if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
         struct dkwedge_info dkw;
 
@@ -1126,7 +1126,7 @@ static int64_t raw_getlength(BlockDriverState *bs)
             struct disklabel dl;
 
             if (ioctl(fd, DIOCGDINFO, &dl))
-                return -1;
+                return -errno;
             return (uint64_t)dl.d_secsize *
                 dl.d_partitions[DISKPART(st.st_rdev)].p_size;
         }
@@ -1139,6 +1139,7 @@ static int64_t raw_getlength(BlockDriverState *bs)
     BDRVRawState *s = bs->opaque;
     struct dk_minfo minfo;
     int ret;
+    int64_t size;
 
     ret = fd_open(bs);
     if (ret < 0) {
@@ -1157,7 +1158,11 @@ static int64_t raw_getlength(BlockDriverState *bs)
      * There are reports that lseek on some devices fails, but
      * irc discussion said that contingency on contingency was overkill.
      */
-    return lseek(s->fd, 0, SEEK_END);
+    size = lseek(s->fd, 0, SEEK_END);
+    if (size < 0) {
+        return -errno;
+    }
+    return size;
 }
 #elif defined(CONFIG_BSD)
 static int64_t raw_getlength(BlockDriverState *bs)
@@ -1195,6 +1200,9 @@ again:
         size = LLONG_MAX;
 #else
         size = lseek(fd, 0LL, SEEK_END);
+        if (size < 0) {
+            return -errno;
+        }
 #endif
 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
         switch(s->type) {
@@ -1211,6 +1219,9 @@ again:
 #endif
     } else {
         size = lseek(fd, 0, SEEK_END);
+        if (size < 0) {
+            return -errno;
+        }
     }
     return size;
 }
@@ -1219,13 +1230,18 @@ static int64_t raw_getlength(BlockDriverState *bs)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
+    int64_t size;
 
     ret = fd_open(bs);
     if (ret < 0) {
         return ret;
     }
 
-    return lseek(s->fd, 0, SEEK_END);
+    size = lseek(s->fd, 0, SEEK_END);
+    if (size < 0) {
+        return -errno;
+    }
+    return size;
 }
 #endif
 
-- 
1.9.3

[Qemu-devel] [PATCH v4 02/10] block: New bdrv_nb_sectors()

[Markus Armbruster]

A call to retrieve the image size converts between bytes and sectors
several times:

* BlockDriver method bdrv_getlength() returns bytes.

* refresh_total_sectors() converts to sectors, rounding up, and stores
  in total_sectors.

* bdrv_getlength() converts total_sectors back to bytes (now rounded
  up to a multiple of the sector size).

* Callers wanting sectors rather bytes convert it right back.
  Example: bdrv_get_geometry().

bdrv_nb_sectors() provides a way to omit the last two conversions.
It's exactly bdrv_getlength() with the conversion to bytes omitted.
It's functionally like bdrv_get_geometry() without its odd error
handling.

Reimplement bdrv_getlength() and bdrv_get_geometry() on top of
bdrv_nb_sectors().

The next patches will convert some users of bdrv_getlength() to
bdrv_nb_sectors().

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 block.c               | 29 +++++++++++++++++++----------
 include/block/block.h |  1 +
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index 310ea89..0db7d39 100644
--- a/block.c
+++ b/block.c
@@ -693,6 +693,7 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
 
 /**
  * Set the current 'total_sectors' value
+ * Return 0 on success, -errno on error.
  */
 static int refresh_total_sectors(BlockDriverState *bs, int64_t hint)
 {
@@ -3552,11 +3553,12 @@ int64_t bdrv_get_allocated_file_size(BlockDriverState *bs)
 }
 
 /**
- * Length of a file in bytes. Return < 0 if error or unknown.
+ * Return number of sectors on success, -errno on error.
  */
-int64_t bdrv_getlength(BlockDriverState *bs)
+int64_t bdrv_nb_sectors(BlockDriverState *bs)
 {
     BlockDriver *drv = bs->drv;
+
     if (!drv)
         return -ENOMEDIUM;
 
@@ -3566,19 +3568,26 @@ int64_t bdrv_getlength(BlockDriverState *bs)
             return ret;
         }
     }
-    return bs->total_sectors * BDRV_SECTOR_SIZE;
+    return bs->total_sectors;
+}
+
+/**
+ * Return length in bytes on success, -errno on error.
+ * The length is always a multiple of BDRV_SECTOR_SIZE.
+ */
+int64_t bdrv_getlength(BlockDriverState *bs)
+{
+    int64_t ret = bdrv_nb_sectors(bs);
+
+    return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE;
 }
 
 /* return 0 as number of sectors if no device present or error */
 void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr)
 {
-    int64_t length;
-    length = bdrv_getlength(bs);
-    if (length < 0)
-        length = 0;
-    else
-        length = length >> BDRV_SECTOR_BITS;
-    *nb_sectors_ptr = length;
+    int64_t nb_sectors = bdrv_nb_sectors(bs);
+
+    *nb_sectors_ptr = nb_sectors < 0 ? 0 : nb_sectors;
 }
 
 void bdrv_set_on_error(BlockDriverState *bs, BlockdevOnError on_read_error,
diff --git a/include/block/block.h b/include/block/block.h
index faee3aa..4a1e875 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -279,6 +279,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
     const char *backing_file);
 int bdrv_get_backing_file_depth(BlockDriverState *bs);
 int bdrv_truncate(BlockDriverState *bs, int64_t offset);
+int64_t bdrv_nb_sectors(BlockDriverState *bs);
 int64_t bdrv_getlength(BlockDriverState *bs);
 int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
 void bdrv_get_geometry(BlockDriverState *bs, uint64_t *nb_sectors_ptr);
-- 
1.9.3

[Qemu-devel] [PATCH v4 03/10] block: Use bdrv_nb_sectors() in bdrv_make_zero()

[Markus Armbruster]

Instead of bdrv_getlength().

Variable target_size is initially in bytes, then changes meaning to
sectors.  Ugh.  Replace by target_sectors.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 block.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index 0db7d39..54e16ad 100644
--- a/block.c
+++ b/block.c
@@ -2856,18 +2856,16 @@ int bdrv_write_zeroes(BlockDriverState *bs, int64_t sector_num,
  */
 int bdrv_make_zero(BlockDriverState *bs, BdrvRequestFlags flags)
 {
-    int64_t target_size;
-    int64_t ret, nb_sectors, sector_num = 0;
+    int64_t target_sectors, ret, nb_sectors, sector_num = 0;
     int n;
 
-    target_size = bdrv_getlength(bs);
-    if (target_size < 0) {
-        return target_size;
+    target_sectors = bdrv_nb_sectors(bs);
+    if (target_sectors < 0) {
+        return target_sectors;
     }
-    target_size /= BDRV_SECTOR_SIZE;
 
     for (;;) {
-        nb_sectors = target_size - sector_num;
+        nb_sectors = target_sectors - sector_num;
         if (nb_sectors <= 0) {
             return 0;
         }
-- 
1.9.3

[Qemu-devel] [PATCH v4 04/10] block: Use bdrv_nb_sectors() in bdrv_aligned_preadv()

[Markus Armbruster]

Instead of bdrv_getlength().  Eliminate variable len.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 block.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 54e16ad..c73dcba 100644
--- a/block.c
+++ b/block.c
@@ -3082,15 +3082,14 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
         ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
     } else {
         /* Read zeros after EOF of growable BDSes */
-        int64_t len, total_sectors, max_nb_sectors;
+        int64_t total_sectors, max_nb_sectors;
 
-        len = bdrv_getlength(bs);
-        if (len < 0) {
-            ret = len;
+        total_sectors = bdrv_nb_sectors(bs);
+        if (total_sectors < 0) {
+            ret = total_sectors;
             goto out;
         }
 
-        total_sectors = DIV_ROUND_UP(len, BDRV_SECTOR_SIZE);
         max_nb_sectors = ROUND_UP(MAX(0, total_sectors - sector_num),
                                   align >> BDRV_SECTOR_BITS);
         if (max_nb_sectors > 0) {
-- 
1.9.3

[Qemu-devel] [PATCH v4 05/10] block: Use bdrv_nb_sectors() in bdrv_co_get_block_status()

[Markus Armbruster]

Instead of bdrv_getlength().

Replace variables length, length2 by total_sectors, nb_sectors2.
Bonus: use total_sectors instead of the slightly unclean
bs->total_sectors.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 block.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index c73dcba..65e563a 100644
--- a/block.c
+++ b/block.c
@@ -3927,21 +3927,21 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
                                                      int64_t sector_num,
                                                      int nb_sectors, int *pnum)
 {
-    int64_t length;
+    int64_t total_sectors;
     int64_t n;
     int64_t ret, ret2;
 
-    length = bdrv_getlength(bs);
-    if (length < 0) {
-        return length;
+    total_sectors = bdrv_nb_sectors(bs);
+    if (total_sectors < 0) {
+        return total_sectors;
     }
 
-    if (sector_num >= (length >> BDRV_SECTOR_BITS)) {
+    if (sector_num >= total_sectors) {
         *pnum = 0;
         return 0;
     }
 
-    n = bs->total_sectors - sector_num;
+    n = total_sectors - sector_num;
     if (n < nb_sectors) {
         nb_sectors = n;
     }
@@ -3976,8 +3976,8 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
             ret |= BDRV_BLOCK_ZERO;
         } else if (bs->backing_hd) {
             BlockDriverState *bs2 = bs->backing_hd;
-            int64_t length2 = bdrv_getlength(bs2);
-            if (length2 >= 0 && sector_num >= (length2 >> BDRV_SECTOR_BITS)) {
+            int64_t nb_sectors2 = bdrv_nb_sectors(bs2);
+            if (nb_sectors2 >= 0 && sector_num >= nb_sectors2) {
                 ret |= BDRV_BLOCK_ZERO;
             }
         }
-- 
1.9.3

[Qemu-devel] [PATCH v4 06/10] block: Use bdrv_nb_sectors() in img_convert()

[Markus Armbruster]

Instead of bdrv_getlength().  Replace variable output_length by
output_sectors.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 qemu-img.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index b3d2bc6..c8695ca 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1477,13 +1477,13 @@ static int img_convert(int argc, char **argv)
     buf = qemu_blockalign(out_bs, bufsectors * BDRV_SECTOR_SIZE);
 
     if (skip_create) {
-        int64_t output_length = bdrv_getlength(out_bs);
-        if (output_length < 0) {
+        int64_t output_sectors = bdrv_nb_sectors(out_bs);
+        if (output_sectors < 0) {
             error_report("unable to get output image length: %s\n",
-                         strerror(-output_length));
+                         strerror(-output_sectors));
             ret = -1;
             goto out;
-        } else if (output_length < total_sectors << BDRV_SECTOR_BITS) {
+        } else if (output_sectors < total_sectors) {
             error_report("output file is smaller than input file");
             ret = -1;
             goto out;
-- 
1.9.3

[Qemu-devel] [PATCH v4 07/10] block: Use bdrv_nb_sectors() where sectors, not bytes are wanted

[Markus Armbruster]

Instead of bdrv_getlength().

Aside: a few of these callers don't handle errors.  I didn't
investigate whether they should.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 block-migration.c | 9 ++++-----
 block.c           | 3 +--
 block/qcow2.c     | 2 +-
 block/vmdk.c      | 5 ++---
 4 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 1656270..1c7b7be 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -186,7 +186,7 @@ static int bmds_aio_inflight(BlkMigDevState *bmds, int64_t sector)
 {
     int64_t chunk = sector / (int64_t)BDRV_SECTORS_PER_DIRTY_CHUNK;
 
-    if ((sector << BDRV_SECTOR_BITS) < bdrv_getlength(bmds->bs)) {
+    if (sector < bdrv_nb_sectors(bmds->bs)) {
         return !!(bmds->aio_bitmap[chunk / (sizeof(unsigned long) * 8)] &
             (1UL << (chunk % (sizeof(unsigned long) * 8))));
     } else {
@@ -223,8 +223,7 @@ static void alloc_aio_bitmap(BlkMigDevState *bmds)
     BlockDriverState *bs = bmds->bs;
     int64_t bitmap_size;
 
-    bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS) +
-            BDRV_SECTORS_PER_DIRTY_CHUNK * 8 - 1;
+    bitmap_size = bdrv_nb_sectors(bs) + BDRV_SECTORS_PER_DIRTY_CHUNK * 8 - 1;
     bitmap_size /= BDRV_SECTORS_PER_DIRTY_CHUNK * 8;
 
     bmds->aio_bitmap = g_malloc0(bitmap_size);
@@ -350,7 +349,7 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
     int64_t sectors;
 
     if (!bdrv_is_read_only(bs)) {
-        sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
+        sectors = bdrv_nb_sectors(bs);
         if (sectors <= 0) {
             return;
         }
@@ -800,7 +799,7 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
 
             if (bs != bs_prev) {
                 bs_prev = bs;
-                total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
+                total_sectors = bdrv_nb_sectors(bs);
                 if (total_sectors <= 0) {
                     error_report("Error getting length of block device %s",
                                  device_name);
diff --git a/block.c b/block.c
index 65e563a..ee007c2 100644
--- a/block.c
+++ b/block.c
@@ -5259,13 +5259,12 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
 
     granularity >>= BDRV_SECTOR_BITS;
     assert(granularity);
-    bitmap_size = bdrv_getlength(bs);
+    bitmap_size = bdrv_nb_sectors(bs);
     if (bitmap_size < 0) {
         error_setg_errno(errp, -bitmap_size, "could not get length of device");
         errno = -bitmap_size;
         return NULL;
     }
-    bitmap_size >>= BDRV_SECTOR_BITS;
     bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
     bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
     QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
diff --git a/block/qcow2.c b/block/qcow2.c
index a54d2ba..1f15adf 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1538,7 +1538,7 @@ static int preallocate(BlockDriverState *bs)
     int ret;
     QCowL2Meta *meta;
 
-    nb_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
+    nb_sectors = bdrv_nb_sectors(bs);
     offset = 0;
 
     while (nb_sectors) {
diff --git a/block/vmdk.c b/block/vmdk.c
index 2b38f61..b51ad15 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -669,8 +669,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
     if (le32_to_cpu(header.flags) & VMDK4_FLAG_RGD) {
         l1_backup_offset = le64_to_cpu(header.rgd_offset) << 9;
     }
-    if (bdrv_getlength(file) <
-            le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE) {
+    if (bdrv_nb_sectors(file) < le64_to_cpu(header.grain_offset)) {
         error_setg(errp, "File truncated, expecting at least %" PRId64 " bytes",
                    (int64_t)(le64_to_cpu(header.grain_offset)
                              * BDRV_SECTOR_SIZE));
@@ -2004,7 +2003,7 @@ static int vmdk_check(BlockDriverState *bs, BdrvCheckResult *result,
     BDRVVmdkState *s = bs->opaque;
     VmdkExtent *extent = NULL;
     int64_t sector_num = 0;
-    int64_t total_sectors = bdrv_getlength(bs) / BDRV_SECTOR_SIZE;
+    int64_t total_sectors = bdrv_nb_sectors(bs);
     int ret;
     uint64_t cluster_offset;
 
-- 
1.9.3

Re: [Qemu-devel] [PATCH v4 07/10] block: Use bdrv_nb_sectors() where sectors, not bytes are wanted

[Benoît Canet]

The Wednesday 04 Jun 2014 à 13:51:48 (+0200), Markus Armbruster wrote :
> Instead of bdrv_getlength().
> 
> Aside: a few of these callers don't handle errors.  I didn't
> investigate whether they should.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block-migration.c | 9 ++++-----
>  block.c           | 3 +--
>  block/qcow2.c     | 2 +-
>  block/vmdk.c      | 5 ++---
>  4 files changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/block-migration.c b/block-migration.c
> index 1656270..1c7b7be 100644
> --- a/block-migration.c
> +++ b/block-migration.c
> @@ -186,7 +186,7 @@ static int bmds_aio_inflight(BlkMigDevState *bmds, int64_t sector)
>  {
>      int64_t chunk = sector / (int64_t)BDRV_SECTORS_PER_DIRTY_CHUNK;
>  
> -    if ((sector << BDRV_SECTOR_BITS) < bdrv_getlength(bmds->bs)) {
> +    if (sector < bdrv_nb_sectors(bmds->bs)) {
>          return !!(bmds->aio_bitmap[chunk / (sizeof(unsigned long) * 8)] &
>              (1UL << (chunk % (sizeof(unsigned long) * 8))));
>      } else {
> @@ -223,8 +223,7 @@ static void alloc_aio_bitmap(BlkMigDevState *bmds)
>      BlockDriverState *bs = bmds->bs;
>      int64_t bitmap_size;
>  
> -    bitmap_size = (bdrv_getlength(bs) >> BDRV_SECTOR_BITS) +
> -            BDRV_SECTORS_PER_DIRTY_CHUNK * 8 - 1;
> +    bitmap_size = bdrv_nb_sectors(bs) + BDRV_SECTORS_PER_DIRTY_CHUNK * 8 - 1;
>      bitmap_size /= BDRV_SECTORS_PER_DIRTY_CHUNK * 8;
>  
>      bmds->aio_bitmap = g_malloc0(bitmap_size);
> @@ -350,7 +349,7 @@ static void init_blk_migration_it(void *opaque, BlockDriverState *bs)
>      int64_t sectors;
>  
>      if (!bdrv_is_read_only(bs)) {
> -        sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> +        sectors = bdrv_nb_sectors(bs);
>          if (sectors <= 0) {
>              return;
>          }
> @@ -800,7 +799,7 @@ static int block_load(QEMUFile *f, void *opaque, int version_id)
>  
>              if (bs != bs_prev) {
>                  bs_prev = bs;
> -                total_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> +                total_sectors = bdrv_nb_sectors(bs);
>                  if (total_sectors <= 0) {
>                      error_report("Error getting length of block device %s",
>                                   device_name);
> diff --git a/block.c b/block.c
> index 65e563a..ee007c2 100644
> --- a/block.c
> +++ b/block.c
> @@ -5259,13 +5259,12 @@ BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs, int granularity,
>  
>      granularity >>= BDRV_SECTOR_BITS;
>      assert(granularity);
> -    bitmap_size = bdrv_getlength(bs);
> +    bitmap_size = bdrv_nb_sectors(bs);
>      if (bitmap_size < 0) {
>          error_setg_errno(errp, -bitmap_size, "could not get length of device");
>          errno = -bitmap_size;
>          return NULL;
>      }
> -    bitmap_size >>= BDRV_SECTOR_BITS;
>      bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
>      bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
>      QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
> diff --git a/block/qcow2.c b/block/qcow2.c
> index a54d2ba..1f15adf 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1538,7 +1538,7 @@ static int preallocate(BlockDriverState *bs)
>      int ret;
>      QCowL2Meta *meta;
>  
> -    nb_sectors = bdrv_getlength(bs) >> BDRV_SECTOR_BITS;
> +    nb_sectors = bdrv_nb_sectors(bs);
>      offset = 0;
>  
>      while (nb_sectors) {
> diff --git a/block/vmdk.c b/block/vmdk.c
> index 2b38f61..b51ad15 100644
> --- a/block/vmdk.c
> +++ b/block/vmdk.c
> @@ -669,8 +669,7 @@ static int vmdk_open_vmdk4(BlockDriverState *bs,
>      if (le32_to_cpu(header.flags) & VMDK4_FLAG_RGD) {
>          l1_backup_offset = le64_to_cpu(header.rgd_offset) << 9;
>      }
> -    if (bdrv_getlength(file) <
> -            le64_to_cpu(header.grain_offset) * BDRV_SECTOR_SIZE) {
> +    if (bdrv_nb_sectors(file) < le64_to_cpu(header.grain_offset)) {
>          error_setg(errp, "File truncated, expecting at least %" PRId64 " bytes",
>                     (int64_t)(le64_to_cpu(header.grain_offset)
>                               * BDRV_SECTOR_SIZE));
> @@ -2004,7 +2003,7 @@ static int vmdk_check(BlockDriverState *bs, BdrvCheckResult *result,
>      BDRVVmdkState *s = bs->opaque;
>      VmdkExtent *extent = NULL;
>      int64_t sector_num = 0;
> -    int64_t total_sectors = bdrv_getlength(bs) / BDRV_SECTOR_SIZE;
> +    int64_t total_sectors = bdrv_nb_sectors(bs);
>      int ret;
>      uint64_t cluster_offset;
>  
> -- 
> 1.9.3
> 
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>

[Qemu-devel] [PATCH v4 08/10] block: Drop superfluous aligning of bdrv_getlength()'s value

[Markus Armbruster]

It returns a multiple of the sector size.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 block.c       | 1 -
 block/qcow2.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/block.c b/block.c
index ee007c2..cae9085 100644
--- a/block.c
+++ b/block.c
@@ -1261,7 +1261,6 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp)
         error_setg_errno(errp, -total_size, "Could not get image size");
         goto out;
     }
-    total_size &= BDRV_SECTOR_MASK;
 
     /* Create the temporary image */
     ret = get_tmp_filename(tmp_filename, PATH_MAX + 1);
diff --git a/block/qcow2.c b/block/qcow2.c
index 1f15adf..1cfcb63 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1926,7 +1926,6 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
         /* align end of file to a sector boundary to ease reading with
            sector based I/Os */
         cluster_offset = bdrv_getlength(bs->file);
-        cluster_offset = (cluster_offset + 511) & ~511;
         bdrv_truncate(bs->file, cluster_offset);
         return 0;
     }
-- 
1.9.3

[Qemu-devel] [PATCH v4 09/10] qemu-img: Make img_convert() get image size just once per image

[Markus Armbruster]

Chiefly so I don't have to do the error checking in quadruplicate in
the next commit.  Moreover, replacing the frequently updated
bs_sectors by an array assigned just once makes the code easier to
understand.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Benoit Canet <benoit@irqsave.net>
---
 qemu-img.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index c8695ca..e6d0edf 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1185,7 +1185,7 @@ static int img_convert(int argc, char **argv)
     BlockDriver *drv, *proto_drv;
     BlockDriverState **bs = NULL, *out_bs = NULL;
     int64_t total_sectors, nb_sectors, sector_num, bs_offset;
-    uint64_t bs_sectors;
+    uint64_t *bs_sectors = NULL;
     uint8_t * buf = NULL;
     size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
     const uint8_t *buf1;
@@ -1325,7 +1325,8 @@ static int img_convert(int argc, char **argv)
 
     qemu_progress_print(0, 100);
 
-    bs = g_malloc0(bs_n * sizeof(BlockDriverState *));
+    bs = g_new0(BlockDriverState *, bs_n);
+    bs_sectors = g_new(uint64_t, bs_n);
 
     total_sectors = 0;
     for (bs_i = 0; bs_i < bs_n; bs_i++) {
@@ -1339,8 +1340,8 @@ static int img_convert(int argc, char **argv)
             ret = -1;
             goto out;
         }
-        bdrv_get_geometry(bs[bs_i], &bs_sectors);
-        total_sectors += bs_sectors;
+        bdrv_get_geometry(bs[bs_i], &bs_sectors[bs_i]);
+        total_sectors += bs_sectors[bs_i];
     }
 
     if (sn_opts) {
@@ -1464,7 +1465,6 @@ static int img_convert(int argc, char **argv)
 
     bs_i = 0;
     bs_offset = 0;
-    bdrv_get_geometry(bs[0], &bs_sectors);
 
     /* increase bufsectors from the default 4096 (2M) if opt_transfer_length
      * or discard_alignment of the out_bs is greater. Limit to 32768 (16MB)
@@ -1531,19 +1531,19 @@ static int img_convert(int argc, char **argv)
             buf2 = buf;
             while (remainder > 0) {
                 int nlow;
-                while (bs_num == bs_sectors) {
+                while (bs_num == bs_sectors[bs_i]) {
+                    bs_offset += bs_sectors[bs_i];
                     bs_i++;
                     assert (bs_i < bs_n);
-                    bs_offset += bs_sectors;
-                    bdrv_get_geometry(bs[bs_i], &bs_sectors);
                     bs_num = 0;
                     /* printf("changing part: sector_num=%" PRId64 ", "
                        "bs_i=%d, bs_offset=%" PRId64 ", bs_sectors=%" PRId64
-                       "\n", sector_num, bs_i, bs_offset, bs_sectors); */
+                       "\n", sector_num, bs_i, bs_offset, bs_sectors[bs_i]); */
                 }
-                assert (bs_num < bs_sectors);
+                assert (bs_num < bs_sectors[bs_i]);
 
-                nlow = (remainder > bs_sectors - bs_num) ? bs_sectors - bs_num : remainder;
+                nlow = remainder > bs_sectors[bs_i] - bs_num
+                    ? bs_sectors[bs_i] - bs_num : remainder;
 
                 ret = bdrv_read(bs[bs_i], bs_num, buf2, nlow);
                 if (ret < 0) {
@@ -1604,14 +1604,13 @@ restart:
                 break;
             }
 
-            while (sector_num - bs_offset >= bs_sectors) {
+            while (sector_num - bs_offset >= bs_sectors[bs_i]) {
+                bs_offset += bs_sectors[bs_i];
                 bs_i ++;
                 assert (bs_i < bs_n);
-                bs_offset += bs_sectors;
-                bdrv_get_geometry(bs[bs_i], &bs_sectors);
                 /* printf("changing part: sector_num=%" PRId64 ", bs_i=%d, "
                   "bs_offset=%" PRId64 ", bs_sectors=%" PRId64 "\n",
-                   sector_num, bs_i, bs_offset, bs_sectors); */
+                   sector_num, bs_i, bs_offset, bs_sectors[bs_i]); */
             }
 
             if ((out_baseimg || has_zero_init) &&
@@ -1664,7 +1663,7 @@ restart:
                 }
             }
 
-            n = MIN(n, bs_sectors - (sector_num - bs_offset));
+            n = MIN(n, bs_sectors[bs_i] - (sector_num - bs_offset));
 
             sectors_read += n;
             if (count_allocated_sectors) {
@@ -1722,6 +1721,7 @@ out:
         }
         g_free(bs);
     }
+    g_free(bs_sectors);
 fail_getopt:
     g_free(options);
 
-- 
1.9.3

[Qemu-devel] [PATCH v4 10/10] block: Avoid bdrv_get_geometry() where errors should be detected

[Markus Armbruster]

bdrv_get_geometry() hides errors.  Use bdrv_nb_sectors() or
bdrv_getlength() instead where that's obviously inappropriate.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block.c      | 11 ++++++++---
 block/qapi.c | 14 +++++++++----
 qemu-img.c   | 64 +++++++++++++++++++++++++++++++++++++++++++++++-------------
 3 files changed, 68 insertions(+), 21 deletions(-)

diff --git a/block.c b/block.c
index cae9085..b92f05f 100644
--- a/block.c
+++ b/block.c
@@ -5574,7 +5574,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
     if (size && size->value.n == -1) {
         if (backing_file && backing_file->value.s) {
             BlockDriverState *bs;
-            uint64_t size;
+            int64_t size;
             char buf[32];
             int back_flags;
 
@@ -5593,8 +5593,13 @@ void bdrv_img_create(const char *filename, const char *fmt,
                 local_err = NULL;
                 goto out;
             }
-            bdrv_get_geometry(bs, &size);
-            size *= 512;
+            size = bdrv_getlength(bs);
+            if (size < 0) {
+                error_setg_errno(errp, -size, "Could not get size of '%s'",
+                                 backing_file->value.s);
+                bdrv_unref(bs);
+                goto out;
+            }
 
             snprintf(buf, sizeof(buf), "%" PRId64, size);
             set_option_parameter(param, BLOCK_OPT_SIZE, buf);
diff --git a/block/qapi.c b/block/qapi.c
index 97e1641..90f406d 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -165,19 +165,25 @@ void bdrv_query_image_info(BlockDriverState *bs,
                            ImageInfo **p_info,
                            Error **errp)
 {
-    uint64_t total_sectors;
+    int64_t size;
     const char *backing_filename;
     char backing_filename2[1024];
     BlockDriverInfo bdi;
     int ret;
     Error *err = NULL;
-    ImageInfo *info = g_new0(ImageInfo, 1);
+    ImageInfo *info;
 
-    bdrv_get_geometry(bs, &total_sectors);
+    size = bdrv_getlength(bs);
+    if (size < 0) {
+        error_setg_errno(errp, -size, "Can't get size of device '%s'",
+                         bdrv_get_device_name(bs));
+        return;
+    }
 
+    info = g_new0(ImageInfo, 1);
     info->filename        = g_strdup(bs->filename);
     info->format          = g_strdup(bdrv_get_format_name(bs));
-    info->virtual_size    = total_sectors * 512;
+    info->virtual_size    = size;
     info->actual_size     = bdrv_get_allocated_file_size(bs);
     info->has_actual_size = info->actual_size >= 0;
     if (bdrv_is_encrypted(bs)) {
diff --git a/qemu-img.c b/qemu-img.c
index e6d0edf..7e6dde0 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -958,7 +958,6 @@ static int img_compare(int argc, char **argv)
     int64_t sector_num = 0;
     int64_t nb_sectors;
     int c, pnum;
-    uint64_t bs_sectors;
     uint64_t progress_base;
 
     for (;;) {
@@ -1020,10 +1019,20 @@ static int img_compare(int argc, char **argv)
 
     buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
     buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
-    bdrv_get_geometry(bs1, &bs_sectors);
-    total_sectors1 = bs_sectors;
-    bdrv_get_geometry(bs2, &bs_sectors);
-    total_sectors2 = bs_sectors;
+    total_sectors1 = bdrv_nb_sectors(bs1);
+    if (total_sectors1 < 0) {
+        error_report("Can't get size of %s: %s",
+                     filename1, strerror(-total_sectors1));
+        ret = 4;
+        goto out;
+    }
+    total_sectors2 = bdrv_nb_sectors(bs2);
+    if (total_sectors2 < 0) {
+        error_report("Can't get size of %s: %s",
+                     filename2, strerror(-total_sectors2));
+        ret = 4;
+        goto out;
+    }
     total_sectors = MIN(total_sectors1, total_sectors2);
     progress_base = MAX(total_sectors1, total_sectors2);
 
@@ -1185,7 +1194,7 @@ static int img_convert(int argc, char **argv)
     BlockDriver *drv, *proto_drv;
     BlockDriverState **bs = NULL, *out_bs = NULL;
     int64_t total_sectors, nb_sectors, sector_num, bs_offset;
-    uint64_t *bs_sectors = NULL;
+    int64_t *bs_sectors = NULL;
     uint8_t * buf = NULL;
     size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
     const uint8_t *buf1;
@@ -1326,7 +1335,7 @@ static int img_convert(int argc, char **argv)
     qemu_progress_print(0, 100);
 
     bs = g_new0(BlockDriverState *, bs_n);
-    bs_sectors = g_new(uint64_t, bs_n);
+    bs_sectors = g_new(int64_t, bs_n);
 
     total_sectors = 0;
     for (bs_i = 0; bs_i < bs_n; bs_i++) {
@@ -1340,7 +1349,13 @@ static int img_convert(int argc, char **argv)
             ret = -1;
             goto out;
         }
-        bdrv_get_geometry(bs[bs_i], &bs_sectors[bs_i]);
+        bs_sectors[bs_i] = bdrv_nb_sectors(bs[bs_i]);
+        if (bs_sectors[bs_i] < 0) {
+            error_report("Could not get size of %s: %s",
+                         argv[optind + bs_i], strerror(-bs_sectors[bs_i]));
+            ret = -1;
+            goto out;
+        }
         total_sectors += bs_sectors[bs_i];
     }
 
@@ -2421,9 +2436,9 @@ static int img_rebase(int argc, char **argv)
      * the image is the same as the original one at any time.
      */
     if (!unsafe) {
-        uint64_t num_sectors;
-        uint64_t old_backing_num_sectors;
-        uint64_t new_backing_num_sectors = 0;
+        int64_t num_sectors;
+        int64_t old_backing_num_sectors;
+        int64_t new_backing_num_sectors = 0;
         uint64_t sector;
         int n;
         uint8_t * buf_old;
@@ -2433,10 +2448,31 @@ static int img_rebase(int argc, char **argv)
         buf_old = qemu_blockalign(bs, IO_BUF_SIZE);
         buf_new = qemu_blockalign(bs, IO_BUF_SIZE);
 
-        bdrv_get_geometry(bs, &num_sectors);
-        bdrv_get_geometry(bs_old_backing, &old_backing_num_sectors);
+        num_sectors = bdrv_nb_sectors(bs);
+        if (num_sectors < 0) {
+            error_report("Could not get size of '%s': %s",
+                         filename, strerror(-num_sectors));
+            ret = -1;
+            goto out;
+        }
+        old_backing_num_sectors = bdrv_nb_sectors(bs_old_backing);
+        if (old_backing_num_sectors < 0) {
+            char backing_name[1024];
+
+            bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
+            error_report("Could not get size of '%s': %s",
+                         backing_name, strerror(-old_backing_num_sectors));
+            ret = -1;
+            goto out;
+        }
         if (bs_new_backing) {
-            bdrv_get_geometry(bs_new_backing, &new_backing_num_sectors);
+            new_backing_num_sectors = bdrv_nb_sectors(bs_new_backing);
+            if (new_backing_num_sectors < 0) {
+                error_report("Could not get size of '%s': %s",
+                             out_baseimg, strerror(-new_backing_num_sectors));
+                ret = -1;
+                goto out;
+            }
         }
 
         if (num_sectors != 0) {
-- 
1.9.3

Re: [Qemu-devel] [PATCH v4 10/10] block: Avoid bdrv_get_geometry() where errors should be detected

[Benoît Canet]

The Wednesday 04 Jun 2014 à 13:51:51 (+0200), Markus Armbruster wrote :
> bdrv_get_geometry() hides errors.  Use bdrv_nb_sectors() or
> bdrv_getlength() instead where that's obviously inappropriate.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c      | 11 ++++++++---
>  block/qapi.c | 14 +++++++++----
>  qemu-img.c   | 64 +++++++++++++++++++++++++++++++++++++++++++++++-------------
>  3 files changed, 68 insertions(+), 21 deletions(-)
> 
> diff --git a/block.c b/block.c
> index cae9085..b92f05f 100644
> --- a/block.c
> +++ b/block.c
> @@ -5574,7 +5574,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>      if (size && size->value.n == -1) {
>          if (backing_file && backing_file->value.s) {
>              BlockDriverState *bs;
> -            uint64_t size;
> +            int64_t size;
>              char buf[32];
>              int back_flags;
>  
> @@ -5593,8 +5593,13 @@ void bdrv_img_create(const char *filename, const char *fmt,
>                  local_err = NULL;
>                  goto out;
>              }
> -            bdrv_get_geometry(bs, &size);
> -            size *= 512;
> +            size = bdrv_getlength(bs);
> +            if (size < 0) {
> +                error_setg_errno(errp, -size, "Could not get size of '%s'",
> +                                 backing_file->value.s);
> +                bdrv_unref(bs);
> +                goto out;
> +            }
>  
>              snprintf(buf, sizeof(buf), "%" PRId64, size);
>              set_option_parameter(param, BLOCK_OPT_SIZE, buf);
> diff --git a/block/qapi.c b/block/qapi.c
> index 97e1641..90f406d 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -165,19 +165,25 @@ void bdrv_query_image_info(BlockDriverState *bs,
>                             ImageInfo **p_info,
>                             Error **errp)
>  {
> -    uint64_t total_sectors;
> +    int64_t size;
>      const char *backing_filename;
>      char backing_filename2[1024];
>      BlockDriverInfo bdi;
>      int ret;
>      Error *err = NULL;
> -    ImageInfo *info = g_new0(ImageInfo, 1);
> +    ImageInfo *info;
>  
> -    bdrv_get_geometry(bs, &total_sectors);
> +    size = bdrv_getlength(bs);
> +    if (size < 0) {
> +        error_setg_errno(errp, -size, "Can't get size of device '%s'",
> +                         bdrv_get_device_name(bs));
> +        return;
> +    }
>  
> +    info = g_new0(ImageInfo, 1);
>      info->filename        = g_strdup(bs->filename);
>      info->format          = g_strdup(bdrv_get_format_name(bs));
> -    info->virtual_size    = total_sectors * 512;
> +    info->virtual_size    = size;
>      info->actual_size     = bdrv_get_allocated_file_size(bs);
>      info->has_actual_size = info->actual_size >= 0;
>      if (bdrv_is_encrypted(bs)) {
> diff --git a/qemu-img.c b/qemu-img.c
> index e6d0edf..7e6dde0 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -958,7 +958,6 @@ static int img_compare(int argc, char **argv)
>      int64_t sector_num = 0;
>      int64_t nb_sectors;
>      int c, pnum;
> -    uint64_t bs_sectors;
>      uint64_t progress_base;
>  
>      for (;;) {
> @@ -1020,10 +1019,20 @@ static int img_compare(int argc, char **argv)
>  
>      buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
>      buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
> -    bdrv_get_geometry(bs1, &bs_sectors);
> -    total_sectors1 = bs_sectors;
> -    bdrv_get_geometry(bs2, &bs_sectors);
> -    total_sectors2 = bs_sectors;
> +    total_sectors1 = bdrv_nb_sectors(bs1);
> +    if (total_sectors1 < 0) {
> +        error_report("Can't get size of %s: %s",
> +                     filename1, strerror(-total_sectors1));
> +        ret = 4;
> +        goto out;
> +    }
> +    total_sectors2 = bdrv_nb_sectors(bs2);
> +    if (total_sectors2 < 0) {
> +        error_report("Can't get size of %s: %s",
> +                     filename2, strerror(-total_sectors2));
> +        ret = 4;
> +        goto out;
> +    }
>      total_sectors = MIN(total_sectors1, total_sectors2);
>      progress_base = MAX(total_sectors1, total_sectors2);
>  
> @@ -1185,7 +1194,7 @@ static int img_convert(int argc, char **argv)
>      BlockDriver *drv, *proto_drv;
>      BlockDriverState **bs = NULL, *out_bs = NULL;
>      int64_t total_sectors, nb_sectors, sector_num, bs_offset;
> -    uint64_t *bs_sectors = NULL;
> +    int64_t *bs_sectors = NULL;
>      uint8_t * buf = NULL;
>      size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
>      const uint8_t *buf1;
> @@ -1326,7 +1335,7 @@ static int img_convert(int argc, char **argv)
>      qemu_progress_print(0, 100);
>  
>      bs = g_new0(BlockDriverState *, bs_n);
> -    bs_sectors = g_new(uint64_t, bs_n);
> +    bs_sectors = g_new(int64_t, bs_n);
>  
>      total_sectors = 0;
>      for (bs_i = 0; bs_i < bs_n; bs_i++) {
> @@ -1340,7 +1349,13 @@ static int img_convert(int argc, char **argv)
>              ret = -1;
>              goto out;
>          }
> -        bdrv_get_geometry(bs[bs_i], &bs_sectors[bs_i]);
> +        bs_sectors[bs_i] = bdrv_nb_sectors(bs[bs_i]);
> +        if (bs_sectors[bs_i] < 0) {
> +            error_report("Could not get size of %s: %s",
> +                         argv[optind + bs_i], strerror(-bs_sectors[bs_i]));
> +            ret = -1;
> +            goto out;
> +        }
>          total_sectors += bs_sectors[bs_i];
>      }
>  
> @@ -2421,9 +2436,9 @@ static int img_rebase(int argc, char **argv)
>       * the image is the same as the original one at any time.
>       */
>      if (!unsafe) {
> -        uint64_t num_sectors;
> -        uint64_t old_backing_num_sectors;
> -        uint64_t new_backing_num_sectors = 0;
> +        int64_t num_sectors;
> +        int64_t old_backing_num_sectors;
> +        int64_t new_backing_num_sectors = 0;
>          uint64_t sector;
>          int n;
>          uint8_t * buf_old;
> @@ -2433,10 +2448,31 @@ static int img_rebase(int argc, char **argv)
>          buf_old = qemu_blockalign(bs, IO_BUF_SIZE);
>          buf_new = qemu_blockalign(bs, IO_BUF_SIZE);
>  
> -        bdrv_get_geometry(bs, &num_sectors);
> -        bdrv_get_geometry(bs_old_backing, &old_backing_num_sectors);
> +        num_sectors = bdrv_nb_sectors(bs);
> +        if (num_sectors < 0) {
> +            error_report("Could not get size of '%s': %s",
> +                         filename, strerror(-num_sectors));
> +            ret = -1;
> +            goto out;
> +        }
> +        old_backing_num_sectors = bdrv_nb_sectors(bs_old_backing);
> +        if (old_backing_num_sectors < 0) {
> +            char backing_name[1024];

Could you put this on the heap ?

I recently fixed a stack overflow when taking snapshots due to multiple PATH_MAX
char array in a recursive function.

We don't know how this function will be used later.

Best regards

Benoît

> +
> +            bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
> +            error_report("Could not get size of '%s': %s",
> +                         backing_name, strerror(-old_backing_num_sectors));
> +            ret = -1;
> +            goto out;
> +        }
>          if (bs_new_backing) {
> -            bdrv_get_geometry(bs_new_backing, &new_backing_num_sectors);
> +            new_backing_num_sectors = bdrv_nb_sectors(bs_new_backing);
> +            if (new_backing_num_sectors < 0) {
> +                error_report("Could not get size of '%s': %s",
> +                             out_baseimg, strerror(-new_backing_num_sectors));
> +                ret = -1;
> +                goto out;
> +            }
>          }
>  
>          if (num_sectors != 0) {
> -- 
> 1.9.3
> 

Re: [Qemu-devel] [PATCH v4 10/10] block: Avoid bdrv_get_geometry() where errors should be detected

[Markus Armbruster]

Benoît Canet <benoit.canet@irqsave.net> writes:

> The Wednesday 04 Jun 2014 à 13:51:51 (+0200), Markus Armbruster wrote :
>> bdrv_get_geometry() hides errors.  Use bdrv_nb_sectors() or
>> bdrv_getlength() instead where that's obviously inappropriate.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> Reviewed-by: Max Reitz <mreitz@redhat.com>
[...]
>> diff --git a/qemu-img.c b/qemu-img.c
>> index e6d0edf..7e6dde0 100644
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -958,7 +958,6 @@ static int img_compare(int argc, char **argv)
>>      int64_t sector_num = 0;
>>      int64_t nb_sectors;
>>      int c, pnum;
>> -    uint64_t bs_sectors;
>>      uint64_t progress_base;
>>  
>>      for (;;) {
>> @@ -1020,10 +1019,20 @@ static int img_compare(int argc, char **argv)
>>  
>>      buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
>>      buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
>> -    bdrv_get_geometry(bs1, &bs_sectors);
>> -    total_sectors1 = bs_sectors;
>> -    bdrv_get_geometry(bs2, &bs_sectors);
>> -    total_sectors2 = bs_sectors;
>> +    total_sectors1 = bdrv_nb_sectors(bs1);
>> +    if (total_sectors1 < 0) {
>> +        error_report("Can't get size of %s: %s",
>> +                     filename1, strerror(-total_sectors1));
>> +        ret = 4;
>> +        goto out;
>> +    }
>> +    total_sectors2 = bdrv_nb_sectors(bs2);
>> +    if (total_sectors2 < 0) {
>> +        error_report("Can't get size of %s: %s",
>> +                     filename2, strerror(-total_sectors2));
>> +        ret = 4;
>> +        goto out;
>> +    }
>>      total_sectors = MIN(total_sectors1, total_sectors2);
>>      progress_base = MAX(total_sectors1, total_sectors2);
>>  
>> @@ -1185,7 +1194,7 @@ static int img_convert(int argc, char **argv)
>>      BlockDriver *drv, *proto_drv;
>>      BlockDriverState **bs = NULL, *out_bs = NULL;
>>      int64_t total_sectors, nb_sectors, sector_num, bs_offset;
>> -    uint64_t *bs_sectors = NULL;
>> +    int64_t *bs_sectors = NULL;
>>      uint8_t * buf = NULL;
>>      size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
>>      const uint8_t *buf1;
>> @@ -1326,7 +1335,7 @@ static int img_convert(int argc, char **argv)
>>      qemu_progress_print(0, 100);
>>  
>>      bs = g_new0(BlockDriverState *, bs_n);
>> -    bs_sectors = g_new(uint64_t, bs_n);
>> +    bs_sectors = g_new(int64_t, bs_n);
>>  
>>      total_sectors = 0;
>>      for (bs_i = 0; bs_i < bs_n; bs_i++) {
>> @@ -1340,7 +1349,13 @@ static int img_convert(int argc, char **argv)
>>              ret = -1;
>>              goto out;
>>          }
>> -        bdrv_get_geometry(bs[bs_i], &bs_sectors[bs_i]);
>> +        bs_sectors[bs_i] = bdrv_nb_sectors(bs[bs_i]);
>> +        if (bs_sectors[bs_i] < 0) {
>> +            error_report("Could not get size of %s: %s",
>> +                         argv[optind + bs_i], strerror(-bs_sectors[bs_i]));
>> +            ret = -1;
>> +            goto out;
>> +        }
>>          total_sectors += bs_sectors[bs_i];
>>      }
>>  
>> @@ -2421,9 +2436,9 @@ static int img_rebase(int argc, char **argv)
>>       * the image is the same as the original one at any time.
>>       */
>>      if (!unsafe) {
>> -        uint64_t num_sectors;
>> -        uint64_t old_backing_num_sectors;
>> -        uint64_t new_backing_num_sectors = 0;
>> +        int64_t num_sectors;
>> +        int64_t old_backing_num_sectors;
>> +        int64_t new_backing_num_sectors = 0;
>>          uint64_t sector;
>>          int n;
>>          uint8_t * buf_old;
>> @@ -2433,10 +2448,31 @@ static int img_rebase(int argc, char **argv)
>>          buf_old = qemu_blockalign(bs, IO_BUF_SIZE);
>>          buf_new = qemu_blockalign(bs, IO_BUF_SIZE);
>>  
>> -        bdrv_get_geometry(bs, &num_sectors);
>> -        bdrv_get_geometry(bs_old_backing, &old_backing_num_sectors);
>> +        num_sectors = bdrv_nb_sectors(bs);
>> +        if (num_sectors < 0) {
>> +            error_report("Could not get size of '%s': %s",
>> +                         filename, strerror(-num_sectors));
>> +            ret = -1;
>> +            goto out;
>> +        }
>> +        old_backing_num_sectors = bdrv_nb_sectors(bs_old_backing);
>> +        if (old_backing_num_sectors < 0) {
>> +            char backing_name[1024];
>
> Could you put this on the heap ?
>
> I recently fixed a stack overflow when taking snapshots due to multiple PATH_MAX
> char array in a recursive function.
>
> We don't know how this function will be used later.

img_rebase() is not a general purpose function, it's a qemu-img command.
Stack use is well below a single page even with my patch.  I can't see
how it could possibly become recursive.

Re: [Qemu-devel] [PATCH v4 10/10] block: Avoid bdrv_get_geometry() where errors should be detected

[Benoît Canet]

The Wednesday 04 Jun 2014 à 15:20:18 (+0200), Markus Armbruster wrote :
> Benoît Canet <benoit.canet@irqsave.net> writes:
> 
> > The Wednesday 04 Jun 2014 à 13:51:51 (+0200), Markus Armbruster wrote :
> >> bdrv_get_geometry() hides errors.  Use bdrv_nb_sectors() or
> >> bdrv_getlength() instead where that's obviously inappropriate.
> >> 
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> >> Reviewed-by: Max Reitz <mreitz@redhat.com>
> [...]
> >> diff --git a/qemu-img.c b/qemu-img.c
> >> index e6d0edf..7e6dde0 100644
> >> --- a/qemu-img.c
> >> +++ b/qemu-img.c
> >> @@ -958,7 +958,6 @@ static int img_compare(int argc, char **argv)
> >>      int64_t sector_num = 0;
> >>      int64_t nb_sectors;
> >>      int c, pnum;
> >> -    uint64_t bs_sectors;
> >>      uint64_t progress_base;
> >>  
> >>      for (;;) {
> >> @@ -1020,10 +1019,20 @@ static int img_compare(int argc, char **argv)
> >>  
> >>      buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
> >>      buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
> >> -    bdrv_get_geometry(bs1, &bs_sectors);
> >> -    total_sectors1 = bs_sectors;
> >> -    bdrv_get_geometry(bs2, &bs_sectors);
> >> -    total_sectors2 = bs_sectors;
> >> +    total_sectors1 = bdrv_nb_sectors(bs1);
> >> +    if (total_sectors1 < 0) {
> >> +        error_report("Can't get size of %s: %s",
> >> +                     filename1, strerror(-total_sectors1));
> >> +        ret = 4;
> >> +        goto out;
> >> +    }
> >> +    total_sectors2 = bdrv_nb_sectors(bs2);
> >> +    if (total_sectors2 < 0) {
> >> +        error_report("Can't get size of %s: %s",
> >> +                     filename2, strerror(-total_sectors2));
> >> +        ret = 4;
> >> +        goto out;
> >> +    }
> >>      total_sectors = MIN(total_sectors1, total_sectors2);
> >>      progress_base = MAX(total_sectors1, total_sectors2);
> >>  
> >> @@ -1185,7 +1194,7 @@ static int img_convert(int argc, char **argv)
> >>      BlockDriver *drv, *proto_drv;
> >>      BlockDriverState **bs = NULL, *out_bs = NULL;
> >>      int64_t total_sectors, nb_sectors, sector_num, bs_offset;
> >> -    uint64_t *bs_sectors = NULL;
> >> +    int64_t *bs_sectors = NULL;
> >>      uint8_t * buf = NULL;
> >>      size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
> >>      const uint8_t *buf1;
> >> @@ -1326,7 +1335,7 @@ static int img_convert(int argc, char **argv)
> >>      qemu_progress_print(0, 100);
> >>  
> >>      bs = g_new0(BlockDriverState *, bs_n);
> >> -    bs_sectors = g_new(uint64_t, bs_n);
> >> +    bs_sectors = g_new(int64_t, bs_n);
> >>  
> >>      total_sectors = 0;
> >>      for (bs_i = 0; bs_i < bs_n; bs_i++) {
> >> @@ -1340,7 +1349,13 @@ static int img_convert(int argc, char **argv)
> >>              ret = -1;
> >>              goto out;
> >>          }
> >> -        bdrv_get_geometry(bs[bs_i], &bs_sectors[bs_i]);
> >> +        bs_sectors[bs_i] = bdrv_nb_sectors(bs[bs_i]);
> >> +        if (bs_sectors[bs_i] < 0) {
> >> +            error_report("Could not get size of %s: %s",
> >> +                         argv[optind + bs_i], strerror(-bs_sectors[bs_i]));
> >> +            ret = -1;
> >> +            goto out;
> >> +        }
> >>          total_sectors += bs_sectors[bs_i];
> >>      }
> >>  
> >> @@ -2421,9 +2436,9 @@ static int img_rebase(int argc, char **argv)
> >>       * the image is the same as the original one at any time.
> >>       */
> >>      if (!unsafe) {
> >> -        uint64_t num_sectors;
> >> -        uint64_t old_backing_num_sectors;
> >> -        uint64_t new_backing_num_sectors = 0;
> >> +        int64_t num_sectors;
> >> +        int64_t old_backing_num_sectors;
> >> +        int64_t new_backing_num_sectors = 0;
> >>          uint64_t sector;
> >>          int n;
> >>          uint8_t * buf_old;
> >> @@ -2433,10 +2448,31 @@ static int img_rebase(int argc, char **argv)
> >>          buf_old = qemu_blockalign(bs, IO_BUF_SIZE);
> >>          buf_new = qemu_blockalign(bs, IO_BUF_SIZE);
> >>  
> >> -        bdrv_get_geometry(bs, &num_sectors);
> >> -        bdrv_get_geometry(bs_old_backing, &old_backing_num_sectors);
> >> +        num_sectors = bdrv_nb_sectors(bs);
> >> +        if (num_sectors < 0) {
> >> +            error_report("Could not get size of '%s': %s",
> >> +                         filename, strerror(-num_sectors));
> >> +            ret = -1;
> >> +            goto out;
> >> +        }
> >> +        old_backing_num_sectors = bdrv_nb_sectors(bs_old_backing);
> >> +        if (old_backing_num_sectors < 0) {
> >> +            char backing_name[1024];
> >
> > Could you put this on the heap ?
> >
> > I recently fixed a stack overflow when taking snapshots due to multiple PATH_MAX
> > char array in a recursive function.
> >
> > We don't know how this function will be used later.
> 
> img_rebase() is not a general purpose function, it's a qemu-img command.
> Stack use is well below a single page even with my patch.  I can't see
> how it could possibly become recursive.

ok

Re: [Qemu-devel] [PATCH v4 10/10] block: Avoid bdrv_get_geometry() where errors should be detected

[Benoît Canet]

The Wednesday 04 Jun 2014 à 13:51:51 (+0200), Markus Armbruster wrote :
> bdrv_get_geometry() hides errors.  Use bdrv_nb_sectors() or
> bdrv_getlength() instead where that's obviously inappropriate.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c      | 11 ++++++++---
>  block/qapi.c | 14 +++++++++----
>  qemu-img.c   | 64 +++++++++++++++++++++++++++++++++++++++++++++++-------------
>  3 files changed, 68 insertions(+), 21 deletions(-)
> 
> diff --git a/block.c b/block.c
> index cae9085..b92f05f 100644
> --- a/block.c
> +++ b/block.c
> @@ -5574,7 +5574,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
>      if (size && size->value.n == -1) {
>          if (backing_file && backing_file->value.s) {
>              BlockDriverState *bs;
> -            uint64_t size;
> +            int64_t size;
>              char buf[32];
>              int back_flags;
>  
> @@ -5593,8 +5593,13 @@ void bdrv_img_create(const char *filename, const char *fmt,
>                  local_err = NULL;
>                  goto out;
>              }
> -            bdrv_get_geometry(bs, &size);
> -            size *= 512;
> +            size = bdrv_getlength(bs);
> +            if (size < 0) {
> +                error_setg_errno(errp, -size, "Could not get size of '%s'",
> +                                 backing_file->value.s);
> +                bdrv_unref(bs);
> +                goto out;
> +            }
>  
>              snprintf(buf, sizeof(buf), "%" PRId64, size);
>              set_option_parameter(param, BLOCK_OPT_SIZE, buf);
> diff --git a/block/qapi.c b/block/qapi.c
> index 97e1641..90f406d 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -165,19 +165,25 @@ void bdrv_query_image_info(BlockDriverState *bs,
>                             ImageInfo **p_info,
>                             Error **errp)
>  {
> -    uint64_t total_sectors;
> +    int64_t size;
>      const char *backing_filename;
>      char backing_filename2[1024];
>      BlockDriverInfo bdi;
>      int ret;
>      Error *err = NULL;
> -    ImageInfo *info = g_new0(ImageInfo, 1);
> +    ImageInfo *info;
>  
> -    bdrv_get_geometry(bs, &total_sectors);
> +    size = bdrv_getlength(bs);
> +    if (size < 0) {
> +        error_setg_errno(errp, -size, "Can't get size of device '%s'",
> +                         bdrv_get_device_name(bs));
> +        return;
> +    }
>  
> +    info = g_new0(ImageInfo, 1);
>      info->filename        = g_strdup(bs->filename);
>      info->format          = g_strdup(bdrv_get_format_name(bs));
> -    info->virtual_size    = total_sectors * 512;
> +    info->virtual_size    = size;
>      info->actual_size     = bdrv_get_allocated_file_size(bs);
>      info->has_actual_size = info->actual_size >= 0;
>      if (bdrv_is_encrypted(bs)) {
> diff --git a/qemu-img.c b/qemu-img.c
> index e6d0edf..7e6dde0 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -958,7 +958,6 @@ static int img_compare(int argc, char **argv)
>      int64_t sector_num = 0;
>      int64_t nb_sectors;
>      int c, pnum;
> -    uint64_t bs_sectors;
>      uint64_t progress_base;
>  
>      for (;;) {
> @@ -1020,10 +1019,20 @@ static int img_compare(int argc, char **argv)
>  
>      buf1 = qemu_blockalign(bs1, IO_BUF_SIZE);
>      buf2 = qemu_blockalign(bs2, IO_BUF_SIZE);
> -    bdrv_get_geometry(bs1, &bs_sectors);
> -    total_sectors1 = bs_sectors;
> -    bdrv_get_geometry(bs2, &bs_sectors);
> -    total_sectors2 = bs_sectors;
> +    total_sectors1 = bdrv_nb_sectors(bs1);
> +    if (total_sectors1 < 0) {
> +        error_report("Can't get size of %s: %s",
> +                     filename1, strerror(-total_sectors1));
> +        ret = 4;
> +        goto out;
> +    }
> +    total_sectors2 = bdrv_nb_sectors(bs2);
> +    if (total_sectors2 < 0) {
> +        error_report("Can't get size of %s: %s",
> +                     filename2, strerror(-total_sectors2));
> +        ret = 4;
> +        goto out;
> +    }
>      total_sectors = MIN(total_sectors1, total_sectors2);
>      progress_base = MAX(total_sectors1, total_sectors2);
>  
> @@ -1185,7 +1194,7 @@ static int img_convert(int argc, char **argv)
>      BlockDriver *drv, *proto_drv;
>      BlockDriverState **bs = NULL, *out_bs = NULL;
>      int64_t total_sectors, nb_sectors, sector_num, bs_offset;
> -    uint64_t *bs_sectors = NULL;
> +    int64_t *bs_sectors = NULL;
>      uint8_t * buf = NULL;
>      size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
>      const uint8_t *buf1;
> @@ -1326,7 +1335,7 @@ static int img_convert(int argc, char **argv)
>      qemu_progress_print(0, 100);
>  
>      bs = g_new0(BlockDriverState *, bs_n);
> -    bs_sectors = g_new(uint64_t, bs_n);
> +    bs_sectors = g_new(int64_t, bs_n);
>  
>      total_sectors = 0;
>      for (bs_i = 0; bs_i < bs_n; bs_i++) {
> @@ -1340,7 +1349,13 @@ static int img_convert(int argc, char **argv)
>              ret = -1;
>              goto out;
>          }
> -        bdrv_get_geometry(bs[bs_i], &bs_sectors[bs_i]);
> +        bs_sectors[bs_i] = bdrv_nb_sectors(bs[bs_i]);
> +        if (bs_sectors[bs_i] < 0) {
> +            error_report("Could not get size of %s: %s",
> +                         argv[optind + bs_i], strerror(-bs_sectors[bs_i]));
> +            ret = -1;
> +            goto out;
> +        }
>          total_sectors += bs_sectors[bs_i];
>      }
>  
> @@ -2421,9 +2436,9 @@ static int img_rebase(int argc, char **argv)
>       * the image is the same as the original one at any time.
>       */
>      if (!unsafe) {
> -        uint64_t num_sectors;
> -        uint64_t old_backing_num_sectors;
> -        uint64_t new_backing_num_sectors = 0;
> +        int64_t num_sectors;
> +        int64_t old_backing_num_sectors;
> +        int64_t new_backing_num_sectors = 0;
>          uint64_t sector;
>          int n;
>          uint8_t * buf_old;
> @@ -2433,10 +2448,31 @@ static int img_rebase(int argc, char **argv)
>          buf_old = qemu_blockalign(bs, IO_BUF_SIZE);
>          buf_new = qemu_blockalign(bs, IO_BUF_SIZE);
>  
> -        bdrv_get_geometry(bs, &num_sectors);
> -        bdrv_get_geometry(bs_old_backing, &old_backing_num_sectors);
> +        num_sectors = bdrv_nb_sectors(bs);
> +        if (num_sectors < 0) {
> +            error_report("Could not get size of '%s': %s",
> +                         filename, strerror(-num_sectors));
> +            ret = -1;
> +            goto out;
> +        }
> +        old_backing_num_sectors = bdrv_nb_sectors(bs_old_backing);
> +        if (old_backing_num_sectors < 0) {
> +            char backing_name[1024];
> +
> +            bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
> +            error_report("Could not get size of '%s': %s",
> +                         backing_name, strerror(-old_backing_num_sectors));
> +            ret = -1;
> +            goto out;
> +        }
>          if (bs_new_backing) {
> -            bdrv_get_geometry(bs_new_backing, &new_backing_num_sectors);
> +            new_backing_num_sectors = bdrv_nb_sectors(bs_new_backing);
> +            if (new_backing_num_sectors < 0) {
> +                error_report("Could not get size of '%s': %s",
> +                             out_baseimg, strerror(-new_backing_num_sectors));
> +                ret = -1;
> +                goto out;
> +            }
>          }
>  
>          if (num_sectors != 0) {
> -- 
> 1.9.3
> 
Reviewed-by: Benoit Canet <benoit@irqsave.net>