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

[Greg Kurz]

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: ");
>      }
>  }
>