[Qemu-devel] [PATCH 0/5] Clean up around bdrv_getlength()

[Qemu-devel] [PATCH 0/5] 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+3].

* 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 4].

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

Issues not addressed:

* 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.

Markus Armbruster (5):
  raw-posix: Fix raw_getlength() to always return -errno on error
  block: New bdrv_nb_sectors()
  block: Use bdrv_nb_sectors() when sectors, not bytes are wanted
  block: Drop superfluous aligning of bdrv_getlength()'s value
  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            | 93 ++++++++++++++++++++++++++++++++++-----------------
 8 files changed, 147 insertions(+), 87 deletions(-)

-- 
1.8.1.4

[Qemu-devel] [PATCH 1/5] 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>
---
 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 3ce026d..9bf06e5 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1094,12 +1094,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
@@ -1113,7 +1113,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;
 
@@ -1123,7 +1123,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;
         }
@@ -1136,6 +1136,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) {
@@ -1154,7 +1155,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)
@@ -1192,6 +1197,9 @@ again:
         size = LONG_LONG_MAX;
 #else
         size = lseek(fd, 0LL, SEEK_END);
+        if (size < 0) {
+            return -errno;
+        }
 #endif
 #if defined(__FreeBSD__) || defined(__FreeBSD_kernel__)
         switch(s->type) {
@@ -1208,6 +1216,9 @@ again:
 #endif
     } else {
         size = lseek(fd, 0, SEEK_END);
+        if (size < 0) {
+            return -errno;
+        }
     }
     return size;
 }
@@ -1216,13 +1227,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.8.1.4

[Qemu-devel] [PATCH 2/5] 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>
---
 block.c               | 29 +++++++++++++++++++----------
 include/block/block.h |  1 +
 2 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c
index b749d31..44e1f57 100644
--- a/block.c
+++ b/block.c
@@ -689,6 +689,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)
 {
@@ -3470,11 +3471,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;
 
@@ -3484,19 +3486,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 467fb2b..0438b58 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -252,6 +252,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.8.1.4

[Qemu-devel] [PATCH 3/5] block: Use bdrv_nb_sectors() when sectors, not bytes are wanted

[Markus Armbruster]

Instead of bdrv_nb_sectors().

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>
---
 block-migration.c |  9 ++++-----
 block.c           | 40 ++++++++++++++++++----------------------
 block/qcow2.c     |  2 +-
 block/vmdk.c      |  5 ++---
 qemu-img.c        |  8 ++++----
 5 files changed, 29 insertions(+), 35 deletions(-)

diff --git a/block-migration.c b/block-migration.c
index 56951e0..f5bda8f 100644
--- a/block-migration.c
+++ b/block-migration.c
@@ -185,7 +185,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 {
@@ -222,8 +222,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);
@@ -349,7 +348,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;
         }
@@ -797,7 +796,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 44e1f57..1b99cb1 100644
--- a/block.c
+++ b/block.c
@@ -2784,18 +2784,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;
         }
@@ -3012,15 +3010,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) {
@@ -3848,21 +3845,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_getlength(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;
     }
@@ -3893,8 +3890,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_getlength(bs2);
+            if (nb_sectors2 >= 0 && sector_num >= nb_sectors2) {
                 ret |= BDRV_BLOCK_ZERO;
             }
         }
@@ -5178,13 +5175,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 a4b97e8..98f624c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1539,7 +1539,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 06a1f9f..eb2b03e 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));
@@ -1991,7 +1990,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;
 
diff --git a/qemu-img.c b/qemu-img.c
index 96f4463..73aac0f 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1459,13 +1459,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.8.1.4

[Qemu-devel] [PATCH 4/5] 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>
---
 block.c       | 1 -
 block/qcow2.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/block.c b/block.c
index 1b99cb1..89cab7c 100644
--- a/block.c
+++ b/block.c
@@ -1228,7 +1228,6 @@ void bdrv_append_temp_snapshot(BlockDriverState *bs, 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 98f624c..4af09bd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1927,7 +1927,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.8.1.4

[Qemu-devel] [PATCH 5/5] 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>
---
 block.c      | 11 +++++---
 block/qapi.c | 14 +++++++---
 qemu-img.c   | 85 +++++++++++++++++++++++++++++++++++++++++-------------------
 3 files changed, 76 insertions(+), 34 deletions(-)

diff --git a/block.c b/block.c
index 89cab7c..c78362e 100644
--- a/block.c
+++ b/block.c
@@ -5431,7 +5431,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;
 
@@ -5450,8 +5450,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 af11445..bb8c08e 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -164,19 +164,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 73aac0f..6b4804c 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -940,7 +940,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 (;;) {
@@ -1002,10 +1001,18 @@ 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));
+        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));
+        goto out;
+    }
     total_sectors = MIN(total_sectors1, total_sectors2);
     progress_base = MAX(total_sectors1, total_sectors2);
 
@@ -1167,7 +1174,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;
+    int64_t *bs_sectors = NULL;
     uint8_t * buf = NULL;
     size_t bufsectors = IO_BUF_SIZE / BDRV_SECTOR_SIZE;
     const uint8_t *buf1;
@@ -1307,7 +1314,8 @@ static int img_convert(int argc, char **argv)
 
     qemu_progress_print(0, 100);
 
-    bs = g_malloc0(bs_n * sizeof(BlockDriverState *));
+    bs = g_new(BlockDriverState *, bs_n);
+    bs_sectors = g_new(int64_t, bs_n);
 
     total_sectors = 0;
     for (bs_i = 0; bs_i < bs_n; bs_i++) {
@@ -1321,8 +1329,14 @@ static int img_convert(int argc, char **argv)
             ret = -1;
             goto out;
         }
-        bdrv_get_geometry(bs[bs_i], &bs_sectors);
-        total_sectors += bs_sectors;
+        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];
     }
 
     if (sn_opts) {
@@ -1446,7 +1460,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)
@@ -1512,19 +1525,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_i++;
                     assert (bs_i < bs_n);
-                    bs_offset += bs_sectors;
-                    bdrv_get_geometry(bs[bs_i], &bs_sectors);
+                    bs_offset += bs_sectors[bs_i];
                     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) {
@@ -1585,14 +1598,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) &&
@@ -1645,7 +1657,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) {
@@ -1703,6 +1715,7 @@ out:
         }
         g_free(bs);
     }
+    g_free(bs_sectors);
 fail_getopt:
     g_free(options);
 
@@ -2402,9 +2415,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;
@@ -2414,10 +2427,28 @@ 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));
+            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));
+            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));
+                goto out;
+            }
         }
 
         if (num_sectors != 0) {
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH 0/5] Clean up around bdrv_getlength()

[Stefan Hajnoczi]

On Fri, May 09, 2014 at 11:48:13AM +0200, Markus Armbruster wrote:
> 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+3].
> 
> * 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 4].
> 
> * bdrv_get_geometry() hides errors.  Don't use it in places where
>   errors should be detected [PATCH 5].
> 
> Issues not addressed:
> 
> * 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.
> 
> Markus Armbruster (5):
>   raw-posix: Fix raw_getlength() to always return -errno on error
>   block: New bdrv_nb_sectors()
>   block: Use bdrv_nb_sectors() when sectors, not bytes are wanted
>   block: Drop superfluous aligning of bdrv_getlength()'s value
>   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            | 93 ++++++++++++++++++++++++++++++++++-----------------
>  8 files changed, 147 insertions(+), 87 deletions(-)
> 
> -- 
> 1.8.1.4
> 
> 

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

Re: [Qemu-devel] [PATCH 0/5] Clean up around bdrv_getlength()

[Stefan Hajnoczi]

On Fri, May 09, 2014 at 11:48:13AM +0200, Markus Armbruster wrote:
> 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+3].
> 
> * 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 4].
> 
> * bdrv_get_geometry() hides errors.  Don't use it in places where
>   errors should be detected [PATCH 5].
> 
> Issues not addressed:
> 
> * 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.
> 
> Markus Armbruster (5):
>   raw-posix: Fix raw_getlength() to always return -errno on error
>   block: New bdrv_nb_sectors()
>   block: Use bdrv_nb_sectors() when sectors, not bytes are wanted

There's a regression in this patch.

Dropped from the block queue because qemu-iotests ./check -qcow2 030
seems to go into an infinite loop or at least takes a long time.

Stefan

Re: [Qemu-devel] [PATCH 3/5] block: Use bdrv_nb_sectors() when sectors, not bytes are wanted

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Instead of bdrv_nb_sectors().
>
> 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>
> ---
[...]
> diff --git a/block.c b/block.c
> index 44e1f57..1b99cb1 100644
> --- a/block.c
> +++ b/block.c
[...]
> @@ -3848,21 +3845,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_getlength(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;
>      }
> @@ -3893,8 +3890,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_getlength(bs2);
> +            if (nb_sectors2 >= 0 && sector_num >= nb_sectors2) {
>                  ret |= BDRV_BLOCK_ZERO;
>              }
>          }
[...]

I neglected to actually replace bdrv_getlength() by bdrv_nb_sectors()
here, breaking test 030 (I forgot that make check-block doesn't run all
the tests).  With that fixed, the tests pass.  Full respin wanted?

Re: [Qemu-devel] [PATCH 2/5] block: New bdrv_nb_sectors()

[Kevin Wolf]

Am 09.05.2014 um 11:48 hat Markus Armbruster geschrieben:
> 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>

Is this really the right direction to move?

Generally I think we should be trying to move away from those arbitrary
units of 512 bytes (that are called sectors for some reason, but aren't
really related to either guest or host sector sizes), and towards
interfaces that use byte granularity.

So I would rather see bs->total_sectors becoming bs->total_bytes than
adding a new sector-based function.

Kevin

Re: [Qemu-devel] [PATCH 2/5] block: New bdrv_nb_sectors()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 09.05.2014 um 11:48 hat Markus Armbruster geschrieben:
>> 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>
>
> Is this really the right direction to move?
>
> Generally I think we should be trying to move away from those arbitrary
> units of 512 bytes (that are called sectors for some reason, but aren't
> really related to either guest or host sector sizes), and towards
> interfaces that use byte granularity.

My patches neither move towards nor away from a byte-based interface.

> So I would rather see bs->total_sectors becoming bs->total_bytes than
> adding a new sector-based function.

Doing away with BDRV_SECTOR_SIZE & friends is beyond my reach.

My initial hope was to replace bdrv_get_geometry() by bdrv_nb_sectors(),
but I ran out of steam reviewing whether and how the callers should
handle errors.  So yes, this adds another sector-based function.  But I
believe it makes the code a bit simpler, and won't make an eventual
conversion to byte-based any harder.

Re: [Qemu-devel] [PATCH 2/5] block: New bdrv_nb_sectors()

[Eric Blake]

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

On 05/12/2014 05:11 AM, Kevin Wolf wrote:

>> The next patches will convert some users of bdrv_getlength() to
>> bdrv_nb_sectors().
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> Is this really the right direction to move?
> 
> Generally I think we should be trying to move away from those arbitrary
> units of 512 bytes (that are called sectors for some reason, but aren't
> really related to either guest or host sector sizes), and towards
> interfaces that use byte granularity.
> 
> So I would rather see bs->total_sectors becoming bs->total_bytes than
> adding a new sector-based function.

I tend to agree that storing rounded information is the wrong thing to
do.  The qcow2 format stores the guest size in bytes, so it is
technically possible to have a file where the last sector is only a
partial sector.  However, because the current implementation rounds to
sectors (and in some cases, rounds in the wrong direction), we have
situations like:

$ qemu-img create -f qcow2 img 1234
Formatting 'img', fmt=qcow2 size=1234 encryption=off cluster_size=65536
lazy_refcounts=off
$ qemu-img info img
image: img
file format: qcow2
virtual size: 1.0K (1024 bytes)
disk size: 196K
cluster_size: 65536
Format specific information:
    compat: 1.1
    lazy refcounts: false

where I lost 210 bytes of what the guest was supposed to be able to
access.  Consistently tracking bdrv length in bytes, and rounding up to
sectors when needed, seems like it will be better than pre-rounding to
sectors and possibly truncating the user image.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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