On Thu, 17 Sep 2020 22:55:19 +0300 Vladimir Sementsov-Ogievskiy <vsement...@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 <vsement...@virtuozzo.com> > Reviewed-by: Alberto Garcia <be...@igalia.com> > --- Reviewed-by: Greg Kurz <gr...@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: "); > } > } >