qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [Qemu-devel] [RFC v3 0/6] block: Error parameter for opening/creating images
@ 2013-09-06 15:52 Max Reitz
  2013-09-06 15:52 ` [Qemu-devel] [RFC v3 1/6] bdrv: Use "Error" for opening images Max Reitz
                   ` (6 more replies)
  0 siblings, 7 replies; 8+ messages in thread
From: Max Reitz @ 2013-09-06 15:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz

This RFC adds an Error ** parameter to bdrv_open, bdrv_file_open,
bdrv_create and the respective functions provided by a block driver.

This results in more specific error information than just -errno provided
to the user when opening or creating images (disregarding the fact that
block drivers often already use error_report, which is generally changed
to error_setg through this patch).

The sixth patch in this series changes the qcow2 block driver to set an
example of usage in a block driver.

Note that several I/O tests break by applying this RFC since they expect
different error messages (generally, previously, an error message on
image opening/creation consisted of two lines; the first of which would be
generated by the driver through error_report, the second by the block
layer itself through strerror(-ret); this patch is designed to merge these
two lines into a single one). This applies to the tests 49, 51, 54 and 60.

v3:
 - merged old patch 6 into 3/4
 - extracted new patch 5 from 3 (independent change)
 - (=> old patch 5 is new patch 6)
 - fixed (some?) mistakes
   - wrong function calls due to the newly added parameters (some remained
     unchanged in v2, others were changed too early)
   - removed unintended changes in new patch 6
   - added neglected qerror_report_err in old patch 6 (now in patch 3)
   - took care of NULL filename in patch 3
 - retained some old error messages in patch 4

v2:
 - included all block drivers
 - split open/create patches (old 1 into 1/2 and old 2 into 3/4)
 - added a patch (6) for printing out error messages from blk_open,
   blk_file_open and blk_create in all block drivers (except qcow2, for
   which there is a dedicated patch (5))
   I intentionally made this an own commit since I think this to be
   real functionality, whereas the patches 3 and 4 are more for
   introducing Error into block.c and making sure everything compiles
   with these changes (without actually adding functionality to any
   other code). However, I'm well aware that this in some way breaks
   the code between patches 3/4 and 6, since some error messages may be
   discarded between these commits. Therefore I wouldn't object too much
   to merging patch 6 with 3/4.

Max Reitz (6):
  bdrv: Use "Error" for opening images
  bdrv: Use "Error" for creating images
  block: Error parameter for open functions
  block: Error parameter for create functions
  qemu-img create: Emit filename on error
  qcow2: Use Error parameter

 block.c                   | 176 +++++++++++++++++++++++++++++++---------------
 block/blkdebug.c          |   7 +-
 block/blkverify.c         |  11 ++-
 block/bochs.c             |   3 +-
 block/cloop.c             |   3 +-
 block/cow.c               |  15 ++--
 block/curl.c              |   3 +-
 block/dmg.c               |   3 +-
 block/gluster.c           |   4 +-
 block/iscsi.c             |   8 ++-
 block/mirror.c            |   5 +-
 block/nbd.c               |   3 +-
 block/parallels.c         |   3 +-
 block/qcow.c              |  15 ++--
 block/qcow2.c             | 142 ++++++++++++++++++++++++-------------
 block/qed.c               |  18 +++--
 block/raw-posix.c         |  18 +++--
 block/raw-win32.c         |   9 ++-
 block/raw_bsd.c           |  16 ++++-
 block/rbd.c               |   6 +-
 block/sheepdog.c          |  16 +++--
 block/snapshot.c          |   2 +-
 block/ssh.c               |   6 +-
 block/vdi.c               |   6 +-
 block/vhdx.c              |   3 +-
 block/vmdk.c              |  17 +++--
 block/vpc.c               |   6 +-
 block/vvfat.c             |  13 +++-
 blockdev.c                |  30 ++++----
 hw/block/xen_disk.c       |   7 +-
 include/block/block.h     |  11 +--
 include/block/block_int.h |   9 ++-
 qemu-img.c                |  39 +++++-----
 qemu-io.c                 |  14 ++--
 qemu-nbd.c                |   6 +-
 35 files changed, 431 insertions(+), 222 deletions(-)

-- 
1.8.4

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Qemu-devel] [RFC v3 1/6] bdrv: Use "Error" for opening images
  2013-09-06 15:52 [Qemu-devel] [RFC v3 0/6] block: Error parameter for opening/creating images Max Reitz
@ 2013-09-06 15:52 ` Max Reitz
  2013-09-06 15:52 ` [Qemu-devel] [RFC v3 2/6] bdrv: Use "Error" for creating images Max Reitz
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2013-09-06 15:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz

Add an Error ** parameter to BlockDriver.bdrv_open and
BlockDriver.bdrv_file_open to allow more specific error messages.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   |  4 ++--
 block/blkdebug.c          |  3 ++-
 block/blkverify.c         |  3 ++-
 block/bochs.c             |  3 ++-
 block/cloop.c             |  3 ++-
 block/cow.c               |  3 ++-
 block/curl.c              |  3 ++-
 block/dmg.c               |  3 ++-
 block/gluster.c           |  2 +-
 block/iscsi.c             |  5 +++--
 block/nbd.c               |  3 ++-
 block/parallels.c         |  3 ++-
 block/qcow.c              |  3 ++-
 block/qcow2.c             |  5 +++--
 block/qed.c               |  5 +++--
 block/raw-posix.c         | 12 ++++++++----
 block/raw-win32.c         |  6 ++++--
 block/raw_bsd.c           |  3 ++-
 block/rbd.c               |  3 ++-
 block/sheepdog.c          |  3 ++-
 block/snapshot.c          |  2 +-
 block/ssh.c               |  3 ++-
 block/vdi.c               |  3 ++-
 block/vhdx.c              |  3 ++-
 block/vmdk.c              |  3 ++-
 block/vpc.c               |  3 ++-
 block/vvfat.c             |  3 ++-
 include/block/block_int.h |  6 ++++--
 28 files changed, 67 insertions(+), 37 deletions(-)

diff --git a/block.c b/block.c
index 26639e8..c9cb371 100644
--- a/block.c
+++ b/block.c
@@ -734,7 +734,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     if (drv->bdrv_file_open) {
         assert(file == NULL);
         assert(drv->bdrv_parse_filename || filename != NULL);
-        ret = drv->bdrv_file_open(bs, options, open_flags);
+        ret = drv->bdrv_file_open(bs, options, open_flags, NULL);
     } else {
         if (file == NULL) {
             qerror_report(ERROR_CLASS_GENERIC_ERROR, "Can't use '%s' as a "
@@ -744,7 +744,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
             goto free_and_fail;
         }
         bs->file = file;
-        ret = drv->bdrv_open(bs, options, open_flags);
+        ret = drv->bdrv_open(bs, options, open_flags, NULL);
     }
 
     if (ret < 0) {
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 5d33e03..52d65ff 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -350,7 +350,8 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags)
+static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
+                         Error **errp)
 {
     BDRVBlkdebugState *s = bs->opaque;
     QemuOpts *opts;
diff --git a/block/blkverify.c b/block/blkverify.c
index 1d58cc3..5d716bb 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -116,7 +116,8 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int blkverify_open(BlockDriverState *bs, QDict *options, int flags)
+static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
+                          Error **errp)
 {
     BDRVBlkverifyState *s = bs->opaque;
     QemuOpts *opts;
diff --git a/block/bochs.c b/block/bochs.c
index d7078c0..51d9a90 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -108,7 +108,8 @@ static int bochs_probe(const uint8_t *buf, int buf_size, const char *filename)
     return 0;
 }
 
-static int bochs_open(BlockDriverState *bs, QDict *options, int flags)
+static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
+                      Error **errp)
 {
     BDRVBochsState *s = bs->opaque;
     int i;
diff --git a/block/cloop.c b/block/cloop.c
index 6ea7cf4..b907023 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -53,7 +53,8 @@ static int cloop_probe(const uint8_t *buf, int buf_size, const char *filename)
     return 0;
 }
 
-static int cloop_open(BlockDriverState *bs, QDict *options, int flags)
+static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
+                      Error **errp)
 {
     BDRVCloopState *s = bs->opaque;
     uint32_t offsets_size, max_compressed_block_size = 1, i;
diff --git a/block/cow.c b/block/cow.c
index 1cc2e89..4961ca0 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -58,7 +58,8 @@ static int cow_probe(const uint8_t *buf, int buf_size, const char *filename)
         return 0;
 }
 
-static int cow_open(BlockDriverState *bs, QDict *options, int flags)
+static int cow_open(BlockDriverState *bs, QDict *options, int flags,
+                    Error **errp)
 {
     BDRVCowState *s = bs->opaque;
     struct cow_header_v2 cow_header;
diff --git a/block/curl.c b/block/curl.c
index ca2cedc..5a46f97 100644
--- a/block/curl.c
+++ b/block/curl.c
@@ -395,7 +395,8 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int curl_open(BlockDriverState *bs, QDict *options, int flags)
+static int curl_open(BlockDriverState *bs, QDict *options, int flags,
+                     Error **errp)
 {
     BDRVCURLState *s = bs->opaque;
     CURLState *state = NULL;
diff --git a/block/dmg.c b/block/dmg.c
index 3141cb5..d5e9b1f 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -92,7 +92,8 @@ static int read_uint32(BlockDriverState *bs, int64_t offset, uint32_t *result)
     return 0;
 }
 
-static int dmg_open(BlockDriverState *bs, QDict *options, int flags)
+static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
+                    Error **errp)
 {
     BDRVDMGState *s = bs->opaque;
     uint64_t info_begin,info_end,last_in_offset,last_out_offset;
diff --git a/block/gluster.c b/block/gluster.c
index dbb03f4..6c7b000 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -288,7 +288,7 @@ static QemuOptsList runtime_opts = {
 };
 
 static int qemu_gluster_open(BlockDriverState *bs,  QDict *options,
-                             int bdrv_flags)
+                             int bdrv_flags, Error **errp)
 {
     BDRVGlusterState *s = bs->opaque;
     int open_flags = O_BINARY;
diff --git a/block/iscsi.c b/block/iscsi.c
index 2bbee1f..fd34752 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1046,7 +1046,8 @@ static QemuOptsList runtime_opts = {
  * We support iscsi url's on the form
  * iscsi://[<username>%<password>@]<host>[:<port>]/<targetname>/<lun>
  */
-static int iscsi_open(BlockDriverState *bs, QDict *options, int flags)
+static int iscsi_open(BlockDriverState *bs, QDict *options, int flags,
+                      Error **errp)
 {
     IscsiLun *iscsilun = bs->opaque;
     struct iscsi_context *iscsi = NULL;
@@ -1260,7 +1261,7 @@ static int iscsi_create(const char *filename, QEMUOptionParameter *options)
 
     bs_options = qdict_new();
     qdict_put(bs_options, "filename", qstring_from_str(filename));
-    ret = iscsi_open(&bs, bs_options, 0);
+    ret = iscsi_open(&bs, bs_options, 0, NULL);
     QDECREF(bs_options);
 
     if (ret != 0) {
diff --git a/block/nbd.c b/block/nbd.c
index 691066f..c8deeee 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -453,7 +453,8 @@ static void nbd_teardown_connection(BlockDriverState *bs)
     closesocket(s->sock);
 }
 
-static int nbd_open(BlockDriverState *bs, QDict *options, int flags)
+static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
+                    Error **errp)
 {
     BDRVNBDState *s = bs->opaque;
     int result;
diff --git a/block/parallels.c b/block/parallels.c
index 18b3ac0..2121e43 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -68,7 +68,8 @@ static int parallels_probe(const uint8_t *buf, int buf_size, const char *filenam
     return 0;
 }
 
-static int parallels_open(BlockDriverState *bs, QDict *options, int flags)
+static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
+                          Error **errp)
 {
     BDRVParallelsState *s = bs->opaque;
     int i;
diff --git a/block/qcow.c b/block/qcow.c
index 5239bd6..9ac9120 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -92,7 +92,8 @@ static int qcow_probe(const uint8_t *buf, int buf_size, const char *filename)
         return 0;
 }
 
-static int qcow_open(BlockDriverState *bs, QDict *options, int flags)
+static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
+                     Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     int len, i, shift, ret;
diff --git a/block/qcow2.c b/block/qcow2.c
index 4bc679a..33168b0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -350,7 +350,8 @@ static QemuOptsList qcow2_runtime_opts = {
     },
 };
 
-static int qcow2_open(BlockDriverState *bs, QDict *options, int flags)
+static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
+                      Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     int len, i, ret = 0;
@@ -1049,7 +1050,7 @@ static void qcow2_invalidate_cache(BlockDriverState *bs)
               qbool_from_int(s->use_lazy_refcounts));
 
     memset(s, 0, sizeof(BDRVQcowState));
-    qcow2_open(bs, options, flags);
+    qcow2_open(bs, options, flags, NULL);
 
     QDECREF(options);
 
diff --git a/block/qed.c b/block/qed.c
index cc904c4..927f30b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -373,7 +373,8 @@ static void bdrv_qed_rebind(BlockDriverState *bs)
     s->bs = bs;
 }
 
-static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags)
+static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
+                         Error **errp)
 {
     BDRVQEDState *s = bs->opaque;
     QEDHeader le_header;
@@ -1526,7 +1527,7 @@ static void bdrv_qed_invalidate_cache(BlockDriverState *bs)
 
     bdrv_qed_close(bs);
     memset(s, 0, sizeof(BDRVQEDState));
-    bdrv_qed_open(bs, NULL, bs->open_flags);
+    bdrv_qed_open(bs, NULL, bs->open_flags, NULL);
 }
 
 static int bdrv_qed_check(BlockDriverState *bs, BdrvCheckResult *result,
diff --git a/block/raw-posix.c b/block/raw-posix.c
index ba721d3..19068b1 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -335,7 +335,8 @@ fail:
     return ret;
 }
 
-static int raw_open(BlockDriverState *bs, QDict *options, int flags)
+static int raw_open(BlockDriverState *bs, QDict *options, int flags,
+                    Error **errp)
 {
     BDRVRawState *s = bs->opaque;
 
@@ -1325,7 +1326,8 @@ static int check_hdev_writable(BDRVRawState *s)
     return 0;
 }
 
-static int hdev_open(BlockDriverState *bs, QDict *options, int flags)
+static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
+                     Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
@@ -1559,7 +1561,8 @@ static BlockDriver bdrv_host_device = {
 };
 
 #ifdef __linux__
-static int floppy_open(BlockDriverState *bs, QDict *options, int flags)
+static int floppy_open(BlockDriverState *bs, QDict *options, int flags,
+                       Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     int ret;
@@ -1680,7 +1683,8 @@ static BlockDriver bdrv_host_floppy = {
     .bdrv_eject         = floppy_eject,
 };
 
-static int cdrom_open(BlockDriverState *bs, QDict *options, int flags)
+static int cdrom_open(BlockDriverState *bs, QDict *options, int flags,
+                      Error **errp)
 {
     BDRVRawState *s = bs->opaque;
 
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 9b5b2af..395c912 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -234,7 +234,8 @@ static QemuOptsList raw_runtime_opts = {
     },
 };
 
-static int raw_open(BlockDriverState *bs, QDict *options, int flags)
+static int raw_open(BlockDriverState *bs, QDict *options, int flags,
+                    Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     int access_flags;
@@ -531,7 +532,8 @@ static int hdev_probe_device(const char *filename)
     return 0;
 }
 
-static int hdev_open(BlockDriverState *bs, QDict *options, int flags)
+static int hdev_open(BlockDriverState *bs, QDict *options, int flags,
+                     Error **errp)
 {
     BDRVRawState *s = bs->opaque;
     int access_flags, create_flags;
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index ab2b0fd..f996011 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -135,7 +135,8 @@ static int raw_create(const char *filename, QEMUOptionParameter *options)
     return bdrv_create_file(filename, options);
 }
 
-static int raw_open(BlockDriverState *bs, QDict *options, int flags)
+static int raw_open(BlockDriverState *bs, QDict *options, int flags,
+                    Error **errp)
 {
     bs->sg = bs->file->sg;
     return 0;
diff --git a/block/rbd.c b/block/rbd.c
index e798e19..dc06117 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -446,7 +446,8 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags)
+static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags,
+                         Error **errp)
 {
     BDRVRBDState *s = bs->opaque;
     char pool[RBD_MAX_POOL_NAME_SIZE];
diff --git a/block/sheepdog.c b/block/sheepdog.c
index 1ad4d07..b4a86ea 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1242,7 +1242,8 @@ static QemuOptsList runtime_opts = {
     },
 };
 
-static int sd_open(BlockDriverState *bs, QDict *options, int flags)
+static int sd_open(BlockDriverState *bs, QDict *options, int flags,
+                   Error **errp)
 {
     int ret, fd;
     uint32_t vid = 0;
diff --git a/block/snapshot.c b/block/snapshot.c
index 6c6d9de..51b4b96 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -97,7 +97,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
     if (bs->file) {
         drv->bdrv_close(bs);
         ret = bdrv_snapshot_goto(bs->file, snapshot_id);
-        open_ret = drv->bdrv_open(bs, NULL, bs->open_flags);
+        open_ret = drv->bdrv_open(bs, NULL, bs->open_flags, NULL);
         if (open_ret < 0) {
             bdrv_delete(bs->file);
             bs->drv = NULL;
diff --git a/block/ssh.c b/block/ssh.c
index 27691b4..23cd1f0 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -608,7 +608,8 @@ static int connect_to_ssh(BDRVSSHState *s, QDict *options,
     return ret;
 }
 
-static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags)
+static int ssh_file_open(BlockDriverState *bs, QDict *options, int bdrv_flags,
+                         Error **errp)
 {
     BDRVSSHState *s = bs->opaque;
     int ret;
diff --git a/block/vdi.c b/block/vdi.c
index 8a91525..df0be42 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -364,7 +364,8 @@ static int vdi_probe(const uint8_t *buf, int buf_size, const char *filename)
     return result;
 }
 
-static int vdi_open(BlockDriverState *bs, QDict *options, int flags)
+static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
+                    Error **errp)
 {
     BDRVVdiState *s = bs->opaque;
     VdiHeader header;
diff --git a/block/vhdx.c b/block/vhdx.c
index e9704b1..b8aa49c 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -715,7 +715,8 @@ exit:
 }
 
 
-static int vhdx_open(BlockDriverState *bs, QDict *options, int flags)
+static int vhdx_open(BlockDriverState *bs, QDict *options, int flags,
+                     Error **errp)
 {
     BDRVVHDXState *s = bs->opaque;
     int ret = 0;
diff --git a/block/vmdk.c b/block/vmdk.c
index 63b489d..bf424b1 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -806,7 +806,8 @@ exit:
     return ret;
 }
 
-static int vmdk_open(BlockDriverState *bs, QDict *options, int flags)
+static int vmdk_open(BlockDriverState *bs, QDict *options, int flags,
+                     Error **errp)
 {
     int ret;
     BDRVVmdkState *s = bs->opaque;
diff --git a/block/vpc.c b/block/vpc.c
index fe4f311..ed00370 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -155,7 +155,8 @@ static int vpc_probe(const uint8_t *buf, int buf_size, const char *filename)
     return 0;
 }
 
-static int vpc_open(BlockDriverState *bs, QDict *options, int flags)
+static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
+                    Error **errp)
 {
     BDRVVPCState *s = bs->opaque;
     int i;
diff --git a/block/vvfat.c b/block/vvfat.c
index cd3b8ed..d10159d 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1065,7 +1065,8 @@ static void vvfat_parse_filename(const char *filename, QDict *options,
     qdict_put(options, "rw", qbool_from_int(rw));
 }
 
-static int vvfat_open(BlockDriverState *bs, QDict *options, int flags)
+static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
+                      Error **errp)
 {
     BDRVVVFATState *s = bs->opaque;
     int cyls, heads, secs;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8012e25..b05f850 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -97,8 +97,10 @@ struct BlockDriver {
     void (*bdrv_reopen_commit)(BDRVReopenState *reopen_state);
     void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
 
-    int (*bdrv_open)(BlockDriverState *bs, QDict *options, int flags);
-    int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags);
+    int (*bdrv_open)(BlockDriverState *bs, QDict *options, int flags,
+                     Error **errp);
+    int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
+                          Error **errp);
     int (*bdrv_read)(BlockDriverState *bs, int64_t sector_num,
                      uint8_t *buf, int nb_sectors);
     int (*bdrv_write)(BlockDriverState *bs, int64_t sector_num,
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [RFC v3 2/6] bdrv: Use "Error" for creating images
  2013-09-06 15:52 [Qemu-devel] [RFC v3 0/6] block: Error parameter for opening/creating images Max Reitz
  2013-09-06 15:52 ` [Qemu-devel] [RFC v3 1/6] bdrv: Use "Error" for opening images Max Reitz
@ 2013-09-06 15:52 ` Max Reitz
  2013-09-06 15:52 ` [Qemu-devel] [RFC v3 3/6] block: Error parameter for open functions Max Reitz
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2013-09-06 15:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz

Add an Error ** parameter to BlockDriver.bdrv_create to allow more
specific error messages.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c                   | 2 +-
 block/cow.c               | 3 ++-
 block/gluster.c           | 2 +-
 block/iscsi.c             | 3 ++-
 block/qcow.c              | 3 ++-
 block/qcow2.c             | 3 ++-
 block/qed.c               | 3 ++-
 block/raw-posix.c         | 6 ++++--
 block/raw-win32.c         | 3 ++-
 block/raw_bsd.c           | 3 ++-
 block/rbd.c               | 3 ++-
 block/sheepdog.c          | 3 ++-
 block/ssh.c               | 3 ++-
 block/vdi.c               | 3 ++-
 block/vmdk.c              | 3 ++-
 block/vpc.c               | 3 ++-
 include/block/block_int.h | 3 ++-
 17 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/block.c b/block.c
index c9cb371..f485906 100644
--- a/block.c
+++ b/block.c
@@ -374,7 +374,7 @@ static void coroutine_fn bdrv_create_co_entry(void *opaque)
     CreateCo *cco = opaque;
     assert(cco->drv);
 
-    cco->ret = cco->drv->bdrv_create(cco->filename, cco->options);
+    cco->ret = cco->drv->bdrv_create(cco->filename, cco->options, NULL);
 }
 
 int bdrv_create(BlockDriver *drv, const char* filename,
diff --git a/block/cow.c b/block/cow.c
index 4961ca0..207fea1 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -256,7 +256,8 @@ static void cow_close(BlockDriverState *bs)
 {
 }
 
-static int cow_create(const char *filename, QEMUOptionParameter *options)
+static int cow_create(const char *filename, QEMUOptionParameter *options,
+                      Error **errp)
 {
     struct cow_header_v2 cow_header;
     struct stat st;
diff --git a/block/gluster.c b/block/gluster.c
index 6c7b000..256de10 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -357,7 +357,7 @@ out:
 }
 
 static int qemu_gluster_create(const char *filename,
-        QEMUOptionParameter *options)
+        QEMUOptionParameter *options, Error **errp)
 {
     struct glfs *glfs;
     struct glfs_fd *fd;
diff --git a/block/iscsi.c b/block/iscsi.c
index fd34752..c0c116d 100644
--- a/block/iscsi.c
+++ b/block/iscsi.c
@@ -1238,7 +1238,8 @@ static int iscsi_has_zero_init(BlockDriverState *bs)
     return 0;
 }
 
-static int iscsi_create(const char *filename, QEMUOptionParameter *options)
+static int iscsi_create(const char *filename, QEMUOptionParameter *options,
+                        Error **errp)
 {
     int ret = 0;
     int64_t total_size = 0;
diff --git a/block/qcow.c b/block/qcow.c
index 9ac9120..6f7e73d 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -652,7 +652,8 @@ static void qcow_close(BlockDriverState *bs)
     error_free(s->migration_blocker);
 }
 
-static int qcow_create(const char *filename, QEMUOptionParameter *options)
+static int qcow_create(const char *filename, QEMUOptionParameter *options,
+                       Error **errp)
 {
     int header_size, backing_filename_len, l1_size, shift, i;
     QCowHeader header;
diff --git a/block/qcow2.c b/block/qcow2.c
index 33168b0..98d1551 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1457,7 +1457,8 @@ out:
     return ret;
 }
 
-static int qcow2_create(const char *filename, QEMUOptionParameter *options)
+static int qcow2_create(const char *filename, QEMUOptionParameter *options,
+                        Error **errp)
 {
     const char *backing_file = NULL;
     const char *backing_fmt = NULL;
diff --git a/block/qed.c b/block/qed.c
index 927f30b..1a9d01b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -604,7 +604,8 @@ out:
     return ret;
 }
 
-static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options)
+static int bdrv_qed_create(const char *filename, QEMUOptionParameter *options,
+                           Error **errp)
 {
     uint64_t image_size = 0;
     uint32_t cluster_size = QED_DEFAULT_CLUSTER_SIZE;
diff --git a/block/raw-posix.c b/block/raw-posix.c
index 19068b1..22c8303 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1041,7 +1041,8 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs)
     return (int64_t)st.st_blocks * 512;
 }
 
-static int raw_create(const char *filename, QEMUOptionParameter *options)
+static int raw_create(const char *filename, QEMUOptionParameter *options,
+                      Error **errp)
 {
     int fd;
     int result = 0;
@@ -1500,7 +1501,8 @@ static coroutine_fn BlockDriverAIOCB *hdev_aio_discard(BlockDriverState *bs,
                        cb, opaque, QEMU_AIO_DISCARD|QEMU_AIO_BLKDEV);
 }
 
-static int hdev_create(const char *filename, QEMUOptionParameter *options)
+static int hdev_create(const char *filename, QEMUOptionParameter *options,
+                       Error **errp)
 {
     int fd;
     int ret = 0;
diff --git a/block/raw-win32.c b/block/raw-win32.c
index 395c912..1b287bc 100644
--- a/block/raw-win32.c
+++ b/block/raw-win32.c
@@ -421,7 +421,8 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs)
     return st.st_size;
 }
 
-static int raw_create(const char *filename, QEMUOptionParameter *options)
+static int raw_create(const char *filename, QEMUOptionParameter *options,
+                      Error **errp)
 {
     int fd;
     int64_t total_size = 0;
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index f996011..793121a 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -130,7 +130,8 @@ static int raw_has_zero_init(BlockDriverState *bs)
     return bdrv_has_zero_init(bs->file);
 }
 
-static int raw_create(const char *filename, QEMUOptionParameter *options)
+static int raw_create(const char *filename, QEMUOptionParameter *options,
+                      Error **errp)
 {
     return bdrv_create_file(filename, options);
 }
diff --git a/block/rbd.c b/block/rbd.c
index dc06117..06cbef7 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -287,7 +287,8 @@ static int qemu_rbd_set_conf(rados_t cluster, const char *conf)
     return ret;
 }
 
-static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options)
+static int qemu_rbd_create(const char *filename, QEMUOptionParameter *options,
+                           Error **errp)
 {
     int64_t bytes = 0;
     int64_t objsize;
diff --git a/block/sheepdog.c b/block/sheepdog.c
index b4a86ea..b1a0f7a 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1438,7 +1438,8 @@ out:
     return ret;
 }
 
-static int sd_create(const char *filename, QEMUOptionParameter *options)
+static int sd_create(const char *filename, QEMUOptionParameter *options,
+                     Error **errp)
 {
     int ret = 0;
     uint32_t vid = 0, base_vid = 0;
diff --git a/block/ssh.c b/block/ssh.c
index 23cd1f0..aa63c9d 100644
--- a/block/ssh.c
+++ b/block/ssh.c
@@ -651,7 +651,8 @@ static QEMUOptionParameter ssh_create_options[] = {
     { NULL }
 };
 
-static int ssh_create(const char *filename, QEMUOptionParameter *options)
+static int ssh_create(const char *filename, QEMUOptionParameter *options,
+                      Error **errp)
 {
     int r, ret;
     Error *local_err = NULL;
diff --git a/block/vdi.c b/block/vdi.c
index df0be42..c22f94c 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -634,7 +634,8 @@ static int vdi_co_write(BlockDriverState *bs,
     return ret;
 }
 
-static int vdi_create(const char *filename, QEMUOptionParameter *options)
+static int vdi_create(const char *filename, QEMUOptionParameter *options,
+                      Error **errp)
 {
     int fd;
     int result = 0;
diff --git a/block/vmdk.c b/block/vmdk.c
index bf424b1..eb8bba2 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1535,7 +1535,8 @@ static int filename_decompose(const char *filename, char *path, char *prefix,
     return VMDK_OK;
 }
 
-static int vmdk_create(const char *filename, QEMUOptionParameter *options)
+static int vmdk_create(const char *filename, QEMUOptionParameter *options,
+                       Error **errp)
 {
     int fd, idx = 0;
     char desc[BUF_SIZE];
diff --git a/block/vpc.c b/block/vpc.c
index ed00370..db61274 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -684,7 +684,8 @@ static int create_fixed_disk(int fd, uint8_t *buf, int64_t total_size)
     return ret;
 }
 
-static int vpc_create(const char *filename, QEMUOptionParameter *options)
+static int vpc_create(const char *filename, QEMUOptionParameter *options,
+                      Error **errp)
 {
     uint8_t buf[1024];
     struct vhd_footer *footer = (struct vhd_footer *) buf;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index b05f850..84001b8 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -107,7 +107,8 @@ struct BlockDriver {
                       const uint8_t *buf, int nb_sectors);
     void (*bdrv_close)(BlockDriverState *bs);
     void (*bdrv_rebind)(BlockDriverState *bs);
-    int (*bdrv_create)(const char *filename, QEMUOptionParameter *options);
+    int (*bdrv_create)(const char *filename, QEMUOptionParameter *options,
+                       Error **errp);
     int (*bdrv_set_key)(BlockDriverState *bs, const char *key);
     int (*bdrv_make_empty)(BlockDriverState *bs);
     /* aio */
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [RFC v3 3/6] block: Error parameter for open functions
  2013-09-06 15:52 [Qemu-devel] [RFC v3 0/6] block: Error parameter for opening/creating images Max Reitz
  2013-09-06 15:52 ` [Qemu-devel] [RFC v3 1/6] bdrv: Use "Error" for opening images Max Reitz
  2013-09-06 15:52 ` [Qemu-devel] [RFC v3 2/6] bdrv: Use "Error" for creating images Max Reitz
@ 2013-09-06 15:52 ` Max Reitz
  2013-09-06 15:52 ` [Qemu-devel] [RFC v3 4/6] block: Error parameter for create functions Max Reitz
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2013-09-06 15:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz

Add an Error ** parameter to bdrv_open, bdrv_file_open and associated
functions to allow more specific error messages.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 100 ++++++++++++++++++++++++++++++++------------------
 block/blkdebug.c      |   4 +-
 block/blkverify.c     |   8 +++-
 block/cow.c           |   5 ++-
 block/mirror.c        |   5 ++-
 block/qcow.c          |   5 ++-
 block/qcow2.c         |   4 +-
 block/qed.c           |   6 ++-
 block/sheepdog.c      |  10 ++++-
 block/vmdk.c          |  11 +++++-
 block/vvfat.c         |   6 ++-
 blockdev.c            |  30 +++++++--------
 hw/block/xen_disk.c   |   7 +++-
 include/block/block.h |   6 +--
 qemu-img.c            |  21 +++++++----
 qemu-io.c             |  14 +++++--
 qemu-nbd.c            |   6 ++-
 17 files changed, 163 insertions(+), 85 deletions(-)

diff --git a/block.c b/block.c
index f485906..8b0a729 100644
--- a/block.c
+++ b/block.c
@@ -525,7 +525,7 @@ BlockDriver *bdrv_find_protocol(const char *filename,
 }
 
 static int find_image_format(BlockDriverState *bs, const char *filename,
-                             BlockDriver **pdrv)
+                             BlockDriver **pdrv, Error **errp)
 {
     int score, score_max;
     BlockDriver *drv1, *drv;
@@ -536,6 +536,7 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
     if (bs->sg || !bdrv_is_inserted(bs) || bdrv_getlength(bs) == 0) {
         drv = bdrv_find_format("raw");
         if (!drv) {
+            error_setg(errp, "Could not find raw image format");
             ret = -ENOENT;
         }
         *pdrv = drv;
@@ -544,6 +545,8 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
 
     ret = bdrv_pread(bs, 0, buf, sizeof(buf));
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not read image for determining its "
+                         "format");
         *pdrv = NULL;
         return ret;
     }
@@ -560,6 +563,8 @@ static int find_image_format(BlockDriverState *bs, const char *filename,
         }
     }
     if (!drv) {
+        error_setg(errp, "Could not determine image format: No compatible "
+                   "driver found");
         ret = -ENOENT;
     }
     *pdrv = drv;
@@ -679,10 +684,11 @@ static int bdrv_open_flags(BlockDriverState *bs, int flags)
  * Removes all processed options from *options.
  */
 static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
-    QDict *options, int flags, BlockDriver *drv)
+    QDict *options, int flags, BlockDriver *drv, Error **errp)
 {
     int ret, open_flags;
     const char *filename;
+    Error *local_err = NULL;
 
     assert(drv != NULL);
     assert(bs->file == NULL);
@@ -711,6 +717,7 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     bs->read_only = !(open_flags & BDRV_O_RDWR);
 
     if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {
+        error_setg(errp, "Driver '%s' is not whitelisted", drv->format_name);
         return -ENOTSUP;
     }
 
@@ -734,25 +741,32 @@ static int bdrv_open_common(BlockDriverState *bs, BlockDriverState *file,
     if (drv->bdrv_file_open) {
         assert(file == NULL);
         assert(drv->bdrv_parse_filename || filename != NULL);
-        ret = drv->bdrv_file_open(bs, options, open_flags, NULL);
+        ret = drv->bdrv_file_open(bs, options, open_flags, &local_err);
     } else {
         if (file == NULL) {
-            qerror_report(ERROR_CLASS_GENERIC_ERROR, "Can't use '%s' as a "
-                          "block driver for the protocol level",
-                          drv->format_name);
+            error_setg(errp, "Can't use '%s' as a block driver for the "
+                       "protocol level", drv->format_name);
             ret = -EINVAL;
             goto free_and_fail;
         }
         bs->file = file;
-        ret = drv->bdrv_open(bs, options, open_flags, NULL);
+        ret = drv->bdrv_open(bs, options, open_flags, &local_err);
     }
 
     if (ret < 0) {
+        if (error_is_set(&local_err)) {
+            error_propagate(errp, local_err);
+        } else if (filename) {
+            error_setg_errno(errp, -ret, "Could not open '%s'", filename);
+        } else {
+            error_setg_errno(errp, -ret, "Could not open image");
+        }
         goto free_and_fail;
     }
 
     ret = refresh_total_sectors(bs, bs->total_sectors);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not refresh total sector count");
         goto free_and_fail;
     }
 
@@ -781,12 +795,13 @@ free_and_fail:
  * dictionary, it needs to use QINCREF() before calling bdrv_file_open.
  */
 int bdrv_file_open(BlockDriverState **pbs, const char *filename,
-                   QDict *options, int flags)
+                   QDict *options, int flags, Error **errp)
 {
     BlockDriverState *bs;
     BlockDriver *drv;
     const char *drvname;
     bool allow_protocol_prefix = false;
+    Error *local_err = NULL;
     int ret;
 
     /* NULL means an empty set of options */
@@ -805,8 +820,8 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
         qdict_put(options, "filename", qstring_from_str(filename));
         allow_protocol_prefix = true;
     } else {
-        qerror_report(ERROR_CLASS_GENERIC_ERROR, "Can't specify 'file' and "
-                      "'filename' options at the same time");
+        error_setg(errp, "Can't specify 'file' and 'filename' options at the "
+                   "same time");
         ret = -EINVAL;
         goto fail;
     }
@@ -815,53 +830,53 @@ int bdrv_file_open(BlockDriverState **pbs, const char *filename,
     drvname = qdict_get_try_str(options, "driver");
     if (drvname) {
         drv = bdrv_find_whitelisted_format(drvname, !(flags & BDRV_O_RDWR));
+        if (!drv) {
+            error_setg(errp, "Unknown driver '%s'", drvname);
+        }
         qdict_del(options, "driver");
     } else if (filename) {
         drv = bdrv_find_protocol(filename, allow_protocol_prefix);
         if (!drv) {
-            qerror_report(ERROR_CLASS_GENERIC_ERROR, "Unknown protocol");
+            error_setg(errp, "Unknown protocol");
         }
     } else {
-        qerror_report(ERROR_CLASS_GENERIC_ERROR,
-                      "Must specify either driver or file");
+        error_setg(errp, "Must specify either driver or file");
         drv = NULL;
     }
 
     if (!drv) {
+        /* errp has been set already */
         ret = -ENOENT;
         goto fail;
     }
 
     /* Parse the filename and open it */
     if (drv->bdrv_parse_filename && filename) {
-        Error *local_err = NULL;
         drv->bdrv_parse_filename(filename, options, &local_err);
         if (error_is_set(&local_err)) {
-            qerror_report_err(local_err);
-            error_free(local_err);
+            error_propagate(errp, local_err);
             ret = -EINVAL;
             goto fail;
         }
         qdict_del(options, "filename");
     } else if (!drv->bdrv_parse_filename && !filename) {
-        qerror_report(ERROR_CLASS_GENERIC_ERROR,
-                      "The '%s' block driver requires a file name",
-                      drv->format_name);
+        error_setg(errp, "The '%s' block driver requires a file name",
+                   drv->format_name);
         ret = -EINVAL;
         goto fail;
     }
 
-    ret = bdrv_open_common(bs, NULL, options, flags, drv);
+    ret = bdrv_open_common(bs, NULL, options, flags, drv, &local_err);
     if (ret < 0) {
+        error_propagate(errp, local_err);
         goto fail;
     }
 
     /* Check if any unknown options were used */
     if (qdict_size(options) != 0) {
         const QDictEntry *entry = qdict_first(options);
-        qerror_report(ERROR_CLASS_GENERIC_ERROR, "Block protocol '%s' doesn't "
-                      "support the option '%s'",
-                      drv->format_name, entry->key);
+        error_setg(errp, "Block protocol '%s' doesn't support the option '%s'",
+                   drv->format_name, entry->key);
         ret = -EINVAL;
         goto fail;
     }
@@ -888,11 +903,12 @@ fail:
  * function (even on failure), so if the caller intends to reuse the dictionary,
  * it needs to use QINCREF() before calling bdrv_file_open.
  */
-int bdrv_open_backing_file(BlockDriverState *bs, QDict *options)
+int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp)
 {
     char backing_filename[PATH_MAX];
     int back_flags, ret;
     BlockDriver *back_drv = NULL;
+    Error *local_err = NULL;
 
     if (bs->backing_hd != NULL) {
         QDECREF(options);
@@ -925,11 +941,12 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *options)
 
     ret = bdrv_open(bs->backing_hd,
                     *backing_filename ? backing_filename : NULL, options,
-                    back_flags, back_drv);
+                    back_flags, back_drv, &local_err);
     if (ret < 0) {
         bdrv_delete(bs->backing_hd);
         bs->backing_hd = NULL;
         bs->open_flags |= BDRV_O_NO_BACKING;
+        error_propagate(errp, local_err);
         return ret;
     }
     return 0;
@@ -963,7 +980,7 @@ static void extract_subqdict(QDict *src, QDict **dst, const char *start)
  * dictionary, it needs to use QINCREF() before calling bdrv_open.
  */
 int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
-              int flags, BlockDriver *drv)
+              int flags, BlockDriver *drv, Error **errp)
 {
     int ret;
     /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
@@ -971,6 +988,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     BlockDriverState *file = NULL;
     QDict *file_options = NULL;
     const char *drvname;
+    Error *local_err = NULL;
 
     /* NULL means an empty set of options */
     if (options == NULL) {
@@ -989,7 +1007,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
         char backing_filename[PATH_MAX];
 
         if (qdict_size(options) != 0) {
-            error_report("Can't use snapshot=on with driver-specific options");
+            error_setg(errp, "Can't use snapshot=on with driver-specific options");
             ret = -EINVAL;
             goto fail;
         }
@@ -1000,7 +1018,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
 
         /* if there is a backing file, use it */
         bs1 = bdrv_new("");
-        ret = bdrv_open(bs1, filename, NULL, 0, drv);
+        ret = bdrv_open(bs1, filename, NULL, 0, drv, &local_err);
         if (ret < 0) {
             bdrv_delete(bs1);
             goto fail;
@@ -1011,6 +1029,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
 
         ret = get_tmp_filename(tmp_filename, sizeof(tmp_filename));
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Could not get temporary filename");
             goto fail;
         }
 
@@ -1019,6 +1038,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
             snprintf(backing_filename, sizeof(backing_filename),
                      "%s", filename);
         } else if (!realpath(filename, backing_filename)) {
+            error_setg_errno(errp, errno, "Could not resolve path '%s'", filename);
             ret = -errno;
             goto fail;
         }
@@ -1038,6 +1058,8 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
         ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options);
         free_option_parameters(create_options);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Could not create temporary overlay "
+                             "'%s'", tmp_filename);
             goto fail;
         }
 
@@ -1054,7 +1076,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     extract_subqdict(options, &file_options, "file.");
 
     ret = bdrv_file_open(&file, filename, file_options,
-                         bdrv_open_flags(bs, flags | BDRV_O_UNMAP));
+                         bdrv_open_flags(bs, flags | BDRV_O_UNMAP), &local_err);
     if (ret < 0) {
         goto fail;
     }
@@ -1067,7 +1089,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     }
 
     if (!drv) {
-        ret = find_image_format(file, filename, &drv);
+        ret = find_image_format(file, filename, &drv, &local_err);
     }
 
     if (!drv) {
@@ -1075,7 +1097,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     }
 
     /* Open the image */
-    ret = bdrv_open_common(bs, file, options, flags, drv);
+    ret = bdrv_open_common(bs, file, options, flags, drv, &local_err);
     if (ret < 0) {
         goto unlink_and_fail;
     }
@@ -1090,7 +1112,7 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
         QDict *backing_options;
 
         extract_subqdict(options, &backing_options, "backing.");
-        ret = bdrv_open_backing_file(bs, backing_options);
+        ret = bdrv_open_backing_file(bs, backing_options, &local_err);
         if (ret < 0) {
             goto close_and_fail;
         }
@@ -1099,9 +1121,9 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
     /* Check if any unknown options were used */
     if (qdict_size(options) != 0) {
         const QDictEntry *entry = qdict_first(options);
-        qerror_report(ERROR_CLASS_GENERIC_ERROR, "Block format '%s' used by "
-            "device '%s' doesn't support the option '%s'",
-            drv->format_name, bs->device_name, entry->key);
+        error_setg(errp, "Block format '%s' used by device '%s' doesn't "
+                   "support the option '%s'", drv->format_name, bs->device_name,
+                   entry->key);
 
         ret = -EINVAL;
         goto close_and_fail;
@@ -1130,11 +1152,17 @@ fail:
     QDECREF(bs->options);
     QDECREF(options);
     bs->options = NULL;
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+    }
     return ret;
 
 close_and_fail:
     bdrv_close(bs);
     QDECREF(options);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+    }
     return ret;
 }
 
@@ -4613,7 +4641,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
             bs = bdrv_new("");
 
             ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
-                            backing_drv);
+                            backing_drv, NULL);
             if (ret < 0) {
                 error_setg_errno(errp, -ret, "Could not open '%s'",
                                  backing_file->value.s);
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 52d65ff..be948b2 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -387,8 +387,10 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = bdrv_file_open(&bs->file, filename, NULL, flags);
+    ret = bdrv_file_open(&bs->file, filename, NULL, flags, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         goto fail;
     }
 
diff --git a/block/blkverify.c b/block/blkverify.c
index 5d716bb..cceb88f 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -141,8 +141,10 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
 
-    ret = bdrv_file_open(&bs->file, raw, NULL, flags);
+    ret = bdrv_file_open(&bs->file, raw, NULL, flags, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         goto fail;
     }
 
@@ -154,8 +156,10 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
     }
 
     s->test_file = bdrv_new("");
-    ret = bdrv_open(s->test_file, filename, NULL, flags, NULL);
+    ret = bdrv_open(s->test_file, filename, NULL, flags, NULL, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         bdrv_delete(s->test_file);
         s->test_file = NULL;
         goto fail;
diff --git a/block/cow.c b/block/cow.c
index 207fea1..3468113 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -263,6 +263,7 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
     struct stat st;
     int64_t image_sectors = 0;
     const char *image_filename = NULL;
+    Error *local_err = NULL;
     int ret;
     BlockDriverState *cow_bs;
 
@@ -281,8 +282,10 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
         return ret;
     }
 
-    ret = bdrv_file_open(&cow_bs, filename, NULL, BDRV_O_RDWR);
+    ret = bdrv_file_open(&cow_bs, filename, NULL, BDRV_O_RDWR, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return ret;
     }
 
diff --git a/block/mirror.c b/block/mirror.c
index 86de458..b85326b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -505,14 +505,15 @@ static void mirror_iostatus_reset(BlockJob *job)
 static void mirror_complete(BlockJob *job, Error **errp)
 {
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
+    Error *local_err = NULL;
     int ret;
 
-    ret = bdrv_open_backing_file(s->target, NULL);
+    ret = bdrv_open_backing_file(s->target, NULL, &local_err);
     if (ret < 0) {
         char backing_filename[PATH_MAX];
         bdrv_get_full_backing_filename(s->target, backing_filename,
                                        sizeof(backing_filename));
-        error_setg_file_open(errp, -ret, backing_filename);
+        error_propagate(errp, local_err);
         return;
     }
     if (!s->synced) {
diff --git a/block/qcow.c b/block/qcow.c
index 6f7e73d..0a528e7 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -661,6 +661,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
     int64_t total_size = 0;
     const char *backing_file = NULL;
     int flags = 0;
+    Error *local_err = NULL;
     int ret;
     BlockDriverState *qcow_bs;
 
@@ -681,8 +682,10 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
         return ret;
     }
 
-    ret = bdrv_file_open(&qcow_bs, filename, NULL, BDRV_O_RDWR);
+    ret = bdrv_file_open(&qcow_bs, filename, NULL, BDRV_O_RDWR, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return ret;
     }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 98d1551..8692931 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1359,7 +1359,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         return ret;
     }
 
-    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR);
+    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR, NULL);
     if (ret < 0) {
         return ret;
     }
@@ -1412,7 +1412,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     BlockDriver* drv = bdrv_find_format("qcow2");
     assert(drv != NULL);
     ret = bdrv_open(bs, filename, NULL,
-        BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv);
+        BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, NULL);
     if (ret < 0) {
         goto out;
     }
diff --git a/block/qed.c b/block/qed.c
index 1a9d01b..d6451bb 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -551,6 +551,7 @@ static int qed_create(const char *filename, uint32_t cluster_size,
     QEDHeader le_header;
     uint8_t *l1_table = NULL;
     size_t l1_size = header.cluster_size * header.table_size;
+    Error *local_err = NULL;
     int ret = 0;
     BlockDriverState *bs = NULL;
 
@@ -559,8 +560,11 @@ static int qed_create(const char *filename, uint32_t cluster_size,
         return ret;
     }
 
-    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR | BDRV_O_CACHE_WB);
+    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR | BDRV_O_CACHE_WB,
+                         &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return ret;
     }
 
diff --git a/block/sheepdog.c b/block/sheepdog.c
index b1a0f7a..d32b802 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1401,10 +1401,13 @@ static int sd_prealloc(const char *filename)
     uint32_t idx, max_idx;
     int64_t vdi_size;
     void *buf = g_malloc0(SD_DATA_OBJ_SIZE);
+    Error *local_err = NULL;
     int ret;
 
-    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR);
+    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         goto out;
     }
 
@@ -1449,6 +1452,7 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
     char vdi[SD_MAX_VDI_LEN], tag[SD_MAX_VDI_TAG_LEN];
     uint32_t snapid;
     bool prealloc = false;
+    Error *local_err = NULL;
 
     s = g_malloc0(sizeof(BDRVSheepdogState));
 
@@ -1502,8 +1506,10 @@ static int sd_create(const char *filename, QEMUOptionParameter *options,
             goto out;
         }
 
-        ret = bdrv_file_open(&bs, backing_file, NULL, 0);
+        ret = bdrv_file_open(&bs, backing_file, NULL, 0, &local_err);
         if (ret < 0) {
+            qerror_report_err(local_err);
+            error_free(local_err);
             goto out;
         }
 
diff --git a/block/vmdk.c b/block/vmdk.c
index eb8bba2..e62be63 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -697,6 +697,7 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
     int64_t flat_offset;
     char extent_path[PATH_MAX];
     BlockDriverState *extent_file;
+    Error *local_err = NULL;
 
     while (*p) {
         /* parse extent line:
@@ -726,8 +727,11 @@ static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
 
         path_combine(extent_path, sizeof(extent_path),
                 desc_file_path, fname);
-        ret = bdrv_file_open(&extent_file, extent_path, NULL, bs->open_flags);
+        ret = bdrv_file_open(&extent_file, extent_path, NULL, bs->open_flags,
+                             &local_err);
         if (ret) {
+            qerror_report_err(local_err);
+            error_free(local_err);
             return ret;
         }
 
@@ -1574,6 +1578,7 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
         "ddb.geometry.heads = \"%d\"\n"
         "ddb.geometry.sectors = \"63\"\n"
         "ddb.adapterType = \"%s\"\n";
+    Error *local_err = NULL;
 
     if (filename_decompose(filename, path, prefix, postfix, PATH_MAX)) {
         return -EINVAL;
@@ -1636,8 +1641,10 @@ static int vmdk_create(const char *filename, QEMUOptionParameter *options,
     }
     if (backing_file) {
         BlockDriverState *bs = bdrv_new("");
-        ret = bdrv_open(bs, backing_file, NULL, 0, NULL);
+        ret = bdrv_open(bs, backing_file, NULL, 0, NULL, &local_err);
         if (ret != 0) {
+            qerror_report_err(local_err);
+            error_free(local_err);
             bdrv_delete(bs);
             return ret;
         }
diff --git a/block/vvfat.c b/block/vvfat.c
index d10159d..6084d71 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2909,6 +2909,7 @@ static int enable_write_target(BDRVVVFATState *s)
 {
     BlockDriver *bdrv_qcow;
     QEMUOptionParameter *options;
+    Error *local_err = NULL;
     int ret;
     int size = sector2cluster(s, s->sector_count);
     s->used_clusters = calloc(size, 1);
@@ -2934,8 +2935,11 @@ static int enable_write_target(BDRVVVFATState *s)
     s->qcow = bdrv_new("");
 
     ret = bdrv_open(s->qcow, s->qcow_filename, NULL,
-            BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow);
+            BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, bdrv_qcow,
+            &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         bdrv_delete(s->qcow);
         goto err;
     }
diff --git a/blockdev.c b/blockdev.c
index e70e16e..b1b95fb 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -707,17 +707,11 @@ static DriveInfo *blockdev_init(QemuOpts *all_opts,
     }
 
     QINCREF(bs_opts);
-    ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv);
+    ret = bdrv_open(dinfo->bdrv, file, bs_opts, bdrv_flags, drv, &error);
 
     if (ret < 0) {
-        if (ret == -EMEDIUMTYPE) {
-            error_report("could not open disk image %s: not in %s format",
-                         file ?: dinfo->id, drv ? drv->format_name :
-                         qdict_get_str(bs_opts, "driver"));
-        } else {
-            error_report("could not open disk image %s: %s",
-                         file ?: dinfo->id, strerror(-ret));
-        }
+        error_report("could not open disk image %s: %s",
+                     file ?: dinfo->id, error_get_pretty(error));
         goto err;
     }
 
@@ -957,9 +951,9 @@ static void external_snapshot_prepare(BlkTransactionState *common,
     /* TODO Inherit bs->options or only take explicit options with an
      * extended QMP command? */
     ret = bdrv_open(state->new_bs, new_image_file, NULL,
-                    flags | BDRV_O_NO_BACKING, drv);
+                    flags | BDRV_O_NO_BACKING, drv, &local_err);
     if (ret != 0) {
-        error_setg_file_open(errp, -ret, new_image_file);
+        error_propagate(errp, local_err);
     }
 }
 
@@ -1189,11 +1183,12 @@ static void qmp_bdrv_open_encrypted(BlockDriverState *bs, const char *filename,
                                     int bdrv_flags, BlockDriver *drv,
                                     const char *password, Error **errp)
 {
+    Error *local_err = NULL;
     int ret;
 
-    ret = bdrv_open(bs, filename, NULL, bdrv_flags, drv);
+    ret = bdrv_open(bs, filename, NULL, bdrv_flags, drv, &local_err);
     if (ret < 0) {
-        error_setg_file_open(errp, -ret, filename);
+        error_propagate(errp, local_err);
         return;
     }
 
@@ -1583,10 +1578,10 @@ void qmp_drive_backup(const char *device, const char *target,
     }
 
     target_bs = bdrv_new("");
-    ret = bdrv_open(target_bs, target, NULL, flags, drv);
+    ret = bdrv_open(target_bs, target, NULL, flags, drv, &local_err);
     if (ret < 0) {
         bdrv_delete(target_bs);
-        error_setg_file_open(errp, -ret, target);
+        error_propagate(errp, local_err);
         return;
     }
 
@@ -1723,10 +1718,11 @@ void qmp_drive_mirror(const char *device, const char *target,
      * file.
      */
     target_bs = bdrv_new("");
-    ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv);
+    ret = bdrv_open(target_bs, target, NULL, flags | BDRV_O_NO_BACKING, drv,
+                    &local_err);
     if (ret < 0) {
         bdrv_delete(target_bs);
-        error_setg_file_open(errp, -ret, target);
+        error_propagate(errp, local_err);
         return;
     }
 
diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
index 727f433..aa2080d 100644
--- a/hw/block/xen_disk.c
+++ b/hw/block/xen_disk.c
@@ -809,10 +809,15 @@ static int blk_connect(struct XenDevice *xendev)
         xen_be_printf(&blkdev->xendev, 2, "create new bdrv (xenbus setup)\n");
         blkdev->bs = bdrv_new(blkdev->dev);
         if (blkdev->bs) {
+            Error *local_err = NULL;
             BlockDriver *drv = bdrv_find_whitelisted_format(blkdev->fileproto,
                                                            readonly);
             if (bdrv_open(blkdev->bs,
-                          blkdev->filename, NULL, qflags, drv) != 0) {
+                          blkdev->filename, NULL, qflags, drv, &local_err) != 0)
+            {
+                xen_be_printf(&blkdev->xendev, 0, "error: %s\n",
+                              error_get_pretty(local_err));
+                error_free(local_err);
                 bdrv_delete(blkdev->bs);
                 blkdev->bs = NULL;
             }
diff --git a/include/block/block.h b/include/block/block.h
index e6b391c..efafd7a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -127,10 +127,10 @@ void bdrv_delete(BlockDriverState *bs);
 int bdrv_parse_cache_flags(const char *mode, int *flags);
 int bdrv_parse_discard_flags(const char *mode, int *flags);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename,
-                   QDict *options, int flags);
-int bdrv_open_backing_file(BlockDriverState *bs, QDict *options);
+                   QDict *options, int flags, Error **errp);
+int bdrv_open_backing_file(BlockDriverState *bs, QDict *options, Error **errp);
 int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
-              int flags, BlockDriver *drv);
+              int flags, BlockDriver *drv, Error **errp);
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
                                     BlockDriverState *bs, int flags);
 int bdrv_reopen_multiple(BlockReopenQueue *bs_queue, Error **errp);
diff --git a/qemu-img.c b/qemu-img.c
index b9a848d..3971ba5 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -264,6 +264,7 @@ static BlockDriverState *bdrv_new_open(const char *filename,
     BlockDriverState *bs;
     BlockDriver *drv;
     char password[256];
+    Error *local_err = NULL;
     int ret;
 
     bs = bdrv_new("image");
@@ -278,9 +279,11 @@ static BlockDriverState *bdrv_new_open(const char *filename,
         drv = NULL;
     }
 
-    ret = bdrv_open(bs, filename, NULL, flags, drv);
+    ret = bdrv_open(bs, filename, NULL, flags, drv, &local_err);
     if (ret < 0) {
-        error_report("Could not open '%s': %s", filename, strerror(-ret));
+        error_report("Could not open '%s': %s", filename,
+                     error_get_pretty(local_err));
+        error_free(local_err);
         goto fail;
     }
 
@@ -1912,6 +1915,7 @@ static int img_rebase(int argc, char **argv)
     int unsafe = 0;
     int progress = 0;
     bool quiet = false;
+    Error *local_err = NULL;
 
     /* Parse commandline parameters */
     fmt = NULL;
@@ -2015,18 +2019,21 @@ static int img_rebase(int argc, char **argv)
         bs_old_backing = bdrv_new("old_backing");
         bdrv_get_backing_filename(bs, backing_name, sizeof(backing_name));
         ret = bdrv_open(bs_old_backing, backing_name, NULL, BDRV_O_FLAGS,
-                        old_backing_drv);
+                        old_backing_drv, &local_err);
         if (ret) {
-            error_report("Could not open old backing file '%s'", backing_name);
+            error_report("Could not open old backing file '%s': %s",
+                         backing_name, error_get_pretty(local_err));
+            error_free(local_err);
             goto out;
         }
         if (out_baseimg[0]) {
             bs_new_backing = bdrv_new("new_backing");
             ret = bdrv_open(bs_new_backing, out_baseimg, NULL, BDRV_O_FLAGS,
-                        new_backing_drv);
+                        new_backing_drv, &local_err);
             if (ret) {
-                error_report("Could not open new backing file '%s'",
-                             out_baseimg);
+                error_report("Could not open new backing file '%s': %s",
+                             out_baseimg, error_get_pretty(local_err));
+                error_free(local_err);
                 goto out;
             }
         }
diff --git a/qemu-io.c b/qemu-io.c
index d54dc86..88f74b2 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -46,21 +46,27 @@ static const cmdinfo_t close_cmd = {
 
 static int openfile(char *name, int flags, int growable)
 {
+    Error *local_err = NULL;
+
     if (qemuio_bs) {
         fprintf(stderr, "file open already, try 'help close'\n");
         return 1;
     }
 
     if (growable) {
-        if (bdrv_file_open(&qemuio_bs, name, NULL, flags)) {
-            fprintf(stderr, "%s: can't open device %s\n", progname, name);
+        if (bdrv_file_open(&qemuio_bs, name, NULL, flags, &local_err)) {
+            fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
+                    error_get_pretty(local_err));
+            error_free(local_err);
             return 1;
         }
     } else {
         qemuio_bs = bdrv_new("hda");
 
-        if (bdrv_open(qemuio_bs, name, NULL, flags, NULL) < 0) {
-            fprintf(stderr, "%s: can't open device %s\n", progname, name);
+        if (bdrv_open(qemuio_bs, name, NULL, flags, NULL, &local_err) < 0) {
+            fprintf(stderr, "%s: can't open device %s: %s\n", progname, name,
+                    error_get_pretty(local_err));
+            error_free(local_err);
             bdrv_delete(qemuio_bs);
             qemuio_bs = NULL;
             return 1;
diff --git a/qemu-nbd.c b/qemu-nbd.c
index f044546..c26c98e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -355,6 +355,7 @@ int main(int argc, char **argv)
 #endif
     pthread_t client_thread;
     const char *fmt = NULL;
+    Error *local_err = NULL;
 
     /* The client thread uses SIGTERM to interrupt the server.  A signal
      * handler ensures that "qemu-nbd -v -c" exits with a nice status code.
@@ -573,10 +574,11 @@ int main(int argc, char **argv)
 
     bs = bdrv_new("hda");
     srcpath = argv[optind];
-    ret = bdrv_open(bs, srcpath, NULL, flags, drv);
+    ret = bdrv_open(bs, srcpath, NULL, flags, drv, &local_err);
     if (ret < 0) {
         errno = -ret;
-        err(EXIT_FAILURE, "Failed to bdrv_open '%s'", argv[optind]);
+        err(EXIT_FAILURE, "Failed to bdrv_open '%s': %s", argv[optind],
+            error_get_pretty(local_err));
     }
 
     fd_size = bdrv_getlength(bs);
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [RFC v3 4/6] block: Error parameter for create functions
  2013-09-06 15:52 [Qemu-devel] [RFC v3 0/6] block: Error parameter for opening/creating images Max Reitz
                   ` (2 preceding siblings ...)
  2013-09-06 15:52 ` [Qemu-devel] [RFC v3 3/6] block: Error parameter for open functions Max Reitz
@ 2013-09-06 15:52 ` Max Reitz
  2013-09-06 15:52 ` [Qemu-devel] [RFC v3 5/6] qemu-img create: Emit filename on error Max Reitz
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2013-09-06 15:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz

Add an Error ** parameter to bdrv_create and its associated functions to
allow more specific error messages.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block.c               | 80 +++++++++++++++++++++++++++++++++++----------------
 block/cow.c           |  4 ++-
 block/qcow.c          |  4 ++-
 block/qcow2.c         |  2 +-
 block/qed.c           |  4 ++-
 block/raw_bsd.c       | 10 ++++++-
 block/vvfat.c         |  4 ++-
 include/block/block.h |  5 ++--
 qemu-img.c            | 16 ++++-------
 9 files changed, 86 insertions(+), 43 deletions(-)

diff --git a/block.c b/block.c
index 8b0a729..12a2ca2 100644
--- a/block.c
+++ b/block.c
@@ -367,18 +367,26 @@ typedef struct CreateCo {
     char *filename;
     QEMUOptionParameter *options;
     int ret;
+    Error *err;
 } CreateCo;
 
 static void coroutine_fn bdrv_create_co_entry(void *opaque)
 {
+    Error *local_err = NULL;
+    int ret;
+
     CreateCo *cco = opaque;
     assert(cco->drv);
 
-    cco->ret = cco->drv->bdrv_create(cco->filename, cco->options, NULL);
+    ret = cco->drv->bdrv_create(cco->filename, cco->options, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(&cco->err, local_err);
+    }
+    cco->ret = ret;
 }
 
 int bdrv_create(BlockDriver *drv, const char* filename,
-    QEMUOptionParameter *options)
+    QEMUOptionParameter *options, Error **errp)
 {
     int ret;
 
@@ -388,9 +396,11 @@ int bdrv_create(BlockDriver *drv, const char* filename,
         .filename = g_strdup(filename),
         .options = options,
         .ret = NOT_DONE,
+        .err = NULL,
     };
 
     if (!drv->bdrv_create) {
+        error_setg(errp, "Driver '%s' does not support image creation", drv->format_name);
         ret = -ENOTSUP;
         goto out;
     }
@@ -407,22 +417,37 @@ int bdrv_create(BlockDriver *drv, const char* filename,
     }
 
     ret = cco.ret;
+    if (ret < 0) {
+        if (error_is_set(&cco.err)) {
+            error_propagate(errp, cco.err);
+        } else {
+            error_setg_errno(errp, -ret, "Could not create image");
+        }
+    }
 
 out:
     g_free(cco.filename);
     return ret;
 }
 
-int bdrv_create_file(const char* filename, QEMUOptionParameter *options)
+int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
+                     Error **errp)
 {
     BlockDriver *drv;
+    Error *local_err = NULL;
+    int ret;
 
     drv = bdrv_find_protocol(filename, true);
     if (drv == NULL) {
+        error_setg(errp, "Could not find protocol for file '%s'", filename);
         return -ENOENT;
     }
 
-    return bdrv_create(drv, filename, options);
+    ret = bdrv_create(drv, filename, options, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+    }
+    return ret;
 }
 
 /*
@@ -1055,11 +1080,14 @@ int bdrv_open(BlockDriverState *bs, const char *filename, QDict *options,
                 drv->format_name);
         }
 
-        ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options);
+        ret = bdrv_create(bdrv_qcow2, tmp_filename, create_options, &local_err);
         free_option_parameters(create_options);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not create temporary overlay "
-                             "'%s'", tmp_filename);
+                             "'%s': %s", tmp_filename,
+                             error_get_pretty(local_err));
+            error_free(local_err);
+            local_err = NULL;
             goto fail;
         }
 
@@ -4555,6 +4583,7 @@ void bdrv_img_create(const char *filename, const char *fmt,
     BlockDriverState *bs = NULL;
     BlockDriver *drv, *proto_drv;
     BlockDriver *backing_drv = NULL;
+    Error *local_err = NULL;
     int ret = 0;
 
     /* Find driver and parse its options */
@@ -4641,10 +4670,13 @@ void bdrv_img_create(const char *filename, const char *fmt,
             bs = bdrv_new("");
 
             ret = bdrv_open(bs, backing_file->value.s, NULL, back_flags,
-                            backing_drv, NULL);
+                            backing_drv, &local_err);
             if (ret < 0) {
-                error_setg_errno(errp, -ret, "Could not open '%s'",
-                                 backing_file->value.s);
+                error_setg_errno(errp, -ret, "Could not open '%s': %s",
+                                 backing_file->value.s,
+                                 error_get_pretty(local_err));
+                error_free(local_err);
+                local_err = NULL;
                 goto out;
             }
             bdrv_get_geometry(bs, &size);
@@ -4663,22 +4695,19 @@ void bdrv_img_create(const char *filename, const char *fmt,
         print_option_parameters(param);
         puts("");
     }
-    ret = bdrv_create(drv, filename, param);
-    if (ret < 0) {
-        if (ret == -ENOTSUP) {
-            error_setg(errp,"Formatting or formatting option not supported for "
-                            "file format '%s'", fmt);
-        } else if (ret == -EFBIG) {
-            const char *cluster_size_hint = "";
-            if (get_option_parameter(create_options, BLOCK_OPT_CLUSTER_SIZE)) {
-                cluster_size_hint = " (try using a larger cluster size)";
-            }
-            error_setg(errp, "The image size is too large for file format '%s'%s",
-                       fmt, cluster_size_hint);
-        } else {
-            error_setg(errp, "%s: error while creating %s: %s", filename, fmt,
-                       strerror(-ret));
+    ret = bdrv_create(drv, filename, param, &local_err);
+    if (ret == -EFBIG) {
+        /* This is generally a better message than whatever the driver would
+         * deliver (especially because of the cluster_size_hint), since that
+         * is most probably not much different from "image too large". */
+        const char *cluster_size_hint = "";
+        if (get_option_parameter(create_options, BLOCK_OPT_CLUSTER_SIZE)) {
+            cluster_size_hint = " (try using a larger cluster size)";
         }
+        error_setg(errp, "The image size is too large for file format '%s'"
+                   "%s", fmt, cluster_size_hint);
+        error_free(local_err);
+        local_err = NULL;
     }
 
 out:
@@ -4688,6 +4717,9 @@ out:
     if (bs) {
         bdrv_delete(bs);
     }
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+    }
 }
 
 AioContext *bdrv_get_aio_context(BlockDriverState *bs)
diff --git a/block/cow.c b/block/cow.c
index 3468113..0b93b45 100644
--- a/block/cow.c
+++ b/block/cow.c
@@ -277,8 +277,10 @@ static int cow_create(const char *filename, QEMUOptionParameter *options,
         options++;
     }
 
-    ret = bdrv_create_file(filename, options);
+    ret = bdrv_create_file(filename, options, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return ret;
     }
 
diff --git a/block/qcow.c b/block/qcow.c
index 0a528e7..9b4020f 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -677,8 +677,10 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options,
         options++;
     }
 
-    ret = bdrv_create_file(filename, options);
+    ret = bdrv_create_file(filename, options, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return ret;
     }
 
diff --git a/block/qcow2.c b/block/qcow2.c
index 8692931..e16d352 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1354,7 +1354,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     uint8_t* refcount_table;
     int ret;
 
-    ret = bdrv_create_file(filename, options);
+    ret = bdrv_create_file(filename, options, NULL);
     if (ret < 0) {
         return ret;
     }
diff --git a/block/qed.c b/block/qed.c
index d6451bb..168a68a 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -555,8 +555,10 @@ static int qed_create(const char *filename, uint32_t cluster_size,
     int ret = 0;
     BlockDriverState *bs = NULL;
 
-    ret = bdrv_create_file(filename, NULL);
+    ret = bdrv_create_file(filename, NULL, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return ret;
     }
 
diff --git a/block/raw_bsd.c b/block/raw_bsd.c
index 793121a..2effe72 100644
--- a/block/raw_bsd.c
+++ b/block/raw_bsd.c
@@ -133,7 +133,15 @@ static int raw_has_zero_init(BlockDriverState *bs)
 static int raw_create(const char *filename, QEMUOptionParameter *options,
                       Error **errp)
 {
-    return bdrv_create_file(filename, options);
+    Error *local_err = NULL;
+    int ret;
+
+    ret = bdrv_create_file(filename, options, &local_err);
+    if (error_is_set(&local_err)) {
+        qerror_report_err(local_err);
+        error_free(local_err);
+    }
+    return ret;
 }
 
 static int raw_open(BlockDriverState *bs, QDict *options, int flags,
diff --git a/block/vvfat.c b/block/vvfat.c
index 6084d71..fcb97f4 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -2927,8 +2927,10 @@ static int enable_write_target(BDRVVVFATState *s)
     set_option_parameter_int(options, BLOCK_OPT_SIZE, s->sector_count * 512);
     set_option_parameter(options, BLOCK_OPT_BACKING_FILE, "fat:");
 
-    ret = bdrv_create(bdrv_qcow, s->qcow_filename, options);
+    ret = bdrv_create(bdrv_qcow, s->qcow_filename, options, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         goto err;
     }
 
diff --git a/include/block/block.h b/include/block/block.h
index efafd7a..15e226d 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -117,8 +117,9 @@ BlockDriver *bdrv_find_format(const char *format_name);
 BlockDriver *bdrv_find_whitelisted_format(const char *format_name,
                                           bool readonly);
 int bdrv_create(BlockDriver *drv, const char* filename,
-    QEMUOptionParameter *options);
-int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
+    QEMUOptionParameter *options, Error **errp);
+int bdrv_create_file(const char* filename, QEMUOptionParameter *options,
+                     Error **errp);
 BlockDriverState *bdrv_new(const char *device_name);
 void bdrv_make_anon(BlockDriverState *bs);
 void bdrv_swap(BlockDriverState *bs_new, BlockDriverState *bs_old);
diff --git a/qemu-img.c b/qemu-img.c
index 3971ba5..6f8554e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1136,6 +1136,7 @@ static int img_convert(int argc, char **argv)
     float local_progress = 0;
     int min_sparse = 8; /* Need at least 4k of zeros for sparse detection */
     bool quiet = false;
+    Error *local_err = NULL;
 
     fmt = NULL;
     out_fmt = "raw";
@@ -1333,18 +1334,11 @@ static int img_convert(int argc, char **argv)
     }
 
     /* Create the new image */
-    ret = bdrv_create(drv, out_filename, param);
+    ret = bdrv_create(drv, out_filename, param, &local_err);
     if (ret < 0) {
-        if (ret == -ENOTSUP) {
-            error_report("Formatting not supported for file format '%s'",
-                         out_fmt);
-        } else if (ret == -EFBIG) {
-            error_report("The image size is too large for file format '%s'",
-                         out_fmt);
-        } else {
-            error_report("%s: error while converting %s: %s",
-                         out_filename, out_fmt, strerror(-ret));
-        }
+        error_report("%s: error while converting %s: %s",
+                     out_filename, out_fmt, error_get_pretty(local_err));
+        error_free(local_err);
         goto out;
     }
 
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [RFC v3 5/6] qemu-img create: Emit filename on error
  2013-09-06 15:52 [Qemu-devel] [RFC v3 0/6] block: Error parameter for opening/creating images Max Reitz
                   ` (3 preceding siblings ...)
  2013-09-06 15:52 ` [Qemu-devel] [RFC v3 4/6] block: Error parameter for create functions Max Reitz
@ 2013-09-06 15:52 ` Max Reitz
  2013-09-06 15:52 ` [Qemu-devel] [RFC v3 6/6] qcow2: Use Error parameter Max Reitz
  2013-09-10 14:37 ` [Qemu-devel] [RFC v3 0/6] block: Error parameter for opening/creating images Kevin Wolf
  6 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2013-09-06 15:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz

bdrv_img_create generally does not emit the target filename, although
this is pretty important information. Therefore, prepend its error
message with the output filename (if an error occurs).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 qemu-img.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qemu-img.c b/qemu-img.c
index 6f8554e..a0c400b 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -410,7 +410,7 @@ static int img_create(int argc, char **argv)
     bdrv_img_create(filename, fmt, base_filename, base_fmt,
                     options, img_size, BDRV_O_FLAGS, &local_err, quiet);
     if (error_is_set(&local_err)) {
-        error_report("%s", error_get_pretty(local_err));
+        error_report("%s: %s", filename, error_get_pretty(local_err));
         error_free(local_err);
         return 1;
     }
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Qemu-devel] [RFC v3 6/6] qcow2: Use Error parameter
  2013-09-06 15:52 [Qemu-devel] [RFC v3 0/6] block: Error parameter for opening/creating images Max Reitz
                   ` (4 preceding siblings ...)
  2013-09-06 15:52 ` [Qemu-devel] [RFC v3 5/6] qemu-img create: Emit filename on error Max Reitz
@ 2013-09-06 15:52 ` Max Reitz
  2013-09-10 14:37 ` [Qemu-devel] [RFC v3 0/6] block: Error parameter for opening/creating images Kevin Wolf
  6 siblings, 0 replies; 8+ messages in thread
From: Max Reitz @ 2013-09-06 15:52 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz

Employ usage of the new Error ** parameter in qcow2_open, qcow2_create
and associated functions.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 134 ++++++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 88 insertions(+), 46 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index e16d352..65d1c48 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -79,7 +79,8 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
  * return 0 upon success, non-0 otherwise
  */
 static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
-                                 uint64_t end_offset, void **p_feature_table)
+                                 uint64_t end_offset, void **p_feature_table,
+                                 Error **errp)
 {
     BDRVQcowState *s = bs->opaque;
     QCowExtension ext;
@@ -100,10 +101,10 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
         printf("attempting to read extended header in offset %lu\n", offset);
 #endif
 
-        if (bdrv_pread(bs->file, offset, &ext, sizeof(ext)) != sizeof(ext)) {
-            fprintf(stderr, "qcow2_read_extension: ERROR: "
-                    "pread fail from offset %" PRIu64 "\n",
-                    offset);
+        ret = bdrv_pread(bs->file, offset, &ext, sizeof(ext));
+        if (ret < 0) {
+            error_setg_errno(errp, -ret, "qcow2_read_extension: ERROR: "
+                             "pread fail from offset %" PRIu64, offset);
             return 1;
         }
         be32_to_cpus(&ext.magic);
@@ -113,7 +114,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
         printf("ext.magic = 0x%x\n", ext.magic);
 #endif
         if (ext.len > end_offset - offset) {
-            error_report("Header extension too large");
+            error_setg(errp, "Header extension too large");
             return -EINVAL;
         }
 
@@ -123,14 +124,16 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
 
         case QCOW2_EXT_MAGIC_BACKING_FORMAT:
             if (ext.len >= sizeof(bs->backing_format)) {
-                fprintf(stderr, "ERROR: ext_backing_format: len=%u too large"
-                        " (>=%zu)\n",
-                        ext.len, sizeof(bs->backing_format));
+                error_setg(errp, "ERROR: ext_backing_format: len=%u too large"
+                           " (>=%zu)", ext.len, sizeof(bs->backing_format));
                 return 2;
             }
-            if (bdrv_pread(bs->file, offset , bs->backing_format,
-                           ext.len) != ext.len)
+            ret = bdrv_pread(bs->file, offset, bs->backing_format, ext.len);
+            if (ret < 0) {
+                error_setg_errno(errp, -ret, "ERROR: ext_backing_format: "
+                                 "Could not read format name");
                 return 3;
+            }
             bs->backing_format[ext.len] = '\0';
 #ifdef DEBUG_EXT
             printf("Qcow2: Got format extension %s\n", bs->backing_format);
@@ -142,6 +145,8 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
                 void* feature_table = g_malloc0(ext.len + 2 * sizeof(Qcow2Feature));
                 ret = bdrv_pread(bs->file, offset , feature_table, ext.len);
                 if (ret < 0) {
+                    error_setg_errno(errp, -ret, "ERROR: ext_feature_table: "
+                                     "Could not read table");
                     return ret;
                 }
 
@@ -161,6 +166,8 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
 
                 ret = bdrv_pread(bs->file, offset , uext->data, uext->len);
                 if (ret < 0) {
+                    error_setg_errno(errp, -ret, "ERROR: unknown extension: "
+                                     "Could not read data");
                     return ret;
                 }
             }
@@ -184,8 +191,8 @@ static void cleanup_unknown_header_ext(BlockDriverState *bs)
     }
 }
 
-static void GCC_FMT_ATTR(2, 3) report_unsupported(BlockDriverState *bs,
-    const char *fmt, ...)
+static void GCC_FMT_ATTR(3, 4) report_unsupported(BlockDriverState *bs,
+    Error **errp, const char *fmt, ...)
 {
     char msg[64];
     va_list ap;
@@ -194,17 +201,17 @@ static void GCC_FMT_ATTR(2, 3) report_unsupported(BlockDriverState *bs,
     vsnprintf(msg, sizeof(msg), fmt, ap);
     va_end(ap);
 
-    qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
-        bs->device_name, "qcow2", msg);
+    error_set(errp, QERR_UNKNOWN_BLOCK_FORMAT_FEATURE, bs->device_name, "qcow2",
+              msg);
 }
 
 static void report_unsupported_feature(BlockDriverState *bs,
-    Qcow2Feature *table, uint64_t mask)
+    Error **errp, Qcow2Feature *table, uint64_t mask)
 {
     while (table && table->name[0] != '\0') {
         if (table->type == QCOW2_FEAT_TYPE_INCOMPATIBLE) {
             if (mask & (1 << table->bit)) {
-                report_unsupported(bs, "%.46s",table->name);
+                report_unsupported(bs, errp, "%.46s", table->name);
                 mask &= ~(1 << table->bit);
             }
         }
@@ -212,7 +219,8 @@ static void report_unsupported_feature(BlockDriverState *bs,
     }
 
     if (mask) {
-        report_unsupported(bs, "Unknown incompatible feature: %" PRIx64, mask);
+        report_unsupported(bs, errp, "Unknown incompatible feature: %" PRIx64,
+                           mask);
     }
 }
 
@@ -363,6 +371,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
 
     ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not read qcow2 header");
         goto fail;
     }
     be32_to_cpus(&header.magic);
@@ -380,11 +389,12 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     be32_to_cpus(&header.nb_snapshots);
 
     if (header.magic != QCOW_MAGIC) {
+        error_setg(errp, "Image is not in qcow2 format");
         ret = -EMEDIUMTYPE;
         goto fail;
     }
     if (header.version < 2 || header.version > 3) {
-        report_unsupported(bs, "QCOW version %d", header.version);
+        report_unsupported(bs, errp, "QCOW version %d", header.version);
         ret = -ENOTSUP;
         goto fail;
     }
@@ -412,6 +422,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         ret = bdrv_pread(bs->file, sizeof(header), s->unknown_header_fields,
                          s->unknown_header_fields_size);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Could not read unknown qcow2 header "
+                             "fields");
             goto fail;
         }
     }
@@ -430,8 +442,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
         void *feature_table = NULL;
         qcow2_read_extensions(bs, header.header_length, ext_end,
-                              &feature_table);
-        report_unsupported_feature(bs, feature_table,
+                              &feature_table, NULL);
+        report_unsupported_feature(bs, errp, feature_table,
                                    s->incompatible_features &
                                    ~QCOW2_INCOMPAT_MASK);
         ret = -ENOTSUP;
@@ -442,8 +454,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         /* Corrupt images may not be written to unless they are being repaired
          */
         if ((flags & BDRV_O_RDWR) && !(flags & BDRV_O_CHECK)) {
-            error_report("qcow2: Image is corrupt; cannot be opened "
-                    "read/write.");
+            error_setg(errp, "qcow2: Image is corrupt; cannot be opened "
+                       "read/write");
             ret = -EACCES;
             goto fail;
         }
@@ -451,7 +463,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
 
     /* Check support for various header values */
     if (header.refcount_order != 4) {
-        report_unsupported(bs, "%d bit reference counts",
+        report_unsupported(bs, errp, "%d bit reference counts",
                            1 << header.refcount_order);
         ret = -ENOTSUP;
         goto fail;
@@ -459,10 +471,13 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
 
     if (header.cluster_bits < MIN_CLUSTER_BITS ||
         header.cluster_bits > MAX_CLUSTER_BITS) {
+        error_setg(errp, "Unsupported cluster size: 2^%i", header.cluster_bits);
         ret = -EINVAL;
         goto fail;
     }
     if (header.crypt_method > QCOW_CRYPT_AES) {
+        error_setg(errp, "Unsupported encryption method: %i",
+                   header.crypt_method);
         ret = -EINVAL;
         goto fail;
     }
@@ -491,6 +506,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
 
     l1_vm_state_index = size_to_l1(s, header.size);
     if (l1_vm_state_index > INT_MAX) {
+        error_setg(errp, "Image is too big");
         ret = -EFBIG;
         goto fail;
     }
@@ -499,6 +515,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     /* the L1 table must contain at least enough entries to put
        header.size bytes */
     if (s->l1_size < s->l1_vm_state_index) {
+        error_setg(errp, "L1 table is too small");
         ret = -EINVAL;
         goto fail;
     }
@@ -509,6 +526,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         ret = bdrv_pread(bs->file, s->l1_table_offset, s->l1_table,
                          s->l1_size * sizeof(uint64_t));
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Could not read L1 table");
             goto fail;
         }
         for(i = 0;i < s->l1_size; i++) {
@@ -529,6 +547,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
 
     ret = qcow2_refcount_init(bs);
     if (ret != 0) {
+        error_setg_errno(errp, -ret, "Could not initialize refcount handling");
         goto fail;
     }
 
@@ -536,7 +555,9 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     QTAILQ_INIT(&s->discards);
 
     /* read qcow2 extensions */
-    if (qcow2_read_extensions(bs, header.header_length, ext_end, NULL)) {
+    if (qcow2_read_extensions(bs, header.header_length, ext_end, NULL,
+        &local_err)) {
+        error_propagate(errp, local_err);
         ret = -EINVAL;
         goto fail;
     }
@@ -550,6 +571,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         ret = bdrv_pread(bs->file, header.backing_file_offset,
                          bs->backing_file, len);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Could not read backing file name");
             goto fail;
         }
         bs->backing_file[len] = '\0';
@@ -557,6 +579,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
 
     ret = qcow2_read_snapshots(bs);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not read snapshots");
         goto fail;
     }
 
@@ -565,6 +588,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
         s->autoclear_features = 0;
         ret = qcow2_update_header(bs);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Could not update qcow2 header");
             goto fail;
         }
     }
@@ -579,6 +603,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
 
         ret = qcow2_check(bs, &result, BDRV_FIX_ERRORS);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Could not repair dirty image");
             goto fail;
         }
     }
@@ -587,8 +612,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     opts = qemu_opts_create_nofail(&qcow2_runtime_opts);
     qemu_opts_absorb_qdict(opts, options, &local_err);
     if (error_is_set(&local_err)) {
-        qerror_report_err(local_err);
-        error_free(local_err);
+        error_propagate(errp, local_err);
         ret = -EINVAL;
         goto fail;
     }
@@ -609,8 +633,8 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags,
     qemu_opts_del(opts);
 
     if (s->use_lazy_refcounts && s->qcow_version < 3) {
-        qerror_report(ERROR_CLASS_GENERIC_ERROR, "Lazy refcounts require "
-            "a qcow2 image with at least qemu 1.1 compatibility level");
+        error_setg(errp, "Lazy refcounts require a qcow2 image with at least "
+                   "qemu 1.1 compatibility level");
         ret = -EINVAL;
         goto fail;
     }
@@ -1323,7 +1347,8 @@ static int preallocate(BlockDriverState *bs)
 static int qcow2_create2(const char *filename, int64_t total_size,
                          const char *backing_file, const char *backing_format,
                          int flags, size_t cluster_size, int prealloc,
-                         QEMUOptionParameter *options, int version)
+                         QEMUOptionParameter *options, int version,
+                         Error **errp)
 {
     /* Calculate cluster_bits */
     int cluster_bits;
@@ -1331,9 +1356,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     if (cluster_bits < MIN_CLUSTER_BITS || cluster_bits > MAX_CLUSTER_BITS ||
         (1 << cluster_bits) != cluster_size)
     {
-        error_report(
-            "Cluster size must be a power of two between %d and %dk",
-            1 << MIN_CLUSTER_BITS, 1 << (MAX_CLUSTER_BITS - 10));
+        error_setg(errp, "Cluster size must be a power of two between %d and "
+                   "%dk", 1 << MIN_CLUSTER_BITS, 1 << (MAX_CLUSTER_BITS - 10));
         return -EINVAL;
     }
 
@@ -1352,15 +1376,18 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     BlockDriverState* bs;
     QCowHeader header;
     uint8_t* refcount_table;
+    Error *local_err = NULL;
     int ret;
 
-    ret = bdrv_create_file(filename, options, NULL);
+    ret = bdrv_create_file(filename, options, &local_err);
     if (ret < 0) {
+        error_propagate(errp, local_err);
         return ret;
     }
 
-    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR, NULL);
+    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR, &local_err);
     if (ret < 0) {
+        error_propagate(errp, local_err);
         return ret;
     }
 
@@ -1390,6 +1417,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
 
     ret = bdrv_pwrite(bs, 0, &header, sizeof(header));
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not write qcow2 header");
         goto out;
     }
 
@@ -1399,6 +1427,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     g_free(refcount_table);
 
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not write refcount table");
         goto out;
     }
 
@@ -1412,13 +1441,16 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     BlockDriver* drv = bdrv_find_format("qcow2");
     assert(drv != NULL);
     ret = bdrv_open(bs, filename, NULL,
-        BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, NULL);
+        BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH, drv, &local_err);
     if (ret < 0) {
+        error_propagate(errp, local_err);
         goto out;
     }
 
     ret = qcow2_alloc_clusters(bs, 2 * cluster_size);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not allocate clusters for qcow2 "
+                         "header and refcount table");
         goto out;
 
     } else if (ret != 0) {
@@ -1429,6 +1461,7 @@ 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 = bdrv_truncate(bs, total_size * BDRV_SECTOR_SIZE);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "Could not resize image");
         goto out;
     }
 
@@ -1436,6 +1469,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
     if (backing_file) {
         ret = bdrv_change_backing_file(bs, backing_file, backing_format);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Could not assign backing file '%s' "
+                             "with format '%s'", backing_file, backing_format);
             goto out;
         }
     }
@@ -1447,6 +1482,7 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         ret = preallocate(bs);
         qemu_co_mutex_unlock(&s->lock);
         if (ret < 0) {
+            error_setg_errno(errp, -ret, "Could not preallocate metadata");
             goto out;
         }
     }
@@ -1467,6 +1503,8 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
     size_t cluster_size = DEFAULT_CLUSTER_SIZE;
     int prealloc = 0;
     int version = 3;
+    Error *local_err = NULL;
+    int ret;
 
     /* Read out options */
     while (options && options->name) {
@@ -1488,8 +1526,8 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
             } else if (!strcmp(options->value.s, "metadata")) {
                 prealloc = 1;
             } else {
-                fprintf(stderr, "Invalid preallocation mode: '%s'\n",
-                    options->value.s);
+                error_setg(errp, "Invalid preallocation mode: '%s'",
+                           options->value.s);
                 return -EINVAL;
             }
         } else if (!strcmp(options->name, BLOCK_OPT_COMPAT_LEVEL)) {
@@ -1500,8 +1538,8 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
             } else if (!strcmp(options->value.s, "1.1")) {
                 version = 3;
             } else {
-                fprintf(stderr, "Invalid compatibility level: '%s'\n",
-                    options->value.s);
+                error_setg(errp, "Invalid compatibility level: '%s'",
+                           options->value.s);
                 return -EINVAL;
             }
         } else if (!strcmp(options->name, BLOCK_OPT_LAZY_REFCOUNTS)) {
@@ -1511,19 +1549,23 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
     }
 
     if (backing_file && prealloc) {
-        fprintf(stderr, "Backing file and preallocation cannot be used at "
-            "the same time\n");
+        error_setg(errp, "Backing file and preallocation cannot be used at "
+                   "the same time");
         return -EINVAL;
     }
 
     if (version < 3 && (flags & BLOCK_FLAG_LAZY_REFCOUNTS)) {
-        fprintf(stderr, "Lazy refcounts only supported with compatibility "
-                "level 1.1 and above (use compat=1.1 or greater)\n");
+        error_setg(errp, "Lazy refcounts only supported with compatibility "
+                   "level 1.1 and above (use compat=1.1 or greater)");
         return -EINVAL;
     }
 
-    return qcow2_create2(filename, sectors, backing_file, backing_fmt, flags,
-                         cluster_size, prealloc, options, version);
+    ret = qcow2_create2(filename, sectors, backing_file, backing_fmt, flags,
+                        cluster_size, prealloc, options, version, &local_err);
+    if (error_is_set(&local_err)) {
+        error_propagate(errp, local_err);
+    }
+    return ret;
 }
 
 static int qcow2_make_empty(BlockDriverState *bs)
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Qemu-devel] [RFC v3 0/6] block: Error parameter for opening/creating images
  2013-09-06 15:52 [Qemu-devel] [RFC v3 0/6] block: Error parameter for opening/creating images Max Reitz
                   ` (5 preceding siblings ...)
  2013-09-06 15:52 ` [Qemu-devel] [RFC v3 6/6] qcow2: Use Error parameter Max Reitz
@ 2013-09-10 14:37 ` Kevin Wolf
  6 siblings, 0 replies; 8+ messages in thread
From: Kevin Wolf @ 2013-09-10 14:37 UTC (permalink / raw)
  To: Max Reitz; +Cc: qemu-devel, Stefan Hajnoczi

Am 06.09.2013 um 17:52 hat Max Reitz geschrieben:
> This RFC adds an Error ** parameter to bdrv_open, bdrv_file_open,
> bdrv_create and the respective functions provided by a block driver.
> 
> This results in more specific error information than just -errno provided
> to the user when opening or creating images (disregarding the fact that
> block drivers often already use error_report, which is generally changed
> to error_setg through this patch).
> 
> The sixth patch in this series changes the qcow2 block driver to set an
> example of usage in a block driver.
> 
> Note that several I/O tests break by applying this RFC since they expect
> different error messages (generally, previously, an error message on
> image opening/creation consisted of two lines; the first of which would be
> generated by the driver through error_report, the second by the block
> layer itself through strerror(-ret); this patch is designed to merge these
> two lines into a single one). This applies to the tests 49, 51, 54 and 60.

Looks good to me now (except that it needs a rebase because it conflicts
with other recent patches in the block tree).

I guess it's time to address the qemu-iotests part and send out a
non-RFC series.

Kevin

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2013-09-10 14:37 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-06 15:52 [Qemu-devel] [RFC v3 0/6] block: Error parameter for opening/creating images Max Reitz
2013-09-06 15:52 ` [Qemu-devel] [RFC v3 1/6] bdrv: Use "Error" for opening images Max Reitz
2013-09-06 15:52 ` [Qemu-devel] [RFC v3 2/6] bdrv: Use "Error" for creating images Max Reitz
2013-09-06 15:52 ` [Qemu-devel] [RFC v3 3/6] block: Error parameter for open functions Max Reitz
2013-09-06 15:52 ` [Qemu-devel] [RFC v3 4/6] block: Error parameter for create functions Max Reitz
2013-09-06 15:52 ` [Qemu-devel] [RFC v3 5/6] qemu-img create: Emit filename on error Max Reitz
2013-09-06 15:52 ` [Qemu-devel] [RFC v3 6/6] qcow2: Use Error parameter Max Reitz
2013-09-10 14:37 ` [Qemu-devel] [RFC v3 0/6] block: Error parameter for opening/creating images Kevin Wolf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).