* [PATCH v3 01/13] block: return status from bdrv_append and friends
2020-10-16 17:10 [PATCH v3 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
@ 2020-10-16 17:10 ` Vladimir Sementsov-Ogievskiy
2020-10-19 11:50 ` Kevin Wolf
2020-10-16 17:10 ` [PATCH v3 02/13] block: use return status of bdrv_append() Vladimir Sementsov-Ogievskiy
` (11 subsequent siblings)
12 siblings, 1 reply; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-16 17:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, den, Greg Kurz
The recommended use of qemu error api assumes returning status together
with setting errp and avoid void functions with errp parameter. Let's
improve bdrv_append and some friends to reduce error-propagation
overhead in further patches.
Choose int return status, because bdrv_replace_node() has call to
bdrv_check_update_perm(), which reports int status, which seems correct
to propagate.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
include/block/block.h | 12 ++++++------
block.c | 39 ++++++++++++++++++++++++---------------
2 files changed, 30 insertions(+), 21 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index d16c401cb4..afb29cdbe4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -346,10 +346,10 @@ int bdrv_create(BlockDriver *drv, const char* filename,
int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
BlockDriverState *bdrv_new(void);
-void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
- Error **errp);
-void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
- Error **errp);
+int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
+ Error **errp);
+int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+ Error **errp);
int bdrv_parse_aio(const char *mode, int *flags);
int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
@@ -361,8 +361,8 @@ BdrvChild *bdrv_open_child(const char *filename,
BdrvChildRole child_role,
bool allow_none, Error **errp);
BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
- Error **errp);
+int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+ Error **errp);
int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
const char *bdref_key, Error **errp);
BlockDriverState *bdrv_open(const char *filename, const char *reference,
diff --git a/block.c b/block.c
index 430edf79bb..b05fbff42d 100644
--- a/block.c
+++ b/block.c
@@ -2870,14 +2870,15 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)
* Sets the bs->backing link of a BDS. A new reference is created; callers
* which don't need their own reference any more must call bdrv_unref().
*/
-void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
+int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
Error **errp)
{
+ int ret = 0;
bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
bdrv_inherits_from_recursive(backing_hd, bs);
if (bdrv_is_backing_chain_frozen(bs, child_bs(bs->backing), errp)) {
- return;
+ return -EPERM;
}
if (backing_hd) {
@@ -2896,15 +2897,22 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_of_bds,
bdrv_backing_role(bs), errp);
+ if (!bs->backing) {
+ ret = -EINVAL;
+ goto out;
+ }
+
/* If backing_hd was already part of bs's backing chain, and
* inherits_from pointed recursively to bs then let's update it to
* point directly to bs (else it will become NULL). */
- if (bs->backing && update_inherits_from) {
+ if (update_inherits_from) {
backing_hd->inherits_from = bs;
}
out:
bdrv_refresh_limits(bs, NULL);
+
+ return ret;
}
/*
@@ -4554,8 +4562,8 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
return ret;
}
-void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
- Error **errp)
+int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+ Error **errp)
{
BdrvChild *c, *next;
GSList *list = NULL, *p;
@@ -4577,6 +4585,7 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
continue;
}
if (c->frozen) {
+ ret = -EPERM;
error_setg(errp, "Cannot change '%s' link to '%s'",
c->name, from->node_name);
goto out;
@@ -4612,6 +4621,8 @@ out:
g_slist_free(list);
bdrv_drained_end(from);
bdrv_unref(from);
+
+ return ret;
}
/*
@@ -4630,20 +4641,16 @@ out:
* parents of bs_top after bdrv_append() returns. If the caller needs to keep a
* reference of its own, it must call bdrv_ref().
*/
-void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
- Error **errp)
+int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
+ Error **errp)
{
- Error *local_err = NULL;
-
- bdrv_set_backing_hd(bs_new, bs_top, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ int ret = bdrv_set_backing_hd(bs_new, bs_top, errp);
+ if (ret < 0) {
goto out;
}
- bdrv_replace_node(bs_top, bs_new, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ ret = bdrv_replace_node(bs_top, bs_new, errp);
+ if (ret < 0) {
bdrv_set_backing_hd(bs_new, NULL, &error_abort);
goto out;
}
@@ -4652,6 +4659,8 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
* additional reference any more. */
out:
bdrv_unref(bs_new);
+
+ return ret;
}
static void bdrv_delete(BlockDriverState *bs)
--
2.21.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 01/13] block: return status from bdrv_append and friends
2020-10-16 17:10 ` [PATCH v3 01/13] block: return status from bdrv_append and friends Vladimir Sementsov-Ogievskiy
@ 2020-10-19 11:50 ` Kevin Wolf
0 siblings, 0 replies; 20+ messages in thread
From: Kevin Wolf @ 2020-10-19 11:50 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru, Greg Kurz,
stefanha, den, pbonzini, mreitz, jsnow, ari
Am 16.10.2020 um 19:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> The recommended use of qemu error api assumes returning status together
> with setting errp and avoid void functions with errp parameter. Let's
> improve bdrv_append and some friends to reduce error-propagation
> overhead in further patches.
>
> Choose int return status, because bdrv_replace_node() has call to
> bdrv_check_update_perm(), which reports int status, which seems correct
> to propagate.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
> include/block/block.h | 12 ++++++------
> block.c | 39 ++++++++++++++++++++++++---------------
> 2 files changed, 30 insertions(+), 21 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index d16c401cb4..afb29cdbe4 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -346,10 +346,10 @@ int bdrv_create(BlockDriver *drv, const char* filename,
> int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp);
>
> BlockDriverState *bdrv_new(void);
> -void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
> - Error **errp);
> -void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
> - Error **errp);
> +int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
> + Error **errp);
> +int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
> + Error **errp);
>
> int bdrv_parse_aio(const char *mode, int *flags);
> int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough);
> @@ -361,8 +361,8 @@ BdrvChild *bdrv_open_child(const char *filename,
> BdrvChildRole child_role,
> bool allow_none, Error **errp);
> BlockDriverState *bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
> -void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
> - Error **errp);
> +int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
> + Error **errp);
> int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
> const char *bdref_key, Error **errp);
> BlockDriverState *bdrv_open(const char *filename, const char *reference,
> diff --git a/block.c b/block.c
> index 430edf79bb..b05fbff42d 100644
> --- a/block.c
> +++ b/block.c
> @@ -2870,14 +2870,15 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)
> * Sets the bs->backing link of a BDS. A new reference is created; callers
> * which don't need their own reference any more must call bdrv_unref().
> */
> -void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
> +int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
> Error **errp)
> {
> + int ret = 0;
> bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) &&
> bdrv_inherits_from_recursive(backing_hd, bs);
>
> if (bdrv_is_backing_chain_frozen(bs, child_bs(bs->backing), errp)) {
> - return;
> + return -EPERM;
> }
>
> if (backing_hd) {
> @@ -2896,15 +2897,22 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
>
> bs->backing = bdrv_attach_child(bs, backing_hd, "backing", &child_of_bds,
> bdrv_backing_role(bs), errp);
> + if (!bs->backing) {
> + ret = -EINVAL;
I think -EPERM describes the actual error cases better (though I'm not
sure if we're going to actually use the error code anywhere).
> + goto out;
> + }
> +
> /* If backing_hd was already part of bs's backing chain, and
> * inherits_from pointed recursively to bs then let's update it to
> * point directly to bs (else it will become NULL). */
> - if (bs->backing && update_inherits_from) {
> + if (update_inherits_from) {
> backing_hd->inherits_from = bs;
> }
>
> out:
Please move the ret = 0 from the declaration to right above this line.
Otherwise we'd have to be careful in the future that the last assignment
of ret can't give it a non-zero (probably positive) value. Having
ret = 0 immediately before the label is the safe pattern.
Another possible advantage is that in some cases the compiler may then
be able to warn if you forget to set ret in an error path.
> bdrv_refresh_limits(bs, NULL);
> +
> + return ret;
> }
>
> /*
> @@ -4554,8 +4562,8 @@ static bool should_update_child(BdrvChild *c, BlockDriverState *to)
> return ret;
> }
>
> -void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
> - Error **errp)
> +int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
> + Error **errp)
> {
> BdrvChild *c, *next;
> GSList *list = NULL, *p;
> @@ -4577,6 +4585,7 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
> continue;
> }
> if (c->frozen) {
> + ret = -EPERM;
> error_setg(errp, "Cannot change '%s' link to '%s'",
> c->name, from->node_name);
> goto out;
> @@ -4612,6 +4621,8 @@ out:
> g_slist_free(list);
> bdrv_drained_end(from);
> bdrv_unref(from);
> +
> + return ret;
Please add the ret = 0 before the label, too.
> }
>
> /*
> @@ -4630,20 +4641,16 @@ out:
> * parents of bs_top after bdrv_append() returns. If the caller needs to keep a
> * reference of its own, it must call bdrv_ref().
> */
> -void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
> - Error **errp)
> +int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
> + Error **errp)
> {
> - Error *local_err = NULL;
> -
> - bdrv_set_backing_hd(bs_new, bs_top, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> + int ret = bdrv_set_backing_hd(bs_new, bs_top, errp);
> + if (ret < 0) {
> goto out;
> }
>
> - bdrv_replace_node(bs_top, bs_new, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> + ret = bdrv_replace_node(bs_top, bs_new, errp);
> + if (ret < 0) {
> bdrv_set_backing_hd(bs_new, NULL, &error_abort);
> goto out;
> }
> @@ -4652,6 +4659,8 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
> * additional reference any more. */
> out:
And another one.
> bdrv_unref(bs_new);
> +
> + return ret;
> }
Looks good to me otherwise.
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 02/13] block: use return status of bdrv_append()
2020-10-16 17:10 [PATCH v3 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
2020-10-16 17:10 ` [PATCH v3 01/13] block: return status from bdrv_append and friends Vladimir Sementsov-Ogievskiy
@ 2020-10-16 17:10 ` Vladimir Sementsov-Ogievskiy
2020-10-19 12:13 ` Kevin Wolf
2020-10-16 17:10 ` [PATCH v3 03/13] block: check return value of bdrv_open_child and drop error propagation Vladimir Sementsov-Ogievskiy
` (10 subsequent siblings)
12 siblings, 1 reply; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-16 17:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, den, Greg Kurz
Now bdrv_append returns status and we can drop all the local_err things
around it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block.c | 5 +----
block/backup-top.c | 20 ++++++++------------
block/commit.c | 5 +----
block/mirror.c | 6 ++----
blockdev.c | 4 +---
tests/test-bdrv-graph-mod.c | 6 +++---
6 files changed, 16 insertions(+), 30 deletions(-)
diff --git a/block.c b/block.c
index b05fbff42d..7b6818c681 100644
--- a/block.c
+++ b/block.c
@@ -3161,7 +3161,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
int64_t total_size;
QemuOpts *opts = NULL;
BlockDriverState *bs_snapshot = NULL;
- Error *local_err = NULL;
int ret;
/* if snapshot, we create a temporary backing file and open it
@@ -3208,9 +3207,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
* order to be able to return one, we have to increase
* bs_snapshot's refcount here */
bdrv_ref(bs_snapshot);
- bdrv_append(bs_snapshot, bs, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (bdrv_append(bs_snapshot, bs, errp) < 0) {
bs_snapshot = NULL;
goto out;
}
diff --git a/block/backup-top.c b/block/backup-top.c
index fe6883cc97..eb6a34b726 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -190,7 +190,7 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
BlockCopyState **bcs,
Error **errp)
{
- Error *local_err = NULL;
+ ERRP_GUARD();
BDRVBackupTopState *state;
BlockDriverState *top;
bool appended = false;
@@ -223,9 +223,8 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
bdrv_drained_begin(source);
bdrv_ref(top);
- bdrv_append(top, source, &local_err);
- if (local_err) {
- error_prepend(&local_err, "Cannot append backup-top filter: ");
+ if (bdrv_append(top, source, errp) < 0) {
+ error_prepend(errp, "Cannot append backup-top filter: ");
goto fail;
}
appended = true;
@@ -235,18 +234,16 @@ BlockDriverState *bdrv_backup_top_append(BlockDriverState *source,
* we want.
*/
state->active = true;
- bdrv_child_refresh_perms(top, top->backing, &local_err);
- if (local_err) {
- error_prepend(&local_err,
- "Cannot set permissions for backup-top filter: ");
+ if (bdrv_child_refresh_perms(top, top->backing, errp) < 0) {
+ error_prepend(errp, "Cannot set permissions for backup-top filter: ");
goto fail;
}
state->cluster_size = cluster_size;
state->bcs = block_copy_state_new(top->backing, state->target,
- cluster_size, write_flags, &local_err);
- if (local_err) {
- error_prepend(&local_err, "Cannot create block-copy-state: ");
+ cluster_size, write_flags, errp);
+ if (!state->bcs) {
+ error_prepend(errp, "Cannot create block-copy-state: ");
goto fail;
}
*bcs = state->bcs;
@@ -264,7 +261,6 @@ fail:
}
bdrv_drained_end(source);
- error_propagate(errp, local_err);
return NULL;
}
diff --git a/block/commit.c b/block/commit.c
index 1e85c306cc..6da0902f9d 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -254,7 +254,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
BlockDriverState *iter;
BlockDriverState *commit_top_bs = NULL;
BlockDriverState *filtered_base;
- Error *local_err = NULL;
int64_t base_size, top_size;
uint64_t base_perms, iter_shared_perms;
int ret;
@@ -312,10 +311,8 @@ void commit_start(const char *job_id, BlockDriverState *bs,
commit_top_bs->total_sectors = top->total_sectors;
- bdrv_append(commit_top_bs, top, &local_err);
- if (local_err) {
+ if (bdrv_append(commit_top_bs, top, errp) < 0) {
commit_top_bs = NULL;
- error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/mirror.c b/block/mirror.c
index 26acf4af6f..b3778248b8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1560,7 +1560,6 @@ static BlockJob *mirror_start_job(
BlockDriverState *mirror_top_bs;
bool target_is_backing;
uint64_t target_perms, target_shared_perms;
- Error *local_err = NULL;
int ret;
if (granularity == 0) {
@@ -1609,12 +1608,11 @@ static BlockJob *mirror_start_job(
* it alive until block_job_create() succeeds even if bs has no parent. */
bdrv_ref(mirror_top_bs);
bdrv_drained_begin(bs);
- bdrv_append(mirror_top_bs, bs, &local_err);
+ ret = bdrv_append(mirror_top_bs, bs, errp);
bdrv_drained_end(bs);
- if (local_err) {
+ if (ret < 0) {
bdrv_unref(mirror_top_bs);
- error_propagate(errp, local_err);
return NULL;
}
diff --git a/blockdev.c b/blockdev.c
index fe6fb5dc1d..3a896181fd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1591,9 +1591,7 @@ static void external_snapshot_prepare(BlkActionState *common,
* can fail, so we need to do it in .prepare; undoing it for abort is
* always possible. */
bdrv_ref(state->new_bs);
- bdrv_append(state->new_bs, state->old_bs, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (bdrv_append(state->new_bs, state->old_bs, errp) < 0) {
goto out;
}
state->overlay_appended = true;
diff --git a/tests/test-bdrv-graph-mod.c b/tests/test-bdrv-graph-mod.c
index 8cff13830e..2b71601c24 100644
--- a/tests/test-bdrv-graph-mod.c
+++ b/tests/test-bdrv-graph-mod.c
@@ -101,7 +101,7 @@ static BlockDriverState *pass_through_node(const char *name)
*/
static void test_update_perm_tree(void)
{
- Error *local_err = NULL;
+ int ret;
BlockBackend *root = blk_new(qemu_get_aio_context(),
BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ,
@@ -114,8 +114,8 @@ static void test_update_perm_tree(void)
bdrv_attach_child(filter, bs, "child", &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, &error_abort);
- bdrv_append(filter, bs, &local_err);
- error_free_or_abort(&local_err);
+ ret = bdrv_append(filter, bs, NULL);
+ assert(ret < 0);
blk_unref(root);
}
--
2.21.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 02/13] block: use return status of bdrv_append()
2020-10-16 17:10 ` [PATCH v3 02/13] block: use return status of bdrv_append() Vladimir Sementsov-Ogievskiy
@ 2020-10-19 12:13 ` Kevin Wolf
2020-10-19 15:45 ` Markus Armbruster
0 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2020-10-19 12:13 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru, Greg Kurz,
stefanha, den, pbonzini, mreitz, jsnow, ari
Am 16.10.2020 um 19:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Now bdrv_append returns status and we can drop all the local_err things
> around it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Greg Kurz <groug@kaod.org>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
> block.c | 5 +----
> block/backup-top.c | 20 ++++++++------------
> block/commit.c | 5 +----
> block/mirror.c | 6 ++----
> blockdev.c | 4 +---
> tests/test-bdrv-graph-mod.c | 6 +++---
> 6 files changed, 16 insertions(+), 30 deletions(-)
>
> diff --git a/block.c b/block.c
> index b05fbff42d..7b6818c681 100644
> --- a/block.c
> +++ b/block.c
> @@ -3161,7 +3161,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
> int64_t total_size;
> QemuOpts *opts = NULL;
> BlockDriverState *bs_snapshot = NULL;
> - Error *local_err = NULL;
> int ret;
>
> /* if snapshot, we create a temporary backing file and open it
> @@ -3208,9 +3207,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
> * order to be able to return one, we have to increase
> * bs_snapshot's refcount here */
> bdrv_ref(bs_snapshot);
> - bdrv_append(bs_snapshot, bs, &local_err);
> - if (local_err) {
> - error_propagate(errp, local_err);
> + if (bdrv_append(bs_snapshot, bs, errp) < 0) {
We generally avoid calling functions with side effects inside a
comparison. Let's use the usual pattern:
ret = bdrv_append(bs_snapshot, bs, errp);
if (ret < 0) {
...
}
(I mention it only once, but there are multiple instances in this
patch.)
> bs_snapshot = NULL;
> goto out;
> }
> diff --git a/tests/test-bdrv-graph-mod.c b/tests/test-bdrv-graph-mod.c
> index 8cff13830e..2b71601c24 100644
> --- a/tests/test-bdrv-graph-mod.c
> +++ b/tests/test-bdrv-graph-mod.c
> @@ -101,7 +101,7 @@ static BlockDriverState *pass_through_node(const char *name)
> */
> static void test_update_perm_tree(void)
> {
> - Error *local_err = NULL;
> + int ret;
>
> BlockBackend *root = blk_new(qemu_get_aio_context(),
> BLK_PERM_WRITE | BLK_PERM_CONSISTENT_READ,
> @@ -114,8 +114,8 @@ static void test_update_perm_tree(void)
> bdrv_attach_child(filter, bs, "child", &child_of_bds,
> BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, &error_abort);
>
> - bdrv_append(filter, bs, &local_err);
> - error_free_or_abort(&local_err);
> + ret = bdrv_append(filter, bs, NULL);
> + assert(ret < 0);
Usually, test cases use g_assert_cmpint() which prints the expected and
actual value if it fails.
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 02/13] block: use return status of bdrv_append()
2020-10-19 12:13 ` Kevin Wolf
@ 2020-10-19 15:45 ` Markus Armbruster
2020-10-19 16:10 ` Kevin Wolf
0 siblings, 1 reply; 20+ messages in thread
From: Markus Armbruster @ 2020-10-19 15:45 UTC (permalink / raw)
To: Kevin Wolf
Cc: Vladimir Sementsov-Ogievskiy, berto, stefanha, qemu-block,
Greg Kurz, qemu-devel, pavel.dovgaluk, pbonzini, den, mreitz,
jsnow, ari
Kevin Wolf <kwolf@redhat.com> writes:
> Am 16.10.2020 um 19:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Now bdrv_append returns status and we can drop all the local_err things
>> around it.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Greg Kurz <groug@kaod.org>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>> block.c | 5 +----
>> block/backup-top.c | 20 ++++++++------------
>> block/commit.c | 5 +----
>> block/mirror.c | 6 ++----
>> blockdev.c | 4 +---
>> tests/test-bdrv-graph-mod.c | 6 +++---
>> 6 files changed, 16 insertions(+), 30 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index b05fbff42d..7b6818c681 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3161,7 +3161,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
>> int64_t total_size;
>> QemuOpts *opts = NULL;
>> BlockDriverState *bs_snapshot = NULL;
>> - Error *local_err = NULL;
>> int ret;
>>
>> /* if snapshot, we create a temporary backing file and open it
>> @@ -3208,9 +3207,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
>> * order to be able to return one, we have to increase
>> * bs_snapshot's refcount here */
>> bdrv_ref(bs_snapshot);
>> - bdrv_append(bs_snapshot, bs, &local_err);
>> - if (local_err) {
>> - error_propagate(errp, local_err);
>> + if (bdrv_append(bs_snapshot, bs, errp) < 0) {
>
> We generally avoid calling functions with side effects inside a
> comparison. Let's use the usual pattern:
>
> ret = bdrv_append(bs_snapshot, bs, errp);
> if (ret < 0) {
> ...
> }
I'd also advice against buring side effects too deep, but calling a
function in a failure-checking conditional is pretty benign. It's also
common:
$ git-grep 'if ([a-z].*) < 0) {'
coughs up several hundred instances.
That said, there is none in block.c. Local consistency matters.
[...]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 02/13] block: use return status of bdrv_append()
2020-10-19 15:45 ` Markus Armbruster
@ 2020-10-19 16:10 ` Kevin Wolf
0 siblings, 0 replies; 20+ messages in thread
From: Kevin Wolf @ 2020-10-19 16:10 UTC (permalink / raw)
To: Markus Armbruster
Cc: Vladimir Sementsov-Ogievskiy, berto, stefanha, qemu-block,
Greg Kurz, qemu-devel, pavel.dovgaluk, pbonzini, den, mreitz,
jsnow, ari
Am 19.10.2020 um 17:45 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 16.10.2020 um 19:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> Now bdrv_append returns status and we can drop all the local_err things
> >> around it.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >> Reviewed-by: Greg Kurz <groug@kaod.org>
> >> Reviewed-by: Alberto Garcia <berto@igalia.com>
> >> ---
> >> block.c | 5 +----
> >> block/backup-top.c | 20 ++++++++------------
> >> block/commit.c | 5 +----
> >> block/mirror.c | 6 ++----
> >> blockdev.c | 4 +---
> >> tests/test-bdrv-graph-mod.c | 6 +++---
> >> 6 files changed, 16 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index b05fbff42d..7b6818c681 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -3161,7 +3161,6 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
> >> int64_t total_size;
> >> QemuOpts *opts = NULL;
> >> BlockDriverState *bs_snapshot = NULL;
> >> - Error *local_err = NULL;
> >> int ret;
> >>
> >> /* if snapshot, we create a temporary backing file and open it
> >> @@ -3208,9 +3207,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
> >> * order to be able to return one, we have to increase
> >> * bs_snapshot's refcount here */
> >> bdrv_ref(bs_snapshot);
> >> - bdrv_append(bs_snapshot, bs, &local_err);
> >> - if (local_err) {
> >> - error_propagate(errp, local_err);
> >> + if (bdrv_append(bs_snapshot, bs, errp) < 0) {
> >
> > We generally avoid calling functions with side effects inside a
> > comparison. Let's use the usual pattern:
> >
> > ret = bdrv_append(bs_snapshot, bs, errp);
> > if (ret < 0) {
> > ...
> > }
>
> I'd also advice against buring side effects too deep, but calling a
> function in a failure-checking conditional is pretty benign. It's also
> common:
>
> $ git-grep 'if ([a-z].*) < 0) {'
>
> coughs up several hundred instances.
>
> That said, there is none in block.c. Local consistency matters.
Actually, after looking at the rest of the series, I also need to be a
bit more specific: We do have boolean if (!foo()), it's just the
negative errno ones that aren't very common in the block layer.
And the reason why it's not very common is probably that 0/-errno is
very common in the block layer and a check for < 0 just throws the
specific errno away. We had a lot of such code, which was wrong because
it lost information instead of passing the real error to the caller.
I guess this is the real origin of the block layer habits to avoid it.
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 03/13] block: check return value of bdrv_open_child and drop error propagation
2020-10-16 17:10 [PATCH v3 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
2020-10-16 17:10 ` [PATCH v3 01/13] block: return status from bdrv_append and friends Vladimir Sementsov-Ogievskiy
2020-10-16 17:10 ` [PATCH v3 02/13] block: use return status of bdrv_append() Vladimir Sementsov-Ogievskiy
@ 2020-10-16 17:10 ` Vladimir Sementsov-Ogievskiy
2020-10-16 17:10 ` [PATCH v3 04/13] blockdev: fix drive_backup_prepare() missed error Vladimir Sementsov-Ogievskiy
` (9 subsequent siblings)
12 siblings, 0 replies; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-16 17:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, den, Greg Kurz
This patch is generated by cocci script:
@@
symbol bdrv_open_child, errp, local_err;
expression file;
@@
file = bdrv_open_child(...,
- &local_err
+ errp
);
- if (local_err)
+ if (!file)
{
...
- error_propagate(errp, local_err);
...
}
with command
spatch --sp-file x.cocci --macro-file scripts/cocci-macro-file.h \
--in-place --no-show-diff --max-width 80 --use-gitgrep block
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/blkdebug.c | 6 ++----
block/blklogwrites.c | 10 ++++------
block/blkreplay.c | 6 ++----
block/blkverify.c | 11 ++++-------
block/qcow2.c | 5 ++---
block/quorum.c | 6 ++----
6 files changed, 16 insertions(+), 28 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 54da719dd1..d795077fee 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -465,7 +465,6 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
{
BDRVBlkdebugState *s = bs->opaque;
QemuOpts *opts;
- Error *local_err = NULL;
int ret;
uint64_t align;
@@ -495,10 +494,9 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "image",
bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, &local_err);
- if (local_err) {
+ false, errp);
+ if (!bs->file) {
ret = -EINVAL;
- error_propagate(errp, local_err);
goto out;
}
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 13ae63983b..b7579370a3 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -157,19 +157,17 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
/* Open the file */
bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, false,
- &local_err);
- if (local_err) {
+ errp);
+ if (!bs->file) {
ret = -EINVAL;
- error_propagate(errp, local_err);
goto fail;
}
/* Open the log file */
s->log_file = bdrv_open_child(NULL, options, "log", bs, &child_of_bds,
- BDRV_CHILD_METADATA, false, &local_err);
- if (local_err) {
+ BDRV_CHILD_METADATA, false, errp);
+ if (!s->log_file) {
ret = -EINVAL;
- error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/blkreplay.c b/block/blkreplay.c
index 30a0f5d57a..4a247752fd 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -23,16 +23,14 @@ typedef struct Request {
static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
- Error *local_err = NULL;
int ret;
/* Open the image file */
bs->file = bdrv_open_child(NULL, options, "image", bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, &local_err);
- if (local_err) {
+ false, errp);
+ if (!bs->file) {
ret = -EINVAL;
- error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/blkverify.c b/block/blkverify.c
index 4aed53ab59..95ae73e2aa 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -112,7 +112,6 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
{
BDRVBlkverifyState *s = bs->opaque;
QemuOpts *opts;
- Error *local_err = NULL;
int ret;
opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort);
@@ -125,20 +124,18 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
bs->file = bdrv_open_child(qemu_opt_get(opts, "x-raw"), options, "raw",
bs, &child_of_bds,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
- false, &local_err);
- if (local_err) {
+ false, errp);
+ if (!bs->file) {
ret = -EINVAL;
- error_propagate(errp, local_err);
goto fail;
}
/* Open the test file */
s->test_file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options,
"test", bs, &child_of_bds, BDRV_CHILD_DATA,
- false, &local_err);
- if (local_err) {
+ false, errp);
+ if (!s->test_file) {
ret = -EINVAL;
- error_propagate(errp, local_err);
goto fail;
}
diff --git a/block/qcow2.c b/block/qcow2.c
index b05512718c..41a29072e6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1612,9 +1612,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
/* Open external data file */
s->data_file = bdrv_open_child(NULL, options, "data-file", bs,
&child_of_bds, BDRV_CHILD_DATA,
- true, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ true, errp);
+ if (!s->data_file) {
ret = -EINVAL;
goto fail;
}
diff --git a/block/quorum.c b/block/quorum.c
index e846a7e892..2ebe0ba16d 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -900,7 +900,6 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
Error **errp)
{
BDRVQuorumState *s = bs->opaque;
- Error *local_err = NULL;
QemuOpts *opts = NULL;
const char *pattern_str;
bool *opened;
@@ -978,9 +977,8 @@ static int quorum_open(BlockDriverState *bs, QDict *options, int flags,
s->children[i] = bdrv_open_child(NULL, options, indexstr, bs,
&child_of_bds, BDRV_CHILD_DATA, false,
- &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ errp);
+ if (!s->children[i]) {
ret = -EINVAL;
goto close_exit;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v3 04/13] blockdev: fix drive_backup_prepare() missed error
2020-10-16 17:10 [PATCH v3 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2020-10-16 17:10 ` [PATCH v3 03/13] block: check return value of bdrv_open_child and drop error propagation Vladimir Sementsov-Ogievskiy
@ 2020-10-16 17:10 ` Vladimir Sementsov-Ogievskiy
2020-10-16 17:10 ` [PATCH v3 05/13] block: drop extra error propagation for bdrv_set_backing_hd Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
12 siblings, 0 replies; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-16 17:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, den, Greg Kurz
We leak local_err and don't report failure to the caller. It's
definitely wrong, let's fix.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
blockdev.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 3a896181fd..db59be7c68 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1823,8 +1823,7 @@ static void drive_backup_prepare(BlkActionState *common, Error **errp)
aio_context_acquire(aio_context);
if (set_backing_hd) {
- bdrv_set_backing_hd(target_bs, source, &local_err);
- if (local_err) {
+ if (bdrv_set_backing_hd(target_bs, source, errp) < 0) {
goto unref;
}
}
--
2.21.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v3 05/13] block: drop extra error propagation for bdrv_set_backing_hd
2020-10-16 17:10 [PATCH v3 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2020-10-16 17:10 ` [PATCH v3 04/13] blockdev: fix drive_backup_prepare() missed error Vladimir Sementsov-Ogievskiy
@ 2020-10-16 17:10 ` Vladimir Sementsov-Ogievskiy
2020-10-16 17:10 ` [PATCH v3 06/13] block/mirror: drop extra error propagation in commit_active_start() Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
12 siblings, 0 replies; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-16 17:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, den, Greg Kurz
bdrv_set_backing_hd now returns status, let's use it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index 7b6818c681..a35dc80dd4 100644
--- a/block.c
+++ b/block.c
@@ -3016,11 +3016,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
/* Hook up the backing file link; drop our reference, bs owns the
* backing_hd reference now */
- bdrv_set_backing_hd(bs, backing_hd, &local_err);
+ ret = bdrv_set_backing_hd(bs, backing_hd, errp);
bdrv_unref(backing_hd);
- if (local_err) {
- error_propagate(errp, local_err);
- ret = -EINVAL;
+ if (ret < 0) {
goto free_exit;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v3 06/13] block/mirror: drop extra error propagation in commit_active_start()
2020-10-16 17:10 [PATCH v3 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2020-10-16 17:10 ` [PATCH v3 05/13] block: drop extra error propagation for bdrv_set_backing_hd Vladimir Sementsov-Ogievskiy
@ 2020-10-16 17:10 ` Vladimir Sementsov-Ogievskiy
2020-10-16 17:10 ` [PATCH v3 07/13] blockjob: return status from block_job_set_speed() Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
12 siblings, 0 replies; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-16 17:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, den, Greg Kurz
Let's check return value of mirror_start_job to check for failure
instead of local_err.
Rename ret to job, as ret is usually integer variable.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/mirror.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index b3778248b8..f7c624d6a9 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1851,8 +1851,7 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
bool auto_complete, Error **errp)
{
bool base_read_only;
- Error *local_err = NULL;
- BlockJob *ret;
+ BlockJob *job;
base_read_only = bdrv_is_read_only(base);
@@ -1862,19 +1861,18 @@ BlockJob *commit_active_start(const char *job_id, BlockDriverState *bs,
}
}
- ret = mirror_start_job(
+ job = mirror_start_job(
job_id, bs, creation_flags, base, NULL, speed, 0, 0,
MIRROR_LEAVE_BACKING_CHAIN, false,
on_error, on_error, true, cb, opaque,
&commit_active_job_driver, false, base, auto_complete,
filter_node_name, false, MIRROR_COPY_MODE_BACKGROUND,
- &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ errp);
+ if (!job) {
goto error_restore_flags;
}
- return ret;
+ return job;
error_restore_flags:
/* ignore error and errp for bdrv_reopen, because we want to propagate
--
2.21.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v3 07/13] blockjob: return status from block_job_set_speed()
2020-10-16 17:10 [PATCH v3 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2020-10-16 17:10 ` [PATCH v3 06/13] block/mirror: drop extra error propagation in commit_active_start() Vladimir Sementsov-Ogievskiy
@ 2020-10-16 17:10 ` Vladimir Sementsov-Ogievskiy
2020-10-16 17:10 ` [PATCH v3 08/13] block/qcow2: qcow2_get_specific_info(): drop error propagation Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
12 siblings, 0 replies; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-16 17:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, den, Greg Kurz
Better to return status together with setting errp. It allows to avoid
error propagation in the caller.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
include/block/blockjob.h | 2 +-
blockjob.c | 18 ++++++++----------
2 files changed, 9 insertions(+), 11 deletions(-)
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 35faa3aa26..d200f33c10 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -139,7 +139,7 @@ bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs);
* Set a rate-limiting parameter for the job; the actual meaning may
* vary depending on the job type.
*/
-void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
+bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp);
/**
* block_job_query:
diff --git a/blockjob.c b/blockjob.c
index 98ac8af982..bba96a89ba 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -254,28 +254,30 @@ static bool job_timer_pending(Job *job)
return timer_pending(&job->sleep_timer);
}
-void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
+bool block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
{
int64_t old_speed = job->speed;
- if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp)) {
- return;
+ if (job_apply_verb(&job->job, JOB_VERB_SET_SPEED, errp) < 0) {
+ return false;
}
if (speed < 0) {
error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "speed",
"a non-negative value");
- return;
+ return false;
}
ratelimit_set_speed(&job->limit, speed, BLOCK_JOB_SLICE_TIME);
job->speed = speed;
if (speed && speed <= old_speed) {
- return;
+ return true;
}
/* kick only if a timer is pending */
job_enter_cond(&job->job, job_timer_pending);
+
+ return true;
}
int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n)
@@ -448,12 +450,8 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
/* Only set speed when necessary to avoid NotSupported error */
if (speed != 0) {
- Error *local_err = NULL;
-
- block_job_set_speed(job, speed, &local_err);
- if (local_err) {
+ if (!block_job_set_speed(job, speed, errp)) {
job_early_fail(&job->job);
- error_propagate(errp, local_err);
return NULL;
}
}
--
2.21.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v3 08/13] block/qcow2: qcow2_get_specific_info(): drop error propagation
2020-10-16 17:10 [PATCH v3 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2020-10-16 17:10 ` [PATCH v3 07/13] blockjob: return status from block_job_set_speed() Vladimir Sementsov-Ogievskiy
@ 2020-10-16 17:10 ` Vladimir Sementsov-Ogievskiy
2020-10-16 17:10 ` [PATCH v3 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
12 siblings, 0 replies; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-16 17:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, den, Greg Kurz
Don't use error propagation in qcow2_get_specific_info(). For this
refactor qcow2_get_bitmap_info_list, its current interface is rather
weird.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/qcow2.h | 4 ++--
block/qcow2-bitmap.c | 28 ++++++++++++++--------------
block/qcow2.c | 10 +++-------
3 files changed, 19 insertions(+), 23 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 125ea9679b..024901a6ca 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -976,8 +976,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size);
bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
-Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
- Error **errp);
+bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
+ Qcow2BitmapInfoList **info_list, Error **errp);
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index d7a31a8ddc..55cd52ab96 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1093,30 +1093,30 @@ static Qcow2BitmapInfoFlagsList *get_bitmap_info_flags(uint32_t flags)
/*
* qcow2_get_bitmap_info_list()
* Returns a list of QCOW2 bitmap details.
- * In case of no bitmaps, the function returns NULL and
- * the @errp parameter is not set.
- * When bitmap information can not be obtained, the function returns
- * NULL and the @errp parameter is set.
+ * On success return true with info_list set (note, that if there are no
+ * bitmaps, info_list is set to NULL).
+ * On failure return false with errp set.
*/
-Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
- Error **errp)
+bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
+ Qcow2BitmapInfoList **info_list, Error **errp)
{
BDRVQcow2State *s = bs->opaque;
Qcow2BitmapList *bm_list;
Qcow2Bitmap *bm;
- Qcow2BitmapInfoList *list = NULL;
- Qcow2BitmapInfoList **plist = &list;
if (s->nb_bitmaps == 0) {
- return NULL;
+ *info_list = NULL;
+ return true;
}
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
- if (bm_list == NULL) {
- return NULL;
+ if (!bm_list) {
+ return false;
}
+ *info_list = NULL;
+
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
Qcow2BitmapInfo *info = g_new0(Qcow2BitmapInfo, 1);
Qcow2BitmapInfoList *obj = g_new0(Qcow2BitmapInfoList, 1);
@@ -1124,13 +1124,13 @@ Qcow2BitmapInfoList *qcow2_get_bitmap_info_list(BlockDriverState *bs,
info->name = g_strdup(bm->name);
info->flags = get_bitmap_info_flags(bm->flags & ~BME_RESERVED_FLAGS);
obj->value = info;
- *plist = obj;
- plist = &obj->next;
+ *info_list = obj;
+ info_list = &obj->next;
}
bitmap_list_free(bm_list);
- return list;
+ return true;
}
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
diff --git a/block/qcow2.c b/block/qcow2.c
index 41a29072e6..8c89c98978 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5038,12 +5038,10 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
BDRVQcow2State *s = bs->opaque;
ImageInfoSpecific *spec_info;
QCryptoBlockInfo *encrypt_info = NULL;
- Error *local_err = NULL;
if (s->crypto != NULL) {
- encrypt_info = qcrypto_block_get_info(s->crypto, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ encrypt_info = qcrypto_block_get_info(s->crypto, errp);
+ if (!encrypt_info) {
return NULL;
}
}
@@ -5060,9 +5058,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
};
} else if (s->qcow_version == 3) {
Qcow2BitmapInfoList *bitmaps;
- bitmaps = qcow2_get_bitmap_info_list(bs, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (!qcow2_get_bitmap_info_list(bs, &bitmaps, errp)) {
qapi_free_ImageInfoSpecific(spec_info);
qapi_free_QCryptoBlockInfo(encrypt_info);
return NULL;
--
2.21.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v3 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface
2020-10-16 17:10 [PATCH v3 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2020-10-16 17:10 ` [PATCH v3 08/13] block/qcow2: qcow2_get_specific_info(): drop error propagation Vladimir Sementsov-Ogievskiy
@ 2020-10-16 17:10 ` Vladimir Sementsov-Ogievskiy
2020-10-16 17:10 ` [PATCH v3 10/13] block/qcow2-bitmap: return status from qcow2_store_persistent_dirty_bitmaps Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
12 siblings, 0 replies; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-16 17:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, den, Greg Kurz
It's recommended for bool functions with errp to return true on success
and false on failure. Non-standard interfaces don't help to understand
the code. The change is also needed to reduce error propagation.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
block/qcow2.h | 3 ++-
block/qcow2-bitmap.c | 26 +++++++++++++++-----------
block/qcow2.c | 6 ++----
3 files changed, 19 insertions(+), 16 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 024901a6ca..4c994739ed 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -975,7 +975,8 @@ void qcow2_cache_discard(Qcow2Cache *c, void *table);
int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size);
-bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
+ Error **errp);
bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
Qcow2BitmapInfoList **info_list, Error **errp);
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 55cd52ab96..ca01f08bac 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -962,25 +962,27 @@ static void set_readonly_helper(gpointer bitmap, gpointer value)
bdrv_dirty_bitmap_set_readonly(bitmap, (bool)value);
}
-/* qcow2_load_dirty_bitmaps()
- * Return value is a hint for caller: true means that the Qcow2 header was
- * updated. (false doesn't mean that the header should be updated by the
- * caller, it just means that updating was not needed or the image cannot be
- * written to).
- * On failure the function returns false.
+/*
+ * Return true on success, false on failure.
+ * If header_updated is not NULL then it is set appropriately regardless of
+ * the return value.
*/
-bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
+ Error **errp)
{
BDRVQcow2State *s = bs->opaque;
Qcow2BitmapList *bm_list;
Qcow2Bitmap *bm;
GSList *created_dirty_bitmaps = NULL;
- bool header_updated = false;
bool needs_update = false;
+ if (header_updated) {
+ *header_updated = false;
+ }
+
if (s->nb_bitmaps == 0) {
/* No bitmaps - nothing to do */
- return false;
+ return true;
}
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
@@ -1036,7 +1038,9 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
error_setg_errno(errp, -ret, "Can't update bitmap directory");
goto fail;
}
- header_updated = true;
+ if (header_updated) {
+ *header_updated = true;
+ }
}
if (!can_write(bs)) {
@@ -1047,7 +1051,7 @@ bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
g_slist_free(created_dirty_bitmaps);
bitmap_list_free(bm_list);
- return header_updated;
+ return true;
fail:
g_slist_foreach(created_dirty_bitmaps, release_dirty_bitmap_helper, bs);
diff --git a/block/qcow2.c b/block/qcow2.c
index 8c89c98978..c4b86df7c0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1297,7 +1297,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
unsigned int len, i;
int ret = 0;
QCowHeader header;
- Error *local_err = NULL;
uint64_t ext_end;
uint64_t l1_vm_state_index;
bool update_header = false;
@@ -1785,9 +1784,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
/* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer. */
- bool header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
- if (local_err != NULL) {
- error_propagate(errp, local_err);
+ bool header_updated;
+ if (!qcow2_load_dirty_bitmaps(bs, &header_updated, errp)) {
ret = -EINVAL;
goto fail;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v3 10/13] block/qcow2-bitmap: return status from qcow2_store_persistent_dirty_bitmaps
2020-10-16 17:10 [PATCH v3 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2020-10-16 17:10 ` [PATCH v3 09/13] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface Vladimir Sementsov-Ogievskiy
@ 2020-10-16 17:10 ` Vladimir Sementsov-Ogievskiy
2020-10-16 17:10 ` [PATCH v3 11/13] block/qcow2: read_cache_sizes: return status value Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
12 siblings, 0 replies; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-16 17:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, den, Greg Kurz
It's better to return status together with setting errp. It makes
possible to avoid error propagation.
While being here, put ERRP_GUARD() to fix error_prepend(errp, ...)
usage inside qcow2_store_persistent_dirty_bitmaps() (see the comment
above ERRP_GUARD() definition in include/qapi/error.h)
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/qcow2.h | 2 +-
block/qcow2-bitmap.c | 13 ++++++-------
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 4c994739ed..467cfd4779 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -981,7 +981,7 @@ bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
Qcow2BitmapInfoList **info_list, Error **errp);
int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
int qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
bool release_stored, Error **errp);
int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
bool qcow2_co_can_store_new_dirty_bitmap(BlockDriverState *bs,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index ca01f08bac..9eccaab7f8 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1536,9 +1536,10 @@ out:
* readonly to begin with, and whether we opened directly or reopened to that
* state shouldn't matter for the state we get afterward.
*/
-void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
+bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
bool release_stored, Error **errp)
{
+ ERRP_GUARD();
BdrvDirtyBitmap *bitmap;
BDRVQcow2State *s = bs->opaque;
uint32_t new_nb_bitmaps = s->nb_bitmaps;
@@ -1558,7 +1559,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
if (bm_list == NULL) {
- return;
+ return false;
}
}
@@ -1673,7 +1674,7 @@ success:
}
bitmap_list_free(bm_list);
- return;
+ return true;
fail:
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
@@ -1691,16 +1692,14 @@ fail:
}
bitmap_list_free(bm_list);
+ return false;
}
int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp)
{
BdrvDirtyBitmap *bitmap;
- Error *local_err = NULL;
- qcow2_store_persistent_dirty_bitmaps(bs, false, &local_err);
- if (local_err != NULL) {
- error_propagate(errp, local_err);
+ if (!qcow2_store_persistent_dirty_bitmaps(bs, false, errp)) {
return -EINVAL;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v3 11/13] block/qcow2: read_cache_sizes: return status value
2020-10-16 17:10 [PATCH v3 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (9 preceding siblings ...)
2020-10-16 17:10 ` [PATCH v3 10/13] block/qcow2-bitmap: return status from qcow2_store_persistent_dirty_bitmaps Vladimir Sementsov-Ogievskiy
@ 2020-10-16 17:10 ` Vladimir Sementsov-Ogievskiy
2020-10-16 17:10 ` [PATCH v3 12/13] block/qcow2: simplify qcow2_co_invalidate_cache() Vladimir Sementsov-Ogievskiy
2020-10-16 17:10 ` [PATCH v3 13/13] block/qed: bdrv_qed_do_open: deal with errp Vladimir Sementsov-Ogievskiy
12 siblings, 0 replies; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-16 17:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, den, Greg Kurz
It's better to return status together with setting errp. It allows to
reduce error propagation.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
block/qcow2.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index c4b86df7c0..2b6ec4b757 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -869,7 +869,7 @@ static void qcow2_attach_aio_context(BlockDriverState *bs,
cache_clean_timer_init(bs, new_context);
}
-static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
+static bool read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
uint64_t *l2_cache_size,
uint64_t *l2_cache_entry_size,
uint64_t *refcount_cache_size, Error **errp)
@@ -907,16 +907,16 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
error_setg(errp, QCOW2_OPT_CACHE_SIZE ", " QCOW2_OPT_L2_CACHE_SIZE
" and " QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not be set "
"at the same time");
- return;
+ return false;
} else if (l2_cache_size_set &&
(l2_cache_max_setting > combined_cache_size)) {
error_setg(errp, QCOW2_OPT_L2_CACHE_SIZE " may not exceed "
QCOW2_OPT_CACHE_SIZE);
- return;
+ return false;
} else if (*refcount_cache_size > combined_cache_size) {
error_setg(errp, QCOW2_OPT_REFCOUNT_CACHE_SIZE " may not exceed "
QCOW2_OPT_CACHE_SIZE);
- return;
+ return false;
}
if (l2_cache_size_set) {
@@ -955,8 +955,10 @@ static void read_cache_sizes(BlockDriverState *bs, QemuOpts *opts,
error_setg(errp, "L2 cache entry size must be a power of two "
"between %d and the cluster size (%d)",
1 << MIN_CLUSTER_BITS, s->cluster_size);
- return;
+ return false;
}
+
+ return true;
}
typedef struct Qcow2ReopenState {
@@ -983,7 +985,6 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
int i;
const char *encryptfmt;
QDict *encryptopts = NULL;
- Error *local_err = NULL;
int ret;
qdict_extract_subqdict(options, &encryptopts, "encrypt.");
@@ -996,10 +997,8 @@ static int qcow2_update_options_prepare(BlockDriverState *bs,
}
/* get L2 table/refcount block cache size from command line options */
- read_cache_sizes(bs, opts, &l2_cache_size, &l2_cache_entry_size,
- &refcount_cache_size, &local_err);
- if (local_err) {
- error_propagate(errp, local_err);
+ if (!read_cache_sizes(bs, opts, &l2_cache_size, &l2_cache_entry_size,
+ &refcount_cache_size, errp)) {
ret = -EINVAL;
goto fail;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* [PATCH v3 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()
2020-10-16 17:10 [PATCH v3 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (10 preceding siblings ...)
2020-10-16 17:10 ` [PATCH v3 11/13] block/qcow2: read_cache_sizes: return status value Vladimir Sementsov-Ogievskiy
@ 2020-10-16 17:10 ` Vladimir Sementsov-Ogievskiy
2020-10-19 13:06 ` Kevin Wolf
2020-10-16 17:10 ` [PATCH v3 13/13] block/qed: bdrv_qed_do_open: deal with errp Vladimir Sementsov-Ogievskiy
12 siblings, 1 reply; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-16 17:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, den, Greg Kurz
qcow2_do_open correctly sets errp on each failure path. So, we can
simplify code in qcow2_co_invalidate_cache() and drop explicit error
propagation.
Add ERRP_GUARD() as mandated by the documentation in
include/qapi/error.h so that error_prepend() is actually called even if
errp is &error_fatal.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Greg Kurz <groug@kaod.org>
---
block/qcow2.c | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 2b6ec4b757..cd5f48d3fb 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2702,11 +2702,11 @@ static void qcow2_close(BlockDriverState *bs)
static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
Error **errp)
{
+ ERRP_GUARD();
BDRVQcow2State *s = bs->opaque;
int flags = s->flags;
QCryptoBlock *crypto = NULL;
QDict *options;
- Error *local_err = NULL;
int ret;
/*
@@ -2724,16 +2724,11 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
flags &= ~BDRV_O_INACTIVE;
qemu_co_mutex_lock(&s->lock);
- ret = qcow2_do_open(bs, options, flags, &local_err);
+ ret = qcow2_do_open(bs, options, flags, errp);
qemu_co_mutex_unlock(&s->lock);
qobject_unref(options);
- if (local_err) {
- error_propagate_prepend(errp, local_err,
- "Could not reopen qcow2 layer: ");
- bs->drv = NULL;
- return;
- } else if (ret < 0) {
- error_setg_errno(errp, -ret, "Could not reopen qcow2 layer");
+ if (ret < 0) {
+ error_prepend(errp, "Could not reopen qcow2 layer: ");
bs->drv = NULL;
return;
}
--
2.21.3
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v3 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()
2020-10-16 17:10 ` [PATCH v3 12/13] block/qcow2: simplify qcow2_co_invalidate_cache() Vladimir Sementsov-Ogievskiy
@ 2020-10-19 13:06 ` Kevin Wolf
2020-10-20 12:46 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 20+ messages in thread
From: Kevin Wolf @ 2020-10-19 13:06 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru, Greg Kurz,
stefanha, den, pbonzini, mreitz, jsnow, ari
Am 16.10.2020 um 19:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
> qcow2_do_open correctly sets errp on each failure path. So, we can
> simplify code in qcow2_co_invalidate_cache() and drop explicit error
> propagation.
qcow2_update_options_prepare() can return -EINVAL without setting errp:
if (s->crypt_method_header != QCOW_CRYPT_NONE && !r->crypto_opts) {
ret = -EINVAL;
goto fail;
}
This is called indirectly from qcow2_do_open().
Kevin
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v3 12/13] block/qcow2: simplify qcow2_co_invalidate_cache()
2020-10-19 13:06 ` Kevin Wolf
@ 2020-10-20 12:46 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-20 12:46 UTC (permalink / raw)
To: Kevin Wolf
Cc: qemu-block, qemu-devel, armbru, berto, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, den, Greg Kurz
19.10.2020 16:06, Kevin Wolf wrote:
> Am 16.10.2020 um 19:10 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> qcow2_do_open correctly sets errp on each failure path. So, we can
>> simplify code in qcow2_co_invalidate_cache() and drop explicit error
>> propagation.
>
> qcow2_update_options_prepare() can return -EINVAL without setting errp:
>
> if (s->crypt_method_header != QCOW_CRYPT_NONE && !r->crypto_opts) {
> ret = -EINVAL;
> goto fail;
> }
>
Actually, it's set in previous "switch", either in "default:" branch or if block_crypto_open_opts_init() failed.. But the code pattern is far from being obvious, I'll add a patch to refactor.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 13/13] block/qed: bdrv_qed_do_open: deal with errp
2020-10-16 17:10 [PATCH v3 00/13] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (11 preceding siblings ...)
2020-10-16 17:10 ` [PATCH v3 12/13] block/qcow2: simplify qcow2_co_invalidate_cache() Vladimir Sementsov-Ogievskiy
@ 2020-10-16 17:10 ` Vladimir Sementsov-Ogievskiy
12 siblings, 0 replies; 20+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-10-16 17:10 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, den, Greg Kurz
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: ");
}
}
--
2.21.3
^ permalink raw reply related [flat|nested] 20+ messages in thread