Patchwork [RFC,v2,6/6] bdrv: No silent error message discarding

login
register
mail settings
Submitter Max Reitz
Date Sept. 5, 2013, 1:55 p.m.
Message ID <1378389342-4749-7-git-send-email-mreitz@redhat.com>
Download mbox | patch
Permalink /patch/272897/
State New
Headers show

Comments

Max Reitz - Sept. 5, 2013, 1:55 p.m.
Using NULL as errp parameters to bdrv_open, bdrv_file_open and
bdrv_create in all block drivers which do not yet implement proper error
propagation is not a good idea, since this now silently discards error
messages. Fix this by using a proper parameter and printing its content
on error (until proper propagation is implemented).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
Note that many error propagation are actually trivial (such as in
raw_bsd), but I intentionally refrained from implementing the propagation
and tenaciously followed the pattern:
  Error *local_err = NULL;
  foo(&local_err);
  if (/*error condition*/) {
      qerror_report_err(local_err):
      error_free(local_err);
  }
This is because the propagation may sometimes be trivial, however, it is
often still driver-specific, therefore this deserves its own patch for
every driver, in my opinion. Also, I think it is easier to review this
way. ;-)
---
 block/blkdebug.c  |  4 +++-
 block/blkverify.c |  8 ++++++--
 block/cow.c       |  9 +++++++--
 block/qcow.c      |  9 +++++++--
 block/qed.c       |  9 +++++++--
 block/raw_bsd.c   | 10 +++++++++-
 block/sheepdog.c  |  8 ++++++--
 block/vmdk.c      | 10 ++++++++--
 block/vvfat.c     | 10 ++++++++--
 9 files changed, 61 insertions(+), 16 deletions(-)
Kevin Wolf - Sept. 6, 2013, 2:22 p.m.
Am 05.09.2013 um 15:55 hat Max Reitz geschrieben:
> Using NULL as errp parameters to bdrv_open, bdrv_file_open and
> bdrv_create in all block drivers which do not yet implement proper error
> propagation is not a good idea, since this now silently discards error
> messages. Fix this by using a proper parameter and printing its content
> on error (until proper propagation is implemented).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> Note that many error propagation are actually trivial (such as in
> raw_bsd), but I intentionally refrained from implementing the propagation
> and tenaciously followed the pattern:
>   Error *local_err = NULL;
>   foo(&local_err);
>   if (/*error condition*/) {
>       qerror_report_err(local_err):
>       error_free(local_err);
>   }
> This is because the propagation may sometimes be trivial, however, it is
> often still driver-specific, therefore this deserves its own patch for
> every driver, in my opinion. Also, I think it is easier to review this
> way. ;-)

Agreed. But this patch should be squashed into the patches introducing
the NULL argument. Otherwise I'd need to check if this patch catches all
places that were previously introduced.

> diff --git a/block/sheepdog.c b/block/sheepdog.c
> index 6722771..a9d8019 100644
> --- a/block/sheepdog.c
> +++ b/block/sheepdog.c
> @@ -1401,9 +1401,10 @@ 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, NULL);
> +    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR, &local_err);
>      if (ret < 0) {
>          goto out;
>      }

You didn't feel like printing the message here? :-)

Kevin

Patch

diff --git a/block/blkdebug.c b/block/blkdebug.c
index e0ba16d..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, NULL);
+    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 d428efd..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, NULL);
+    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, 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 1c12996..0b93b45 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;
 
@@ -276,13 +277,17 @@  static int cow_create(const char *filename, QEMUOptionParameter *options,
         options++;
     }
 
-    ret = bdrv_create_file(filename, options, NULL);
+    ret = bdrv_create_file(filename, options, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return ret;
     }
 
-    ret = bdrv_file_open(&cow_bs, filename, NULL, BDRV_O_RDWR, NULL);
+    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/qcow.c b/block/qcow.c
index 50c6657..9b4020f 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;
 
@@ -676,13 +677,17 @@  static int qcow_create(const char *filename, QEMUOptionParameter *options,
         options++;
     }
 
-    ret = bdrv_create_file(filename, options, NULL);
+    ret = bdrv_create_file(filename, options, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return ret;
     }
 
-    ret = bdrv_file_open(&qcow_bs, filename, NULL, BDRV_O_RDWR, NULL);
+    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/qed.c b/block/qed.c
index 3552daf..168a68a 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -551,17 +551,22 @@  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;
 
-    ret = bdrv_create_file(filename, NULL, NULL);
+    ret = bdrv_create_file(filename, NULL, &local_err);
     if (ret < 0) {
+        qerror_report_err(local_err);
+        error_free(local_err);
         return ret;
     }
 
     ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR | BDRV_O_CACHE_WB,
-                         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 0f1b677..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, NULL);
+    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/sheepdog.c b/block/sheepdog.c
index 6722771..a9d8019 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1401,9 +1401,10 @@  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, NULL);
+    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR, &local_err);
     if (ret < 0) {
         goto out;
     }
@@ -1449,6 +1450,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 +1504,10 @@  static int sd_create(const char *filename, QEMUOptionParameter *options,
             goto out;
         }
 
-        ret = bdrv_file_open(&bs, backing_file, NULL, 0, NULL);
+        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 1e1b860..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:
@@ -727,8 +728,10 @@  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,
-                             NULL);
+                             &local_err);
         if (ret) {
+            qerror_report_err(local_err);
+            error_free(local_err);
             return ret;
         }
 
@@ -1575,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;
@@ -1637,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, 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 65e5bd0..fcb97f4 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);
@@ -2926,16 +2927,21 @@  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, NULL);
+    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;
     }
 
     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, NULL);
+            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;
     }