diff mbox series

[v2,13/13] block/qed: bdrv_qed_do_open: deal with errp

Message ID 20200917195519.19589-14-vsementsov@virtuozzo.com
State Superseded
Headers show
Series block: deal with errp: part I | expand

Commit Message

Vladimir Sementsov-Ogievskiy Sept. 17, 2020, 7:55 p.m. UTC 
Set errp always on failure. Generic bdrv_open_driver supports driver
functions which can return negative value and forget to set errp.
That's a strange thing.. Let's improve bdrv_qed_do_open to not behave
this way. This allows to simplify code in
bdrv_qed_co_invalidate_cache().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block/qed.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

Comments

Greg Kurz Sept. 18, 2020, 4:07 p.m. UTC | #1 
On Thu, 17 Sep 2020 22:55:19 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> Set errp always on failure. Generic bdrv_open_driver supports driver
> functions which can return negative value and forget to set errp.
> That's a strange thing.. Let's improve bdrv_qed_do_open to not behave
> this way. This allows to simplify code in
> bdrv_qed_co_invalidate_cache().
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---

Reviewed-by: Greg Kurz <groug@kaod.org>

>  block/qed.c | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/block/qed.c b/block/qed.c
> index b27e7546ca..f45c640513 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -393,6 +393,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>  
>      ret = bdrv_pread(bs->file, 0, &le_header, sizeof(le_header));
>      if (ret < 0) {
> +        error_setg(errp, "Failed to read QED header");
>          return ret;
>      }
>      qed_header_le_to_cpu(&le_header, &s->header);
> @@ -408,25 +409,30 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>          return -ENOTSUP;
>      }
>      if (!qed_is_cluster_size_valid(s->header.cluster_size)) {
> +        error_setg(errp, "QED cluster size is invalid");
>          return -EINVAL;
>      }
>  
>      /* Round down file size to the last cluster */
>      file_size = bdrv_getlength(bs->file->bs);
>      if (file_size < 0) {
> +        error_setg(errp, "Failed to get file length");
>          return file_size;
>      }
>      s->file_size = qed_start_of_cluster(s, file_size);
>  
>      if (!qed_is_table_size_valid(s->header.table_size)) {
> +        error_setg(errp, "QED table size is invalid");
>          return -EINVAL;
>      }
>      if (!qed_is_image_size_valid(s->header.image_size,
>                                   s->header.cluster_size,
>                                   s->header.table_size)) {
> +        error_setg(errp, "QED image size is invalid");
>          return -EINVAL;
>      }
>      if (!qed_check_table_offset(s, s->header.l1_table_offset)) {
> +        error_setg(errp, "QED table offset is invalid");
>          return -EINVAL;
>      }
>  
> @@ -438,6 +444,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>  
>      /* Header size calculation must not overflow uint32_t */
>      if (s->header.header_size > UINT32_MAX / s->header.cluster_size) {
> +        error_setg(errp, "QED header size is too large");
>          return -EINVAL;
>      }
>  
> @@ -445,6 +452,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>          if ((uint64_t)s->header.backing_filename_offset +
>              s->header.backing_filename_size >
>              s->header.cluster_size * s->header.header_size) {
> +            error_setg(errp, "QED backing filename offset is invalid");
>              return -EINVAL;
>          }
>  
> @@ -453,6 +461,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>                                bs->auto_backing_file,
>                                sizeof(bs->auto_backing_file));
>          if (ret < 0) {
> +            error_setg(errp, "Failed to read backing filename");
>              return ret;
>          }
>          pstrcpy(bs->backing_file, sizeof(bs->backing_file),
> @@ -475,6 +484,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>  
>          ret = qed_write_header_sync(s);
>          if (ret) {
> +            error_setg(errp, "Failed to update header");
>              return ret;
>          }
>  
> @@ -487,6 +497,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>  
>      ret = qed_read_l1_table_sync(s);
>      if (ret) {
> +        error_setg(errp, "Failed to read L1 table");
>          goto out;
>      }
>  
> @@ -503,6 +514,7 @@ static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
>  
>              ret = qed_check(s, &result, true);
>              if (ret) {
> +                error_setg(errp, "Image corrupted");
>                  goto out;
>              }
>          }
> @@ -1537,22 +1549,16 @@ static void coroutine_fn bdrv_qed_co_invalidate_cache(BlockDriverState *bs,
>                                                        Error **errp)
>  {
>      BDRVQEDState *s = bs->opaque;
> -    Error *local_err = NULL;
>      int ret;
>  
>      bdrv_qed_close(bs);
>  
>      bdrv_qed_init_state(bs);
>      qemu_co_mutex_lock(&s->table_lock);
> -    ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, &local_err);
> +    ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, errp);
>      qemu_co_mutex_unlock(&s->table_lock);
> -    if (local_err) {
> -        error_propagate_prepend(errp, local_err,
> -                                "Could not reopen qed layer: ");
> -        return;
> -    } else if (ret < 0) {
> -        error_setg_errno(errp, -ret, "Could not reopen qed layer");
> -        return;
> +    if (ret < 0) {
> +        error_prepend(errp, "Could not reopen qed layer: ");
>      }
>  }
>
diff mbox series

Patch

diff --git a/block/qed.c b/block/qed.c
index b27e7546ca..f45c640513 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -393,6 +393,7 @@  static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
 
     ret = bdrv_pread(bs->file, 0, &le_header, sizeof(le_header));
     if (ret < 0) {
+        error_setg(errp, "Failed to read QED header");
         return ret;
     }
     qed_header_le_to_cpu(&le_header, &s->header);
@@ -408,25 +409,30 @@  static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
         return -ENOTSUP;
     }
     if (!qed_is_cluster_size_valid(s->header.cluster_size)) {
+        error_setg(errp, "QED cluster size is invalid");
         return -EINVAL;
     }
 
     /* Round down file size to the last cluster */
     file_size = bdrv_getlength(bs->file->bs);
     if (file_size < 0) {
+        error_setg(errp, "Failed to get file length");
         return file_size;
     }
     s->file_size = qed_start_of_cluster(s, file_size);
 
     if (!qed_is_table_size_valid(s->header.table_size)) {
+        error_setg(errp, "QED table size is invalid");
         return -EINVAL;
     }
     if (!qed_is_image_size_valid(s->header.image_size,
                                  s->header.cluster_size,
                                  s->header.table_size)) {
+        error_setg(errp, "QED image size is invalid");
         return -EINVAL;
     }
     if (!qed_check_table_offset(s, s->header.l1_table_offset)) {
+        error_setg(errp, "QED table offset is invalid");
         return -EINVAL;
     }
 
@@ -438,6 +444,7 @@  static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
 
     /* Header size calculation must not overflow uint32_t */
     if (s->header.header_size > UINT32_MAX / s->header.cluster_size) {
+        error_setg(errp, "QED header size is too large");
         return -EINVAL;
     }
 
@@ -445,6 +452,7 @@  static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
         if ((uint64_t)s->header.backing_filename_offset +
             s->header.backing_filename_size >
             s->header.cluster_size * s->header.header_size) {
+            error_setg(errp, "QED backing filename offset is invalid");
             return -EINVAL;
         }
 
@@ -453,6 +461,7 @@  static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
                               bs->auto_backing_file,
                               sizeof(bs->auto_backing_file));
         if (ret < 0) {
+            error_setg(errp, "Failed to read backing filename");
             return ret;
         }
         pstrcpy(bs->backing_file, sizeof(bs->backing_file),
@@ -475,6 +484,7 @@  static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
 
         ret = qed_write_header_sync(s);
         if (ret) {
+            error_setg(errp, "Failed to update header");
             return ret;
         }
 
@@ -487,6 +497,7 @@  static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
 
     ret = qed_read_l1_table_sync(s);
     if (ret) {
+        error_setg(errp, "Failed to read L1 table");
         goto out;
     }
 
@@ -503,6 +514,7 @@  static int coroutine_fn bdrv_qed_do_open(BlockDriverState *bs, QDict *options,
 
             ret = qed_check(s, &result, true);
             if (ret) {
+                error_setg(errp, "Image corrupted");
                 goto out;
             }
         }
@@ -1537,22 +1549,16 @@  static void coroutine_fn bdrv_qed_co_invalidate_cache(BlockDriverState *bs,
                                                       Error **errp)
 {
     BDRVQEDState *s = bs->opaque;
-    Error *local_err = NULL;
     int ret;
 
     bdrv_qed_close(bs);
 
     bdrv_qed_init_state(bs);
     qemu_co_mutex_lock(&s->table_lock);
-    ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, &local_err);
+    ret = bdrv_qed_do_open(bs, NULL, bs->open_flags, errp);
     qemu_co_mutex_unlock(&s->table_lock);
-    if (local_err) {
-        error_propagate_prepend(errp, local_err,
-                                "Could not reopen qed layer: ");
-        return;
-    } else if (ret < 0) {
-        error_setg_errno(errp, -ret, "Could not reopen qed layer");
-        return;
+    if (ret < 0) {
+        error_prepend(errp, "Could not reopen qed layer: ");
     }
 }