[Qemu-devel] [PATCH for-2.10 0/3] block: Add errp to b{lk, drv}_turncate()

[Qemu-devel] [PATCH for-2.10 0/3] block: Add errp to b{lk, drv}_turncate()

[Max Reitz]

Having an Error parameter for these functions makes sense because we
sometimes want a bit more information than just "Something failed". Some
drivers already use error_report() and the like to emit this additional
information, so it's rather obvious that we do want a real error object
here.

Max Reitz (3):
  block: Add errp to b{lk,drv}_truncate()
  block: Add errp to BD.bdrv_truncate()
  block: Add some bdrv_truncate() error messages

 include/block/block.h          |  2 +-
 include/block/block_int.h      |  2 +-
 include/sysemu/block-backend.h |  2 +-
 block.c                        | 18 +++++++++++++-----
 block/archipelago.c            |  3 ++-
 block/blkdebug.c               |  4 ++--
 block/block-backend.c          |  5 +++--
 block/commit.c                 |  5 +++--
 block/crypto.c                 |  5 +++--
 block/file-posix.c             | 10 ++++++++--
 block/file-win32.c             |  6 +++---
 block/gluster.c                |  3 ++-
 block/iscsi.c                  |  4 ++--
 block/mirror.c                 |  2 +-
 block/nfs.c                    |  2 +-
 block/parallels.c              | 13 ++++++++-----
 block/qcow.c                   |  6 +++---
 block/qcow2-refcount.c         |  5 ++++-
 block/qcow2.c                  | 23 ++++++++++++++---------
 block/qed.c                    |  8 +++++---
 block/raw-format.c             |  6 ++++--
 block/rbd.c                    |  2 +-
 block/sheepdog.c               | 14 ++++++--------
 block/vdi.c                    |  4 ++--
 block/vhdx-log.c               |  2 +-
 block/vhdx.c                   |  6 +++---
 block/vmdk.c                   | 13 +++----------
 block/vpc.c                    |  2 +-
 blockdev.c                     | 21 +--------------------
 qemu-img.c                     | 17 ++++-------------
 qemu-io-cmds.c                 |  2 +-
 31 files changed, 107 insertions(+), 110 deletions(-)

-- 
2.12.0

[Qemu-devel] [PATCH for-2.10 1/3] block: Add errp to b{lk, drv}_truncate()

[Max Reitz]

For one thing, this allows us to drop the error message generation from
qemu-img.c and blockdev.c and instead have it unified in
bdrv_truncate().

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block.h          |  2 +-
 include/sysemu/block-backend.h |  2 +-
 block.c                        | 16 ++++++++++++----
 block/blkdebug.c               |  2 +-
 block/block-backend.c          |  5 +++--
 block/commit.c                 |  5 +++--
 block/crypto.c                 |  2 +-
 block/mirror.c                 |  2 +-
 block/parallels.c              | 13 ++++++++-----
 block/qcow.c                   |  6 +++---
 block/qcow2-refcount.c         |  5 ++++-
 block/qcow2.c                  | 13 ++++++++-----
 block/qed.c                    |  2 +-
 block/raw-format.c             |  2 +-
 block/vdi.c                    |  4 ++--
 block/vhdx-log.c               |  2 +-
 block/vhdx.c                   |  6 +++---
 block/vmdk.c                   | 13 +++----------
 block/vpc.c                    |  2 +-
 blockdev.c                     | 21 +--------------------
 qemu-img.c                     | 17 ++++-------------
 qemu-io-cmds.c                 |  2 +-
 22 files changed, 64 insertions(+), 80 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index c7c4a3ac3a..c0d1e52ea6 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -294,7 +294,7 @@ BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs,
     const char *backing_file);
 int bdrv_get_backing_file_depth(BlockDriverState *bs);
 void bdrv_refresh_filename(BlockDriverState *bs);
-int bdrv_truncate(BdrvChild *child, int64_t offset);
+int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp);
 int64_t bdrv_nb_sectors(BlockDriverState *bs);
 int64_t bdrv_getlength(BlockDriverState *bs);
 int64_t bdrv_get_allocated_file_size(BlockDriverState *bs);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 096c17fce0..c142106006 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -217,7 +217,7 @@ int coroutine_fn blk_co_pwrite_zeroes(BlockBackend *blk, int64_t offset,
                                       int count, BdrvRequestFlags flags);
 int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
                           int count);
-int blk_truncate(BlockBackend *blk, int64_t offset);
+int blk_truncate(BlockBackend *blk, int64_t offset, Error **errp);
 int blk_pdiscard(BlockBackend *blk, int64_t offset, int count);
 int blk_save_vmstate(BlockBackend *blk, const uint8_t *buf,
                      int64_t pos, int size);
diff --git a/block.c b/block.c
index f293ccb5af..dd24161260 100644
--- a/block.c
+++ b/block.c
@@ -3144,7 +3144,7 @@ exit:
 /**
  * Truncate file to 'offset' bytes (needed only for file protocols)
  */
-int bdrv_truncate(BdrvChild *child, int64_t offset)
+int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp)
 {
     BlockDriverState *bs = child->bs;
     BlockDriver *drv = bs->drv;
@@ -3152,12 +3152,18 @@ int bdrv_truncate(BdrvChild *child, int64_t offset)
 
     assert(child->perm & BLK_PERM_RESIZE);
 
-    if (!drv)
+    if (!drv) {
+        error_setg(errp, "No medium inserted");
         return -ENOMEDIUM;
-    if (!drv->bdrv_truncate)
+    }
+    if (!drv->bdrv_truncate) {
+        error_setg(errp, "Image format driver does not support resize");
         return -ENOTSUP;
-    if (bs->read_only)
+    }
+    if (bs->read_only) {
+        error_setg(errp, "Image is read-only");
         return -EACCES;
+    }
 
     ret = drv->bdrv_truncate(bs, offset);
     if (ret == 0) {
@@ -3165,6 +3171,8 @@ int bdrv_truncate(BdrvChild *child, int64_t offset)
         bdrv_dirty_bitmap_truncate(bs);
         bdrv_parent_cb_resize(bs);
         ++bs->write_gen;
+    } else {
+        error_setg_errno(errp, -ret, "Failed to resize image");
     }
     return ret;
 }
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 67e8024e36..15a9966096 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -663,7 +663,7 @@ static int64_t blkdebug_getlength(BlockDriverState *bs)
 
 static int blkdebug_truncate(BlockDriverState *bs, int64_t offset)
 {
-    return bdrv_truncate(bs->file, offset);
+    return bdrv_truncate(bs->file, offset, NULL);
 }
 
 static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options)
diff --git a/block/block-backend.c b/block/block-backend.c
index daa7908d01..97c9b0d315 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1693,13 +1693,14 @@ int blk_pwrite_compressed(BlockBackend *blk, int64_t offset, const void *buf,
                    BDRV_REQ_WRITE_COMPRESSED);
 }
 
-int blk_truncate(BlockBackend *blk, int64_t offset)
+int blk_truncate(BlockBackend *blk, int64_t offset, Error **errp)
 {
     if (!blk_is_available(blk)) {
+        error_setg(errp, "No medium inserted");
         return -ENOMEDIUM;
     }
 
-    return bdrv_truncate(blk->root, offset);
+    return bdrv_truncate(blk->root, offset, errp);
 }
 
 static void blk_pdiscard_entry(void *opaque)
diff --git a/block/commit.c b/block/commit.c
index 22a0a4db98..97e52e0a8e 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -150,7 +150,7 @@ static void coroutine_fn commit_run(void *opaque)
     }
 
     if (base_len < s->common.len) {
-        ret = blk_truncate(s->base, s->common.len);
+        ret = blk_truncate(s->base, s->common.len, NULL);
         if (ret) {
             goto out;
         }
@@ -476,8 +476,9 @@ int bdrv_commit(BlockDriverState *bs)
      * grow the backing file image if possible.  If not possible,
      * we must return an error */
     if (length > backing_length) {
-        ret = blk_truncate(backing, length);
+        ret = blk_truncate(backing, length, &local_err);
         if (ret < 0) {
+            error_report_err(local_err);
             goto ro_cleanup;
         }
     }
diff --git a/block/crypto.c b/block/crypto.c
index 4a2038888d..52e4f2b20f 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -389,7 +389,7 @@ static int block_crypto_truncate(BlockDriverState *bs, int64_t offset)
 
     offset += payload_offset;
 
-    return bdrv_truncate(bs->file, offset);
+    return bdrv_truncate(bs->file, offset, NULL);
 }
 
 static void block_crypto_close(BlockDriverState *bs)
diff --git a/block/mirror.c b/block/mirror.c
index 57f26c33a4..979d131a80 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -710,7 +710,7 @@ static void coroutine_fn mirror_run(void *opaque)
         }
 
         if (s->bdev_length > base_length) {
-            ret = blk_truncate(s->target, s->bdev_length);
+            ret = blk_truncate(s->target, s->bdev_length, NULL);
             if (ret < 0) {
                 goto immediate_exit;
             }
diff --git a/block/parallels.c b/block/parallels.c
index 19935e29a9..618fc860e2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -216,7 +216,8 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
                                      space << BDRV_SECTOR_BITS, 0);
         } else {
             ret = bdrv_truncate(bs->file,
-                                (s->data_end + space) << BDRV_SECTOR_BITS);
+                                (s->data_end + space) << BDRV_SECTOR_BITS,
+                                NULL);
         }
         if (ret < 0) {
             return ret;
@@ -449,8 +450,10 @@ static int parallels_check(BlockDriverState *bs, BdrvCheckResult *res,
                 size - res->image_end_offset);
         res->leaks += count;
         if (fix & BDRV_FIX_LEAKS) {
-            ret = bdrv_truncate(bs->file, res->image_end_offset);
+            Error *local_err = NULL;
+            ret = bdrv_truncate(bs->file, res->image_end_offset, &local_err);
             if (ret < 0) {
+                error_report_err(local_err);
                 res->check_errors++;
                 return ret;
             }
@@ -497,7 +500,7 @@ static int parallels_create(const char *filename, QemuOpts *opts, Error **errp)
 
     blk_set_allow_write_beyond_eof(file, true);
 
-    ret = blk_truncate(file, 0);
+    ret = blk_truncate(file, 0, errp);
     if (ret < 0) {
         goto exit;
     }
@@ -688,7 +691,7 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail_options;
     }
     if (!bdrv_has_zero_init(bs->file->bs) ||
-            bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs)) != 0) {
+            bdrv_truncate(bs->file, bdrv_getlength(bs->file->bs), NULL) != 0) {
         s->prealloc_mode = PRL_PREALLOC_MODE_FALLOCATE;
     }
 
@@ -731,7 +734,7 @@ static void parallels_close(BlockDriverState *bs)
     }
 
     if (bs->open_flags & BDRV_O_RDWR) {
-        bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS);
+        bdrv_truncate(bs->file, s->data_end << BDRV_SECTOR_BITS, NULL);
     }
 
     g_free(s->bat_dirty_bmap);
diff --git a/block/qcow.c b/block/qcow.c
index 9d6ac83959..5d147b962e 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -473,7 +473,7 @@ static uint64_t get_cluster_offset(BlockDriverState *bs,
                 /* round to cluster size */
                 cluster_offset = (cluster_offset + s->cluster_size - 1) &
                     ~(s->cluster_size - 1);
-                bdrv_truncate(bs->file, cluster_offset + s->cluster_size);
+                bdrv_truncate(bs->file, cluster_offset + s->cluster_size, NULL);
                 /* if encrypted, we must initialize the cluster
                    content which won't be written */
                 if (bs->encrypted &&
@@ -833,7 +833,7 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
 
     blk_set_allow_write_beyond_eof(qcow_blk, true);
 
-    ret = blk_truncate(qcow_blk, 0);
+    ret = blk_truncate(qcow_blk, 0, errp);
     if (ret < 0) {
         goto exit;
     }
@@ -916,7 +916,7 @@ static int qcow_make_empty(BlockDriverState *bs)
     if (bdrv_pwrite_sync(bs->file, s->l1_table_offset, s->l1_table,
             l1_length) < 0)
         return -1;
-    ret = bdrv_truncate(bs->file, s->l1_table_offset + l1_length);
+    ret = bdrv_truncate(bs->file, s->l1_table_offset + l1_length, NULL);
     if (ret < 0)
         return ret;
 
diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 9e96f64c8b..4efca7ebdb 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1728,14 +1728,17 @@ static int check_refblocks(BlockDriverState *bs, BdrvCheckResult *res,
 
             if (fix & BDRV_FIX_ERRORS) {
                 int64_t new_nb_clusters;
+                Error *local_err = NULL;
 
                 if (offset > INT64_MAX - s->cluster_size) {
                     ret = -EINVAL;
                     goto resize_fail;
                 }
 
-                ret = bdrv_truncate(bs->file, offset + s->cluster_size);
+                ret = bdrv_truncate(bs->file, offset + s->cluster_size,
+                                    &local_err);
                 if (ret < 0) {
+                    error_report_err(local_err);
                     goto resize_fail;
                 }
                 size = bdrv_getlength(bs->file->bs);
diff --git a/block/qcow2.c b/block/qcow2.c
index 6a92d2ef3f..43b8a986f0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2294,9 +2294,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     }
 
     /* Okay, now that we have a valid image, let's give it the right size */
-    ret = blk_truncate(blk, total_size);
+    ret = blk_truncate(blk, total_size, errp);
     if (ret < 0) {
-        error_setg_errno(errp, -ret, "Could not resize image");
         goto out;
     }
 
@@ -2584,7 +2583,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
         /* align end of file to a sector boundary to ease reading with
            sector based I/Os */
         cluster_offset = bdrv_getlength(bs->file->bs);
-        return bdrv_truncate(bs->file, cluster_offset);
+        return bdrv_truncate(bs->file, cluster_offset, NULL);
     }
 
     buf = qemu_blockalign(bs, s->cluster_size);
@@ -2674,6 +2673,7 @@ fail:
 static int make_completely_empty(BlockDriverState *bs)
 {
     BDRVQcow2State *s = bs->opaque;
+    Error *local_err = NULL;
     int ret, l1_clusters;
     int64_t offset;
     uint64_t *new_reftable = NULL;
@@ -2798,8 +2798,10 @@ static int make_completely_empty(BlockDriverState *bs)
         goto fail;
     }
 
-    ret = bdrv_truncate(bs->file, (3 + l1_clusters) * s->cluster_size);
+    ret = bdrv_truncate(bs->file, (3 + l1_clusters) * s->cluster_size,
+                        &local_err);
     if (ret < 0) {
+        error_report_err(local_err);
         goto fail;
     }
 
@@ -3273,9 +3275,10 @@ static int qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
             return ret;
         }
 
-        ret = blk_truncate(blk, new_size);
+        ret = blk_truncate(blk, new_size, &local_err);
         blk_unref(blk);
         if (ret < 0) {
+            error_report_err(local_err);
             return ret;
         }
     }
diff --git a/block/qed.c b/block/qed.c
index 5ec7fd83f2..53199ac8d7 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -635,7 +635,7 @@ static int qed_create(const char *filename, uint32_t cluster_size,
     blk_set_allow_write_beyond_eof(blk, true);
 
     /* File must start empty and grow, check truncate is supported */
-    ret = blk_truncate(blk, 0);
+    ret = blk_truncate(blk, 0, errp);
     if (ret < 0) {
         goto out;
     }
diff --git a/block/raw-format.c b/block/raw-format.c
index 86fbc657eb..a80073369a 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -341,7 +341,7 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset)
 
     s->size = offset;
     offset += s->offset;
-    return bdrv_truncate(bs->file, offset);
+    return bdrv_truncate(bs->file, offset, NULL);
 }
 
 static int raw_media_changed(BlockDriverState *bs)
diff --git a/block/vdi.c b/block/vdi.c
index 9b4f70e977..d12d9cdc79 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -832,9 +832,9 @@ static int vdi_create(const char *filename, QemuOpts *opts, Error **errp)
     }
 
     if (image_type == VDI_TYPE_STATIC) {
-        ret = blk_truncate(blk, offset + blocks * block_size);
+        ret = blk_truncate(blk, offset + blocks * block_size, errp);
         if (ret < 0) {
-            error_setg(errp, "Failed to statically allocate %s", filename);
+            error_prepend(errp, "Failed to statically allocate %s", filename);
             goto exit;
         }
     }
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index 67a91c0de5..3f4c2aa095 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -548,7 +548,7 @@ static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
             if (new_file_size % (1024*1024)) {
                 /* round up to nearest 1MB boundary */
                 new_file_size = ((new_file_size >> 20) + 1) << 20;
-                bdrv_truncate(bs->file, new_file_size);
+                bdrv_truncate(bs->file, new_file_size, NULL);
             }
         }
         qemu_vfree(desc_entries);
diff --git a/block/vhdx.c b/block/vhdx.c
index 052a753159..19e59b5b4a 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1171,7 +1171,7 @@ static int vhdx_allocate_block(BlockDriverState *bs, BDRVVHDXState *s,
     /* per the spec, the address for a block is in units of 1MB */
     *new_offset = ROUND_UP(*new_offset, 1024 * 1024);
 
-    return bdrv_truncate(bs->file, *new_offset + s->block_size);
+    return bdrv_truncate(bs->file, *new_offset + s->block_size, NULL);
 }
 
 /*
@@ -1607,12 +1607,12 @@ static int vhdx_create_bat(BlockBackend *blk, BDRVVHDXState *s,
     if (type == VHDX_TYPE_DYNAMIC) {
         /* All zeroes, so we can just extend the file - the end of the BAT
          * is the furthest thing we have written yet */
-        ret = blk_truncate(blk, data_file_offset);
+        ret = blk_truncate(blk, data_file_offset, NULL);
         if (ret < 0) {
             goto exit;
         }
     } else if (type == VHDX_TYPE_FIXED) {
-        ret = blk_truncate(blk, data_file_offset + image_size);
+        ret = blk_truncate(blk, data_file_offset + image_size, NULL);
         if (ret < 0) {
             goto exit;
         }
diff --git a/block/vmdk.c b/block/vmdk.c
index a9bd22bf93..c61b9cc8e0 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1714,10 +1714,7 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
     blk_set_allow_write_beyond_eof(blk, true);
 
     if (flat) {
-        ret = blk_truncate(blk, filesize);
-        if (ret < 0) {
-            error_setg_errno(errp, -ret, "Could not truncate file");
-        }
+        ret = blk_truncate(blk, filesize, errp);
         goto exit;
     }
     magic = cpu_to_be32(VMDK4_MAGIC);
@@ -1780,9 +1777,8 @@ static int vmdk_create_extent(const char *filename, int64_t filesize,
         goto exit;
     }
 
-    ret = blk_truncate(blk, le64_to_cpu(header.grain_offset) << 9);
+    ret = blk_truncate(blk, le64_to_cpu(header.grain_offset) << 9, errp);
     if (ret < 0) {
-        error_setg_errno(errp, -ret, "Could not truncate file");
         goto exit;
     }
 
@@ -2090,10 +2086,7 @@ static int vmdk_create(const char *filename, QemuOpts *opts, Error **errp)
     /* bdrv_pwrite write padding zeros to align to sector, we don't need that
      * for description file */
     if (desc_offset == 0) {
-        ret = blk_truncate(new_blk, desc_len);
-        if (ret < 0) {
-            error_setg_errno(errp, -ret, "Could not truncate file");
-        }
+        ret = blk_truncate(new_blk, desc_len, errp);
     }
 exit:
     if (new_blk) {
diff --git a/block/vpc.c b/block/vpc.c
index f591d4be38..488095250a 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -858,7 +858,7 @@ static int create_fixed_disk(BlockBackend *blk, uint8_t *buf,
     /* Add footer to total size */
     total_size += HEADER_SIZE;
 
-    ret = blk_truncate(blk, total_size);
+    ret = blk_truncate(blk, total_size, NULL);
     if (ret < 0) {
         return ret;
     }
diff --git a/blockdev.c b/blockdev.c
index 8eb4e84fe0..2f5c587712 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2926,26 +2926,7 @@ void qmp_block_resize(bool has_device, const char *device,
     /* complete all in-flight operations before resizing the device */
     bdrv_drain_all();
 
-    ret = blk_truncate(blk, size);
-    switch (ret) {
-    case 0:
-        break;
-    case -ENOMEDIUM:
-        error_setg(errp, QERR_DEVICE_HAS_NO_MEDIUM, device);
-        break;
-    case -ENOTSUP:
-        error_setg(errp, QERR_UNSUPPORTED);
-        break;
-    case -EACCES:
-        error_setg(errp, "Device '%s' is read only", device);
-        break;
-    case -EBUSY:
-        error_setg(errp, QERR_DEVICE_IN_USE, device);
-        break;
-    default:
-        error_setg_errno(errp, -ret, "Could not resize");
-        break;
-    }
+    ret = blk_truncate(blk, size, errp);
 
 out:
     blk_unref(blk);
diff --git a/qemu-img.c b/qemu-img.c
index 98b836b030..0b0178949d 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3440,20 +3440,11 @@ static int img_resize(int argc, char **argv)
         goto out;
     }
 
-    ret = blk_truncate(blk, total_size);
-    switch (ret) {
-    case 0:
+    ret = blk_truncate(blk, total_size, &err);
+    if (!ret) {
         qprintf(quiet, "Image resized.\n");
-        break;
-    case -ENOTSUP:
-        error_report("This image does not support resize");
-        break;
-    case -EACCES:
-        error_report("Image is read-only");
-        break;
-    default:
-        error_report("Error resizing image: %s", strerror(-ret));
-        break;
+    } else {
+        error_report_err(err);
     }
 out:
     blk_unref(blk);
diff --git a/qemu-io-cmds.c b/qemu-io-cmds.c
index 2c48f9ce1a..fc4340dedd 100644
--- a/qemu-io-cmds.c
+++ b/qemu-io-cmds.c
@@ -1569,7 +1569,7 @@ static int truncate_f(BlockBackend *blk, int argc, char **argv)
         return 0;
     }
 
-    ret = blk_truncate(blk, offset);
+    ret = blk_truncate(blk, offset, NULL);
     if (ret < 0) {
         printf("truncate: %s\n", strerror(-ret));
         return 0;
-- 
2.12.0

[Qemu-devel] [PATCH for-2.10 2/3] block: Add errp to BD.bdrv_truncate()

[Max Reitz]

Add an Error parameter to the block drivers' bdrv_truncate() interface.
If a block driver does not set this in case of an error, the generic
bdrv_truncate() implementation will do so.

Where it is obvious, this patch also makes some block drivers set this
value.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/block/block_int.h |  2 +-
 block.c                   |  4 ++--
 block/archipelago.c       |  3 ++-
 block/blkdebug.c          |  4 ++--
 block/crypto.c            |  5 +++--
 block/file-posix.c        |  2 +-
 block/file-win32.c        |  6 +++---
 block/gluster.c           |  3 ++-
 block/iscsi.c             |  4 ++--
 block/nfs.c               |  2 +-
 block/qcow2.c             |  8 ++++----
 block/qed.c               |  2 +-
 block/raw-format.c        |  4 ++--
 block/rbd.c               |  2 +-
 block/sheepdog.c          | 14 ++++++--------
 15 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index a57c0bfb55..9374e5db74 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -196,7 +196,7 @@ struct BlockDriver {
     int coroutine_fn (*bdrv_co_flush_to_os)(BlockDriverState *bs);
 
     const char *protocol_name;
-    int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset);
+    int (*bdrv_truncate)(BlockDriverState *bs, int64_t offset, Error **errp);
 
     int64_t (*bdrv_getlength)(BlockDriverState *bs);
     bool has_variable_length;
diff --git a/block.c b/block.c
index dd24161260..46ce64fc46 100644
--- a/block.c
+++ b/block.c
@@ -3165,13 +3165,13 @@ int bdrv_truncate(BdrvChild *child, int64_t offset, Error **errp)
         return -EACCES;
     }
 
-    ret = drv->bdrv_truncate(bs, offset);
+    ret = drv->bdrv_truncate(bs, offset, errp);
     if (ret == 0) {
         ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
         bdrv_dirty_bitmap_truncate(bs);
         bdrv_parent_cb_resize(bs);
         ++bs->write_gen;
-    } else {
+    } else if (errp && !*errp) {
         error_setg_errno(errp, -ret, "Failed to resize image");
     }
     return ret;
diff --git a/block/archipelago.c b/block/archipelago.c
index 2449cfc702..0015178381 100644
--- a/block/archipelago.c
+++ b/block/archipelago.c
@@ -976,7 +976,8 @@ static int64_t qemu_archipelago_getlength(BlockDriverState *bs)
     return archipelago_volume_info(s);
 }
 
-static int qemu_archipelago_truncate(BlockDriverState *bs, int64_t offset)
+static int qemu_archipelago_truncate(BlockDriverState *bs, int64_t offset,
+                                     Error *errp)
 {
     int ret, targetlen;
     struct xseg_request *req;
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 15a9966096..c795ae9e72 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -661,9 +661,9 @@ static int64_t blkdebug_getlength(BlockDriverState *bs)
     return bdrv_getlength(bs->file->bs);
 }
 
-static int blkdebug_truncate(BlockDriverState *bs, int64_t offset)
+static int blkdebug_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
 {
-    return bdrv_truncate(bs->file, offset, NULL);
+    return bdrv_truncate(bs->file, offset, errp);
 }
 
 static void blkdebug_refresh_filename(BlockDriverState *bs, QDict *options)
diff --git a/block/crypto.c b/block/crypto.c
index 52e4f2b20f..17b3140998 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -381,7 +381,8 @@ static int block_crypto_create_generic(QCryptoBlockFormat format,
     return ret;
 }
 
-static int block_crypto_truncate(BlockDriverState *bs, int64_t offset)
+static int block_crypto_truncate(BlockDriverState *bs, int64_t offset,
+                                 Error **errp)
 {
     BlockCrypto *crypto = bs->opaque;
     size_t payload_offset =
@@ -389,7 +390,7 @@ static int block_crypto_truncate(BlockDriverState *bs, int64_t offset)
 
     offset += payload_offset;
 
-    return bdrv_truncate(bs->file, offset, NULL);
+    return bdrv_truncate(bs->file, offset, errp);
 }
 
 static void block_crypto_close(BlockDriverState *bs)
diff --git a/block/file-posix.c b/block/file-posix.c
index 4de1abd023..9d2bea730d 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1337,7 +1337,7 @@ static void raw_close(BlockDriverState *bs)
     }
 }
 
-static int raw_truncate(BlockDriverState *bs, int64_t offset)
+static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     struct stat st;
diff --git a/block/file-win32.c b/block/file-win32.c
index 800fabdd72..3f3925623f 100644
--- a/block/file-win32.c
+++ b/block/file-win32.c
@@ -461,7 +461,7 @@ static void raw_close(BlockDriverState *bs)
     }
 }
 
-static int raw_truncate(BlockDriverState *bs, int64_t offset)
+static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     LONG low, high;
@@ -476,11 +476,11 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset)
      */
     dwPtrLow = SetFilePointer(s->hfile, low, &high, FILE_BEGIN);
     if (dwPtrLow == INVALID_SET_FILE_POINTER && GetLastError() != NO_ERROR) {
-        fprintf(stderr, "SetFilePointer error: %lu\n", GetLastError());
+        error_setg_win32(errp, GetLastError(), "SetFilePointer error");
         return -EIO;
     }
     if (SetEndOfFile(s->hfile) == 0) {
-        fprintf(stderr, "SetEndOfFile error: %lu\n", GetLastError());
+        error_setg_win32(errp, GetLastError(), "SetEndOfFile error");
         return -EIO;
     }
     return 0;
diff --git a/block/gluster.c b/block/gluster.c
index 56b4abe3a7..36885ac029 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -1095,7 +1095,8 @@ static coroutine_fn int qemu_gluster_co_rw(BlockDriverState *bs,
     return acb.ret;
 }
 
-static int qemu_gluster_truncate(BlockDriverState *bs, int64_t offset)
+static int qemu_gluster_truncate(BlockDriverState *bs, int64_t offset,
+                                 Error **errp)
 {
     int ret;
     BDRVGlusterState *s = bs->opaque;
diff --git a/block/iscsi.c b/block/iscsi.c
index 75d890538e..ab559a6f71 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -2060,7 +2060,7 @@ static void iscsi_reopen_commit(BDRVReopenState *reopen_state)
     }
 }
 
-static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
+static int iscsi_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
 {
     IscsiLun *iscsilun = bs->opaque;
     Error *local_err = NULL;
@@ -2071,7 +2071,7 @@ static int iscsi_truncate(BlockDriverState *bs, int64_t offset)
 
     iscsi_readcapacity_sync(iscsilun, &local_err);
     if (local_err != NULL) {
-        error_free(local_err);
+        error_propagate(errp, local_err);
         return -EIO;
     }
 
diff --git a/block/nfs.c b/block/nfs.c
index 3f43f6e26a..57d12efc51 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -757,7 +757,7 @@ static int64_t nfs_get_allocated_file_size(BlockDriverState *bs)
     return (task.ret < 0 ? task.ret : st.st_blocks * 512);
 }
 
-static int nfs_file_truncate(BlockDriverState *bs, int64_t offset)
+static int nfs_file_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
 {
     NFSClient *client = bs->opaque;
     return nfs_ftruncate(client->context, client->fh, offset);
diff --git a/block/qcow2.c b/block/qcow2.c
index 43b8a986f0..17585fbb89 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2524,26 +2524,26 @@ static coroutine_fn int qcow2_co_pdiscard(BlockDriverState *bs,
     return ret;
 }
 
-static int qcow2_truncate(BlockDriverState *bs, int64_t offset)
+static int qcow2_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     int64_t new_l1_size;
     int ret;
 
     if (offset & 511) {
-        error_report("The new size must be a multiple of 512");
+        error_setg(errp, "The new size must be a multiple of 512");
         return -EINVAL;
     }
 
     /* cannot proceed if image has snapshots */
     if (s->nb_snapshots) {
-        error_report("Can't resize an image which has snapshots");
+        error_setg(errp, "Can't resize an image which has snapshots");
         return -ENOTSUP;
     }
 
     /* shrinking is currently not supported */
     if (offset < bs->total_sectors * 512) {
-        error_report("qcow2 doesn't support shrinking images yet");
+        error_setg(errp, "qcow2 doesn't support shrinking images yet");
         return -ENOTSUP;
     }
 
diff --git a/block/qed.c b/block/qed.c
index 53199ac8d7..fa2aeee471 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1518,7 +1518,7 @@ static int coroutine_fn bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs,
     return cb.ret;
 }
 
-static int bdrv_qed_truncate(BlockDriverState *bs, int64_t offset)
+static int bdrv_qed_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
 {
     BDRVQEDState *s = bs->opaque;
     uint64_t old_image_size;
diff --git a/block/raw-format.c b/block/raw-format.c
index a80073369a..9761bdaff8 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -327,7 +327,7 @@ static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
     }
 }
 
-static int raw_truncate(BlockDriverState *bs, int64_t offset)
+static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
 
@@ -341,7 +341,7 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset)
 
     s->size = offset;
     offset += s->offset;
-    return bdrv_truncate(bs->file, offset, NULL);
+    return bdrv_truncate(bs->file, offset, errp);
 }
 
 static int raw_media_changed(BlockDriverState *bs)
diff --git a/block/rbd.c b/block/rbd.c
index ee13f3d9d3..f7d4dd93fe 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -1028,7 +1028,7 @@ static int64_t qemu_rbd_getlength(BlockDriverState *bs)
     return info.size;
 }
 
-static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset)
+static int qemu_rbd_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     int r;
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 743471043e..01c24d3528 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1932,9 +1932,8 @@ static int64_t sd_getlength(BlockDriverState *bs)
     return s->inode.vdi_size;
 }
 
-static int sd_truncate(BlockDriverState *bs, int64_t offset)
+static int sd_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
 {
-    Error *local_err = NULL;
     BDRVSheepdogState *s = bs->opaque;
     int ret, fd;
     unsigned int datalen;
@@ -1942,16 +1941,15 @@ static int sd_truncate(BlockDriverState *bs, int64_t offset)
 
     max_vdi_size = (UINT64_C(1) << s->inode.block_size_shift) * MAX_DATA_OBJS;
     if (offset < s->inode.vdi_size) {
-        error_report("shrinking is not supported");
+        error_setg(errp, "shrinking is not supported");
         return -EINVAL;
     } else if (offset > max_vdi_size) {
-        error_report("too big image size");
+        error_setg(errp, "too big image size");
         return -EINVAL;
     }
 
-    fd = connect_to_sdog(s, &local_err);
+    fd = connect_to_sdog(s, errp);
     if (fd < 0) {
-        error_report_err(local_err);
         return fd;
     }
 
@@ -1964,7 +1962,7 @@ static int sd_truncate(BlockDriverState *bs, int64_t offset)
     close(fd);
 
     if (ret < 0) {
-        error_report("failed to update an inode.");
+        error_setg_errno(errp, -ret, "failed to update an inode");
     }
 
     return ret;
@@ -2229,7 +2227,7 @@ static coroutine_fn int sd_co_writev(BlockDriverState *bs, int64_t sector_num,
     BDRVSheepdogState *s = bs->opaque;
 
     if (offset > s->inode.vdi_size) {
-        ret = sd_truncate(bs, offset);
+        ret = sd_truncate(bs, offset, NULL);
         if (ret < 0) {
             return ret;
         }
-- 
2.12.0

[Qemu-devel] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Max Reitz]

Add missing error messages for the drivers I am comfortable to do this
in.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/file-posix.c | 8 +++++++-
 block/qcow2.c      | 2 ++
 block/qed.c        | 4 +++-
 block/raw-format.c | 2 ++
 4 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index 9d2bea730d..553213221c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -1341,20 +1341,26 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     struct stat st;
+    int ret;
 
     if (fstat(s->fd, &st)) {
-        return -errno;
+        ret = -errno;
+        error_setg_errno(errp, -ret, "Failed to fstat() the file");
+        return ret;
     }
 
     if (S_ISREG(st.st_mode)) {
         if (ftruncate(s->fd, offset) < 0) {
+            /* The generic error message will be fine */
             return -errno;
         }
     } else if (S_ISCHR(st.st_mode) || S_ISBLK(st.st_mode)) {
        if (offset > raw_getlength(bs)) {
+           error_setg(errp, "Cannot grow device files");
            return -EINVAL;
        }
     } else {
+        error_setg(errp, "Resizing this file is not supported");
         return -ENOTSUP;
     }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 17585fbb89..53b0bd61a7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2550,6 +2550,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
     new_l1_size = size_to_l1(s, offset);
     ret = qcow2_grow_l1_table(bs, new_l1_size, true);
     if (ret < 0) {
+        error_setg(errp, "Failed to grow the L1 table");
         return ret;
     }
 
@@ -2558,6 +2559,7 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
     ret = bdrv_pwrite_sync(bs->file, offsetof(QCowHeader, size),
                            &offset, sizeof(uint64_t));
     if (ret < 0) {
+        error_setg(errp, "Failed to update the image size");
         return ret;
     }
 
diff --git a/block/qed.c b/block/qed.c
index fa2aeee471..eb346d645b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1526,11 +1526,12 @@ static int bdrv_qed_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
 
     if (!qed_is_image_size_valid(offset, s->header.cluster_size,
                                  s->header.table_size)) {
+        error_setg(errp, "Invalid image size specified");
         return -EINVAL;
     }
 
-    /* Shrinking is currently not supported */
     if ((uint64_t)offset < s->header.image_size) {
+        error_setg(errp, "Shrinking images is currently not supported");
         return -ENOTSUP;
     }
 
@@ -1539,6 +1540,7 @@ static int bdrv_qed_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
     ret = qed_write_header_sync(s);
     if (ret < 0) {
         s->header.image_size = old_image_size;
+        error_setg(errp, "Failed to update the image size");
     }
     return ret;
 }
diff --git a/block/raw-format.c b/block/raw-format.c
index 9761bdaff8..36e65036f0 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -332,10 +332,12 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
     BDRVRawState *s = bs->opaque;
 
     if (s->has_size) {
+        error_setg(errp, "Cannot resize fixed-size raw disks");
         return -ENOTSUP;
     }
 
     if (INT64_MAX - offset < s->offset) {
+        error_setg(errp, "Disk size too large for the chosen offset");
         return -EINVAL;
     }
 
-- 
2.12.0

Re: [Qemu-devel] [PATCH for-2.10 0/3] block: Add errp to b{lk, drv}_turncate()

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20170306195255.15806-1-mreitz@redhat.com
Subject: [Qemu-devel] [PATCH for-2.10 0/3] block: Add errp to b{lk, drv}_turncate()
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/1488826849-32384-1-git-send-email-armbru@redhat.com -> patchew/1488826849-32384-1-git-send-email-armbru@redhat.com
 * [new tag]         patchew/20170306195255.15806-1-mreitz@redhat.com -> patchew/20170306195255.15806-1-mreitz@redhat.com
Auto packing the repository for optimum performance. You may also
run "git gc" manually. See "git help gc" for more information.
Switched to a new branch 'test'
4ef1757 block: Add some bdrv_truncate() error messages
3e348c6 block: Add errp to BD.bdrv_truncate()
1ba2c64 block: Add errp to b{lk, drv}_truncate()

=== OUTPUT BEGIN ===
Checking PATCH 1/3: block: Add errp to b{lk, drv}_truncate()...
Checking PATCH 2/3: block: Add errp to BD.bdrv_truncate()...
Checking PATCH 3/3: block: Add some bdrv_truncate() error messages...
ERROR: suspect code indent for conditional statements (7, 11)
#35: FILE: block/file-posix.c:1358:
        if (offset > raw_getlength(bs)) {
+           error_setg(errp, "Cannot grow device files");

total: 1 errors, 0 warnings, 73 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Eric Blake]

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

On 03/06/2017 01:54 PM, Max Reitz wrote:
> Add missing error messages for the drivers I am comfortable to do this
> in.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/file-posix.c | 8 +++++++-
>  block/qcow2.c      | 2 ++
>  block/qed.c        | 4 +++-
>  block/raw-format.c | 2 ++
>  4 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 9d2bea730d..553213221c 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -1341,20 +1341,26 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
>  {
>      BDRVRawState *s = bs->opaque;
>      struct stat st;
> +    int ret;
>  
>      if (fstat(s->fd, &st)) {
> -        return -errno;
> +        ret = -errno;
> +        error_setg_errno(errp, -ret, "Failed to fstat() the file");
> +        return ret;
>      }
>  
>      if (S_ISREG(st.st_mode)) {
>          if (ftruncate(s->fd, offset) < 0) {
> +            /* The generic error message will be fine */
>              return -errno;

Relying on a generic error message in the caller is awkward. I see it as
evidence of a partial conversion if we have an interface that requires a
return of a negative errno value to make up for when errp is not set.  I
know you aren't comfortable converting all drivers, but for the drivers
you do convert, I'd rather guarantee that ALL errors set errp.

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

Re: [Qemu-devel] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Max Reitz]

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

On 06.03.2017 21:09, Eric Blake wrote:
> On 03/06/2017 01:54 PM, Max Reitz wrote:
>> Add missing error messages for the drivers I am comfortable to do this
>> in.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>  block/file-posix.c | 8 +++++++-
>>  block/qcow2.c      | 2 ++
>>  block/qed.c        | 4 +++-
>>  block/raw-format.c | 2 ++
>>  4 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/file-posix.c b/block/file-posix.c
>> index 9d2bea730d..553213221c 100644
>> --- a/block/file-posix.c
>> +++ b/block/file-posix.c
>> @@ -1341,20 +1341,26 @@ static int raw_truncate(BlockDriverState *bs, int64_t offset, Error **errp)
>>  {
>>      BDRVRawState *s = bs->opaque;
>>      struct stat st;
>> +    int ret;
>>  
>>      if (fstat(s->fd, &st)) {
>> -        return -errno;
>> +        ret = -errno;
>> +        error_setg_errno(errp, -ret, "Failed to fstat() the file");
>> +        return ret;
>>      }
>>  
>>      if (S_ISREG(st.st_mode)) {
>>          if (ftruncate(s->fd, offset) < 0) {
>> +            /* The generic error message will be fine */
>>              return -errno;
> 
> Relying on a generic error message in the caller is awkward. I see it as
> evidence of a partial conversion if we have an interface that requires a
> return of a negative errno value to make up for when errp is not set.

I'm not sure what you mean by this exactly. I can agree that it's not
nice within a single driver. But I think it's fine to have some drivers
that generate an Error object and others that do not, as long as the
generic interface (bdrv_truncate()) masks this behavior.

>                                                                        I
> know you aren't comfortable converting all drivers, but for the drivers
> you do convert, I'd rather guarantee that ALL errors set errp.

Can do, I just felt bad that it be would exactly the same as the message
that would be generated in bdrv_truncate() anyway.

Max


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

Re: [Qemu-devel] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Eric Blake]

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

On 03/06/2017 02:14 PM, Max Reitz wrote:

>>>      if (S_ISREG(st.st_mode)) {
>>>          if (ftruncate(s->fd, offset) < 0) {
>>> +            /* The generic error message will be fine */
>>>              return -errno;
>>
>> Relying on a generic error message in the caller is awkward. I see it as
>> evidence of a partial conversion if we have an interface that requires a
>> return of a negative errno value to make up for when errp is not set.
> 
> I'm not sure what you mean by this exactly.

The ideal conversion would be having .bdrv_truncate switch to a void
return; then no caller is messing with negative errno values (especially
since some of them are just synthesizing errno values, rather than
actually encountering one, and since some of them are probably using -1
when they should be using errno). But such a conversion requires that
all drivers be updated to properly set errp.


> I can agree that it's not
> nice within a single driver. But I think it's fine to have some drivers
> that generate an Error object and others that do not, as long as the
> generic interface (bdrv_truncate()) masks this behavior.

I agree that as an interim thing, having some drivers that aren't
converted yet is the best way to make progress. But it's still best if
every driver is switched on an all-or-none basis, so that we don't
forget to go back and re-fix drivers that half-heartedly set errp if we
eventually reach the point where all other drivers have been fixed.

In other words, I view the generic bdrv_truncate() supplying an error
based on negative errno is a crutch, and not something that we should
rely on, at least not for the drivers that we fix to set errp directly.

> 
>>                                                                        I
>> know you aren't comfortable converting all drivers, but for the drivers
>> you do convert, I'd rather guarantee that ALL errors set errp.
> 
> Can do, I just felt bad that it be would exactly the same as the message
> that would be generated in bdrv_truncate() anyway.

There may be some duplication of error messages, but it still seems
cleaner to consistently set the error within the driver than to rely on
the generic crutch.

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

Re: [Qemu-devel] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Max Reitz]

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

On 06.03.2017 21:21, Eric Blake wrote:
> On 03/06/2017 02:14 PM, Max Reitz wrote:
> 
>>>>      if (S_ISREG(st.st_mode)) {
>>>>          if (ftruncate(s->fd, offset) < 0) {
>>>> +            /* The generic error message will be fine */
>>>>              return -errno;
>>>
>>> Relying on a generic error message in the caller is awkward. I see it as
>>> evidence of a partial conversion if we have an interface that requires a
>>> return of a negative errno value to make up for when errp is not set.
>>
>> I'm not sure what you mean by this exactly.
> 
> The ideal conversion would be having .bdrv_truncate switch to a void
> return; then no caller is messing with negative errno values (especially
> since some of them are just synthesizing errno values, rather than
> actually encountering one, and since some of them are probably using -1
> when they should be using errno). But such a conversion requires that
> all drivers be updated to properly set errp.

Not sure if that is an ideal conversion. I very much prefer negative
return value + error object because that allows constructs like

if (foo(..., errp) < 0) {
    return;
}

Or:

ret = foo(..., errp);
if (ret < 0) {
    return ret;
}

Instead of:

foo(..., &local_err);
if (local_err) {
    error_propagate(errp, local_err);
    return;
}


For me, the most important thing is that errp will always be set if the
function fails.

(Of course, I'd be just fine with a boolean return value, too.)

>> I can agree that it's not
>> nice within a single driver. But I think it's fine to have some drivers
>> that generate an Error object and others that do not, as long as the
>> generic interface (bdrv_truncate()) masks this behavior.
> 
> I agree that as an interim thing, having some drivers that aren't
> converted yet is the best way to make progress. But it's still best if
> every driver is switched on an all-or-none basis, so that we don't
> forget to go back and re-fix drivers that half-heartedly set errp if we
> eventually reach the point where all other drivers have been fixed.
> 
> In other words, I view the generic bdrv_truncate() supplying an error
> based on negative errno is a crutch, and not something that we should
> rely on, at least not for the drivers that we fix to set errp directly.

Well, the more detailed the error messages are the better, but I don't
see any real improvement over a generic message supplied by
bdrv_truncate() if that is good enough.

Of course, I can easily be convinced that the generic message is in fact
not good enough.

>>> know you aren't comfortable converting all drivers, but for the drivers
>>> you do convert, I'd rather guarantee that ALL errors set errp.
>>
>> Can do, I just felt bad that it be would exactly the same as the message
>> that would be generated in bdrv_truncate() anyway.
> 
> There may be some duplication of error messages, but it still seems
> cleaner to consistently set the error within the driver than to rely on
> the generic crutch.

As I said, I don't necessarily think it's a crutch. :-)

Max


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

Re: [Qemu-devel] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Eric Blake]

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

On 03/06/2017 02:48 PM, Max Reitz wrote:
> On 06.03.2017 21:21, Eric Blake wrote:
>> On 03/06/2017 02:14 PM, Max Reitz wrote:
>>
>>>>>      if (S_ISREG(st.st_mode)) {
>>>>>          if (ftruncate(s->fd, offset) < 0) {
>>>>> +            /* The generic error message will be fine */
>>>>>              return -errno;
>>>>
>>>> Relying on a generic error message in the caller is awkward. I see it as
>>>> evidence of a partial conversion if we have an interface that requires a
>>>> return of a negative errno value to make up for when errp is not set.
>>>
>>> I'm not sure what you mean by this exactly.
>>
>> The ideal conversion would be having .bdrv_truncate switch to a void
>> return; then no caller is messing with negative errno values (especially
>> since some of them are just synthesizing errno values, rather than
>> actually encountering one, and since some of them are probably using -1
>> when they should be using errno). But such a conversion requires that
>> all drivers be updated to properly set errp.
> 
> Not sure if that is an ideal conversion. I very much prefer negative
> return value + error object because that allows constructs like
> 
> if (foo(..., errp) < 0) {
>     return;
> }

Fair point - Markus has argued that we should convert functions from
void to easy-to-spot return sentinels for easier logic (less boilerplate
in creating a local error, checking it, and propagating it).

But the point remains that returning -1 is simpler to code than
returning negative errno, when some of the existing drivers are already
synthesizing errno.  And (temporarily) forcing a void return would make
it easy to spot who still needs conversion, even if we then go back to
returning int (but this time, with the simpler -1 for error, rather than
negative errno).

> For me, the most important thing is that errp will always be set if the
> function fails.

Yes, that's what you are guaranteed with a void return, and what we
would also have with a (sane) integer return.

> 
> (Of course, I'd be just fine with a boolean return value, too.)

boolean works too, except it's a little harder to remember when 'true'
means success, compared to other code where '0' is success and negative
is failure.

>> In other words, I view the generic bdrv_truncate() supplying an error
>> based on negative errno is a crutch, and not something that we should
>> rely on, at least not for the drivers that we fix to set errp directly.
> 
> Well, the more detailed the error messages are the better, but I don't
> see any real improvement over a generic message supplied by
> bdrv_truncate() if that is good enough.

Maybe Markus has some opinions, since error reporting is his area.

> 
> Of course, I can easily be convinced that the generic message is in fact
> not good enough.

Maybe it's hard to argue that from the quality-of-message standpoint,
but that's exactly what I'm arguing from the ease-of-maintenance
standpoint.  Relying on the generic message is harder to maintain.

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

Re: [Qemu-devel] [PATCH for-2.10 2/3] block: Add errp to BD.bdrv_truncate()

[Max Reitz]

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

On 06.03.2017 20:54, Max Reitz wrote:
> Add an Error parameter to the block drivers' bdrv_truncate() interface.
> If a block driver does not set this in case of an error, the generic
> bdrv_truncate() implementation will do so.
> 
> Where it is obvious, this patch also makes some block drivers set this
> value.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  include/block/block_int.h |  2 +-
>  block.c                   |  4 ++--
>  block/archipelago.c       |  3 ++-
>  block/blkdebug.c          |  4 ++--
>  block/crypto.c            |  5 +++--
>  block/file-posix.c        |  2 +-
>  block/file-win32.c        |  6 +++---
>  block/gluster.c           |  3 ++-
>  block/iscsi.c             |  4 ++--
>  block/nfs.c               |  2 +-
>  block/qcow2.c             |  8 ++++----
>  block/qed.c               |  2 +-
>  block/raw-format.c        |  4 ++--
>  block/rbd.c               |  2 +-
>  block/sheepdog.c          | 14 ++++++--------
>  15 files changed, 33 insertions(+), 32 deletions(-)

[...]


> diff --git a/block/archipelago.c b/block/archipelago.c
> index 2449cfc702..0015178381 100644
> --- a/block/archipelago.c
> +++ b/block/archipelago.c
> @@ -976,7 +976,8 @@ static int64_t qemu_archipelago_getlength(BlockDriverState *bs)
>      return archipelago_volume_info(s);
>  }
>  
> -static int qemu_archipelago_truncate(BlockDriverState *bs, int64_t offset)
> +static int qemu_archipelago_truncate(BlockDriverState *bs, int64_t offset,
> +                                     Error *errp)

This should be **errp, of course. Will fix. :-/

Max

>  {
>      int ret, targetlen;
>      struct xseg_request *req;


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

Re: [Qemu-devel] [PATCH for-2.10 0/3] block: Add errp to b{lk, drv}_turncate()

[Max Reitz]

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

On 06.03.2017 20:58, no-reply@patchew.org wrote:
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 
> Message-id: 20170306195255.15806-1-mreitz@redhat.com
> Subject: [Qemu-devel] [PATCH for-2.10 0/3] block: Add errp to b{lk, drv}_turncate()
> Type: series
> 
> === TEST SCRIPT BEGIN ===
> #!/bin/bash
> 
> BASE=base
> n=1
> total=$(git log --oneline $BASE.. | wc -l)
> failed=0
> 
> # Useful git options
> git config --local diff.renamelimit 0
> git config --local diff.renames True
> 
> commits="$(git log --format=%H --reverse $BASE..)"
> for c in $commits; do
>     echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
>     if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
>         failed=1
>         echo
>     fi
>     n=$((n+1))
> done
> 
> exit $failed
> === TEST SCRIPT END ===
> 
> Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
> From https://github.com/patchew-project/qemu
>  - [tag update]      patchew/1488826849-32384-1-git-send-email-armbru@redhat.com -> patchew/1488826849-32384-1-git-send-email-armbru@redhat.com
>  * [new tag]         patchew/20170306195255.15806-1-mreitz@redhat.com -> patchew/20170306195255.15806-1-mreitz@redhat.com
> Auto packing the repository for optimum performance. You may also
> run "git gc" manually. See "git help gc" for more information.
> Switched to a new branch 'test'
> 4ef1757 block: Add some bdrv_truncate() error messages
> 3e348c6 block: Add errp to BD.bdrv_truncate()
> 1ba2c64 block: Add errp to b{lk, drv}_truncate()
> 
> === OUTPUT BEGIN ===
> Checking PATCH 1/3: block: Add errp to b{lk, drv}_truncate()...
> Checking PATCH 2/3: block: Add errp to BD.bdrv_truncate()...
> Checking PATCH 3/3: block: Add some bdrv_truncate() error messages...
> ERROR: suspect code indent for conditional statements (7, 11)
> #35: FILE: block/file-posix.c:1358:
>         if (offset > raw_getlength(bs)) {
> +           error_setg(errp, "Cannot grow device files");
> 
> total: 1 errors, 0 warnings, 73 lines checked

Surrounding conditional block is not correctly aligned; pre-existing,
didn't notice. Will fix in v2.

Max

> 
> Your patch has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 
> === OUTPUT END ===
> 
> Test command exited with code: 1
> 
> 
> ---
> Email generated automatically by Patchew [http://patchew.org/].
> Please send your feedback to patchew-devel@freelists.org
> 



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

Re: [Qemu-devel] [PATCH for-2.10 3/3] block: Add some bdrv_truncate() error messages

[Kevin Wolf]

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

Am 06.03.2017 um 21:53 hat Eric Blake geschrieben:
> On 03/06/2017 02:48 PM, Max Reitz wrote:
> > On 06.03.2017 21:21, Eric Blake wrote:
> >> On 03/06/2017 02:14 PM, Max Reitz wrote:
> >>
> >>>>>      if (S_ISREG(st.st_mode)) {
> >>>>>          if (ftruncate(s->fd, offset) < 0) {
> >>>>> +            /* The generic error message will be fine */
> >>>>>              return -errno;
> >>>>
> >>>> Relying on a generic error message in the caller is awkward. I see it as
> >>>> evidence of a partial conversion if we have an interface that requires a
> >>>> return of a negative errno value to make up for when errp is not set.
> >>>
> >>> I'm not sure what you mean by this exactly.
> >>
> >> The ideal conversion would be having .bdrv_truncate switch to a void
> >> return; then no caller is messing with negative errno values (especially
> >> since some of them are just synthesizing errno values, rather than
> >> actually encountering one, and since some of them are probably using -1
> >> when they should be using errno). But such a conversion requires that
> >> all drivers be updated to properly set errp.
> > 
> > Not sure if that is an ideal conversion. I very much prefer negative
> > return value + error object because that allows constructs like
> > 
> > if (foo(..., errp) < 0) {
> >     return;
> > }
> 
> Fair point - Markus has argued that we should convert functions from
> void to easy-to-spot return sentinels for easier logic (less boilerplate
> in creating a local error, checking it, and propagating it).
> 
> But the point remains that returning -1 is simpler to code than
> returning negative errno, when some of the existing drivers are already
> synthesizing errno.  And (temporarily) forcing a void return would make
> it easy to spot who still needs conversion, even if we then go back to
> returning int (but this time, with the simpler -1 for error, rather than
> negative errno).

Note that bdrv_truncate() is a bit special because it is called in some
paths that only have a -errno return and no Error** parameter, like
bdrv_co_pwritev() implementations, and I don't think we intend to
convert those to Error**.

Sharing some code between bdrv_truncate() and write after EOF is
something that I'd like to have anyway. Some of the things that
bdrv_truncate() does are missing in bdrv_aligned_pwritev(), and in the
age of -blockdev where you can use any node for anything, this is a bug.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH for-2.10 1/3] block: Add errp to b{lk, drv}_truncate()

[Kevin Wolf]

Am 06.03.2017 um 20:54 hat Max Reitz geschrieben:
> For one thing, this allows us to drop the error message generation from
> qemu-img.c and blockdev.c and instead have it unified in
> bdrv_truncate().
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

>  block/commit.c                 |  5 +++--
>  block/mirror.c                 |  2 +-

These still pass NULL after the series. Fixing it would require to add a
way to complete jobs with an Error object, but maybe we should do this
sooner or later anyway. Not necessarily part of this series, though.

>  block/vhdx.c                   |  6 +++---
>  block/vpc.c                    |  2 +-

vpc can easily be fixed to pass the actual errp from vpc_create() to the
blk_truncate() call. In vhdx.c, the blk_truncate() call is a bit more
deeply nested, but it doesn't seem completely unreasonable there either.

> --- a/qemu-io-cmds.c
> +++ b/qemu-io-cmds.c
> @@ -1569,7 +1569,7 @@ static int truncate_f(BlockBackend *blk, int argc, char **argv)
>          return 0;
>      }
>  
> -    ret = blk_truncate(blk, offset);
> +    ret = blk_truncate(blk, offset, NULL);
>      if (ret < 0) {
>          printf("truncate: %s\n", strerror(-ret));
>          return 0;

Come on, using NULL immediately before a printf()? Doing the right thing
here is trivial.

Kevin

Re: [Qemu-devel] [PATCH for-2.10 1/3] block: Add errp to b{lk, drv}_truncate()

[Max Reitz]

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

On 07.03.2017 11:47, Kevin Wolf wrote:
> Am 06.03.2017 um 20:54 hat Max Reitz geschrieben:
>> For one thing, this allows us to drop the error message generation from
>> qemu-img.c and blockdev.c and instead have it unified in
>> bdrv_truncate().
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
>>  block/commit.c                 |  5 +++--
>>  block/mirror.c                 |  2 +-
> 
> These still pass NULL after the series. Fixing it would require to add a
> way to complete jobs with an Error object, but maybe we should do this
> sooner or later anyway. Not necessarily part of this series, though.
> 
>>  block/vhdx.c                   |  6 +++---
>>  block/vpc.c                    |  2 +-
> 
> vpc can easily be fixed to pass the actual errp from vpc_create() to the
> blk_truncate() call. In vhdx.c, the blk_truncate() call is a bit more
> deeply nested, but it doesn't seem completely unreasonable there either.

I'll look into it, thanks.

>> --- a/qemu-io-cmds.c
>> +++ b/qemu-io-cmds.c
>> @@ -1569,7 +1569,7 @@ static int truncate_f(BlockBackend *blk, int argc, char **argv)
>>          return 0;
>>      }
>>  
>> -    ret = blk_truncate(blk, offset);
>> +    ret = blk_truncate(blk, offset, NULL);
>>      if (ret < 0) {
>>          printf("truncate: %s\n", strerror(-ret));
>>          return 0;
> 
> Come on, using NULL immediately before a printf()? Doing the right thing
> here is trivial.

Yeah, but it's also qemu-io, so I somehow felt both "Better not touch it
because it may break tests" and "Nobody really cares about a nice
message here anyway".

Will fix, since apparently at least you do care. ;-)

Max


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

Re: [Qemu-devel] [PATCH for-2.10 1/3] block: Add errp to b{lk, drv}_truncate()

[Max Reitz]

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

On 07.03.2017 11:47, Kevin Wolf wrote:
> Am 06.03.2017 um 20:54 hat Max Reitz geschrieben:
>> For one thing, this allows us to drop the error message generation from
>> qemu-img.c and blockdev.c and instead have it unified in
>> bdrv_truncate().
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> 
>>  block/commit.c                 |  5 +++--
>>  block/mirror.c                 |  2 +-
> 
> These still pass NULL after the series. Fixing it would require to add a
> way to complete jobs with an Error object, but maybe we should do this
> sooner or later anyway. Not necessarily part of this series, though.
> 
>>  block/vhdx.c                   |  6 +++---
>>  block/vpc.c                    |  2 +-
> 
> vpc can easily be fixed to pass the actual errp from vpc_create() to the
> blk_truncate() call. In vhdx.c, the blk_truncate() call is a bit more
> deeply nested, but it doesn't seem completely unreasonable there either.

vhdx is insofar not completely unreasonable because it already sometimes
generates Error objects and sometimes doesn't; so if I were to expand on
that inconsistent behavior, I wouldn't make matters worse. But still I
don't feel quite comfortable touching that.

Let's see, maybe I prepend a patch that makes vhdx_create() always
generate Error objects.

Max


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