[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 13/13] block/qed: bdrv_qed_do_open: deal with errp
From: |
Greg Kurz |
Subject: |
Re: [PATCH v2 13/13] block/qed: bdrv_qed_do_open: deal with errp |
Date: |
Fri, 18 Sep 2020 18:07:13 +0200 |
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: ");
> }
> }
>
- Re: [PATCH v2 12/13] block/qcow2: simplify qcow2_co_invalidate_cache(), (continued)
[PATCH v2 08/13] block/qcow2: qcow2_get_specific_info(): drop error propagation, Vladimir Sementsov-Ogievskiy, 2020/09/17
[PATCH v2 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface, Vladimir Sementsov-Ogievskiy, 2020/09/17
[PATCH v2 13/13] block/qed: bdrv_qed_do_open: deal with errp, Vladimir Sementsov-Ogievskiy, 2020/09/17
- Re: [PATCH v2 13/13] block/qed: bdrv_qed_do_open: deal with errp,
Greg Kurz <=
Re: [PATCH v2 00/13] block: deal with errp: part I, no-reply, 2020/09/17