* [PATCH 01/14] block: return status from bdrv_append and friends
2020-09-09 18:59 [PATCH 00/14] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
@ 2020-09-09 18:59 ` Vladimir Sementsov-Ogievskiy
2020-09-10 15:51 ` Greg Kurz
2020-09-17 14:13 ` Alberto Garcia
2020-09-09 18:59 ` [PATCH 02/14] block: use return status of bdrv_append() Vladimir Sementsov-Ogievskiy
` (12 subsequent siblings)
13 siblings, 2 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-09 18:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
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>
---
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 6e36154061..03b3cee8f8 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -336,10 +336,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);
@@ -351,8 +351,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 2ba76b2c36..6d35449027 100644
--- a/block.c
+++ b/block.c
@@ -2866,14 +2866,15 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)
* Sets the backing file 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, backing_bs(bs), errp)) {
- return;
+ return -EPERM;
}
if (backing_hd) {
@@ -2891,15 +2892,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;
}
/*
@@ -4517,8 +4525,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;
@@ -4540,6 +4548,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;
@@ -4575,6 +4584,8 @@ out:
g_slist_free(list);
bdrv_drained_end(from);
bdrv_unref(from);
+
+ return ret;
}
/*
@@ -4593,20 +4604,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;
}
@@ -4615,6 +4622,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] 57+ messages in thread* Re: [PATCH 01/14] block: return status from bdrv_append and friends
2020-09-09 18:59 ` [PATCH 01/14] block: return status from bdrv_append and friends Vladimir Sementsov-Ogievskiy
@ 2020-09-10 15:51 ` Greg Kurz
2020-09-17 14:13 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Greg Kurz @ 2020-09-10 15:51 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed, 9 Sep 2020 21:59:17 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 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>
> 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 6e36154061..03b3cee8f8 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -336,10 +336,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);
> @@ -351,8 +351,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 2ba76b2c36..6d35449027 100644
> --- a/block.c
> +++ b/block.c
> @@ -2866,14 +2866,15 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)
> * Sets the backing file 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, backing_bs(bs), errp)) {
> - return;
> + return -EPERM;
> }
>
> if (backing_hd) {
> @@ -2891,15 +2892,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;
> }
>
> /*
> @@ -4517,8 +4525,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;
> @@ -4540,6 +4548,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;
> @@ -4575,6 +4584,8 @@ out:
> g_slist_free(list);
> bdrv_drained_end(from);
> bdrv_unref(from);
> +
> + return ret;
> }
>
> /*
> @@ -4593,20 +4604,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;
> }
> @@ -4615,6 +4622,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)
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 01/14] block: return status from bdrv_append and friends
2020-09-09 18:59 ` [PATCH 01/14] block: return status from bdrv_append and friends Vladimir Sementsov-Ogievskiy
2020-09-10 15:51 ` Greg Kurz
@ 2020-09-17 14:13 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Alberto Garcia @ 2020-09-17 14:13 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed 09 Sep 2020 08:59:17 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 02/14] block: use return status of bdrv_append()
2020-09-09 18:59 [PATCH 00/14] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
2020-09-09 18:59 ` [PATCH 01/14] block: return status from bdrv_append and friends Vladimir Sementsov-Ogievskiy
@ 2020-09-09 18:59 ` Vladimir Sementsov-Ogievskiy
2020-09-10 16:10 ` Greg Kurz
2020-09-17 13:59 ` Alberto Garcia
2020-09-09 18:59 ` [PATCH 03/14] block: check return value of bdrv_open_child and drop error propagation Vladimir Sementsov-Ogievskiy
` (11 subsequent siblings)
13 siblings, 2 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-09 18:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
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>
---
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 6d35449027..9b624b2535 100644
--- a/block.c
+++ b/block.c
@@ -3156,7 +3156,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
@@ -3203,9 +3202,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 af2f20f346..de9d5e1634 100644
--- a/block/backup-top.c
+++ b/block/backup-top.c
@@ -192,7 +192,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;
@@ -225,9 +225,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;
@@ -237,18 +236,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;
@@ -266,7 +263,6 @@ fail:
}
bdrv_drained_end(source);
- error_propagate(errp, local_err);
return NULL;
}
diff --git a/block/commit.c b/block/commit.c
index 7732d02dfe..7720d4729b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -253,7 +253,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
CommitBlockJob *s;
BlockDriverState *iter;
BlockDriverState *commit_top_bs = NULL;
- Error *local_err = NULL;
int ret;
assert(top != bs);
@@ -292,10 +291,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 e8e8844afc..ca250f765d 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1557,7 +1557,6 @@ static BlockJob *mirror_start_job(
BlockDriverState *mirror_top_bs;
bool target_graph_mod;
bool target_is_backing;
- Error *local_err = NULL;
int ret;
if (granularity == 0) {
@@ -1606,12 +1605,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 3848a9c8ab..36bef6b188 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1576,9 +1576,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] 57+ messages in thread* Re: [PATCH 02/14] block: use return status of bdrv_append()
2020-09-09 18:59 ` [PATCH 02/14] block: use return status of bdrv_append() Vladimir Sementsov-Ogievskiy
@ 2020-09-10 16:10 ` Greg Kurz
2020-09-17 19:21 ` Vladimir Sementsov-Ogievskiy
2020-09-17 13:59 ` Alberto Garcia
1 sibling, 1 reply; 57+ messages in thread
From: Greg Kurz @ 2020-09-10 16:10 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed, 9 Sep 2020 21:59:18 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 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>
Just one suggestion for a follow-up below...
> 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 6d35449027..9b624b2535 100644
> --- a/block.c
> +++ b/block.c
> @@ -3156,7 +3156,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
> @@ -3203,9 +3202,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 af2f20f346..de9d5e1634 100644
> --- a/block/backup-top.c
> +++ b/block/backup-top.c
> @@ -192,7 +192,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;
> @@ -225,9 +225,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;
> @@ -237,18 +236,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;
> @@ -266,7 +263,6 @@ fail:
> }
>
> bdrv_drained_end(source);
> - error_propagate(errp, local_err);
>
> return NULL;
> }
> diff --git a/block/commit.c b/block/commit.c
> index 7732d02dfe..7720d4729b 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -253,7 +253,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
> CommitBlockJob *s;
> BlockDriverState *iter;
> BlockDriverState *commit_top_bs = NULL;
> - Error *local_err = NULL;
> int ret;
>
... this is unrelated but while reviewing I've noticed that the ret
variable isn't really needed.
> assert(top != bs);
> @@ -292,10 +291,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 e8e8844afc..ca250f765d 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1557,7 +1557,6 @@ static BlockJob *mirror_start_job(
> BlockDriverState *mirror_top_bs;
> bool target_graph_mod;
> bool target_is_backing;
> - Error *local_err = NULL;
> int ret;
>
> if (granularity == 0) {
> @@ -1606,12 +1605,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 3848a9c8ab..36bef6b188 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1576,9 +1576,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);
> }
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 02/14] block: use return status of bdrv_append()
2020-09-10 16:10 ` Greg Kurz
@ 2020-09-17 19:21 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 19:21 UTC (permalink / raw)
To: Greg Kurz
Cc: qemu-block, qemu-devel, armbru, berto, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf
10.09.2020 19:10, Greg Kurz wrote:
> On Wed, 9 Sep 2020 21:59:18 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>
>> 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>
>
> Just one suggestion for a follow-up below...
>
>> block.c | 5 +----
>> block/backup-top.c | 20 ++++++++------------
[..]
>> @@ -253,7 +253,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
>> CommitBlockJob *s;
>> BlockDriverState *iter;
>> BlockDriverState *commit_top_bs = NULL;
>> - Error *local_err = NULL;
>> int ret;
>>
>
> ... this is unrelated but while reviewing I've noticed that the ret
> variable isn't really needed.
>
Looking at this now, I'm not quite agreed. I think that avoiding multi-line if conditions is a good reason for additional variables, like this ret. A kind of taste of course, so if you want you may post a patch, but I don't want do it :)
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 02/14] block: use return status of bdrv_append()
2020-09-09 18:59 ` [PATCH 02/14] block: use return status of bdrv_append() Vladimir Sementsov-Ogievskiy
2020-09-10 16:10 ` Greg Kurz
@ 2020-09-17 13:59 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Alberto Garcia @ 2020-09-17 13:59 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed 09 Sep 2020 08:59:18 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 03/14] block: check return value of bdrv_open_child and drop error propagation
2020-09-09 18:59 [PATCH 00/14] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
2020-09-09 18:59 ` [PATCH 01/14] block: return status from bdrv_append and friends Vladimir Sementsov-Ogievskiy
2020-09-09 18:59 ` [PATCH 02/14] block: use return status of bdrv_append() Vladimir Sementsov-Ogievskiy
@ 2020-09-09 18:59 ` Vladimir Sementsov-Ogievskiy
2020-09-10 16:21 ` Greg Kurz
2020-09-17 14:47 ` Alberto Garcia
2020-09-09 18:59 ` [PATCH 04/14] blockdev: fix drive_backup_prepare() missed error Vladimir Sementsov-Ogievskiy
` (10 subsequent siblings)
13 siblings, 2 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-09 18:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
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>
---
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 9c08d8a005..61aaee9879 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -464,7 +464,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;
@@ -494,10 +493,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 57315f56b4..7ef046cee9 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 da56b1a4df..10175fa399 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1613,9 +1613,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 6df9449fc2..672b09e13d 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -898,7 +898,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;
@@ -976,9 +975,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] 57+ messages in thread* Re: [PATCH 03/14] block: check return value of bdrv_open_child and drop error propagation
2020-09-09 18:59 ` [PATCH 03/14] block: check return value of bdrv_open_child and drop error propagation Vladimir Sementsov-Ogievskiy
@ 2020-09-10 16:21 ` Greg Kurz
2020-09-17 14:47 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Greg Kurz @ 2020-09-10 16:21 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed, 9 Sep 2020 21:59:19 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 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>
> 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 9c08d8a005..61aaee9879 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -464,7 +464,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;
>
> @@ -494,10 +493,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 57315f56b4..7ef046cee9 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 da56b1a4df..10175fa399 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1613,9 +1613,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 6df9449fc2..672b09e13d 100644
> --- a/block/quorum.c
> +++ b/block/quorum.c
> @@ -898,7 +898,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;
> @@ -976,9 +975,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;
> }
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 03/14] block: check return value of bdrv_open_child and drop error propagation
2020-09-09 18:59 ` [PATCH 03/14] block: check return value of bdrv_open_child and drop error propagation Vladimir Sementsov-Ogievskiy
2020-09-10 16:21 ` Greg Kurz
@ 2020-09-17 14:47 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Alberto Garcia @ 2020-09-17 14:47 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed 09 Sep 2020 08:59:19 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 04/14] blockdev: fix drive_backup_prepare() missed error
2020-09-09 18:59 [PATCH 00/14] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2020-09-09 18:59 ` [PATCH 03/14] block: check return value of bdrv_open_child and drop error propagation Vladimir Sementsov-Ogievskiy
@ 2020-09-09 18:59 ` Vladimir Sementsov-Ogievskiy
2020-09-10 16:26 ` Greg Kurz
2020-09-17 13:56 ` Alberto Garcia
2020-09-09 18:59 ` [PATCH 05/14] block: drop extra error propagation for bdrv_set_backing_hd Vladimir Sementsov-Ogievskiy
` (9 subsequent siblings)
13 siblings, 2 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-09 18:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
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>
---
blockdev.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 36bef6b188..74259527c1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1797,8 +1797,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] 57+ messages in thread* Re: [PATCH 04/14] blockdev: fix drive_backup_prepare() missed error
2020-09-09 18:59 ` [PATCH 04/14] blockdev: fix drive_backup_prepare() missed error Vladimir Sementsov-Ogievskiy
@ 2020-09-10 16:26 ` Greg Kurz
2020-09-17 13:56 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Greg Kurz @ 2020-09-10 16:26 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed, 9 Sep 2020 21:59:20 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 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>
> blockdev.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/blockdev.c b/blockdev.c
> index 36bef6b188..74259527c1 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -1797,8 +1797,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;
> }
> }
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 04/14] blockdev: fix drive_backup_prepare() missed error
2020-09-09 18:59 ` [PATCH 04/14] blockdev: fix drive_backup_prepare() missed error Vladimir Sementsov-Ogievskiy
2020-09-10 16:26 ` Greg Kurz
@ 2020-09-17 13:56 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Alberto Garcia @ 2020-09-17 13:56 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed 09 Sep 2020 08:59:20 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 05/14] block: drop extra error propagation for bdrv_set_backing_hd
2020-09-09 18:59 [PATCH 00/14] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (3 preceding siblings ...)
2020-09-09 18:59 ` [PATCH 04/14] blockdev: fix drive_backup_prepare() missed error Vladimir Sementsov-Ogievskiy
@ 2020-09-09 18:59 ` Vladimir Sementsov-Ogievskiy
2020-09-10 16:30 ` Greg Kurz
2020-09-17 13:57 ` Alberto Garcia
2020-09-09 18:59 ` [PATCH 06/14] block/mirror: drop extra error propagation in commit_active_start() Vladimir Sementsov-Ogievskiy
` (8 subsequent siblings)
13 siblings, 2 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-09 18:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
bdrv_set_backing_hd now returns status, let's use it.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index 9b624b2535..c5e3a1927e 100644
--- a/block.c
+++ b/block.c
@@ -3011,11 +3011,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] 57+ messages in thread* Re: [PATCH 05/14] block: drop extra error propagation for bdrv_set_backing_hd
2020-09-09 18:59 ` [PATCH 05/14] block: drop extra error propagation for bdrv_set_backing_hd Vladimir Sementsov-Ogievskiy
@ 2020-09-10 16:30 ` Greg Kurz
2020-09-17 13:57 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Greg Kurz @ 2020-09-10 16:30 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed, 9 Sep 2020 21:59:21 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 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>
> block.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/block.c b/block.c
> index 9b624b2535..c5e3a1927e 100644
> --- a/block.c
> +++ b/block.c
> @@ -3011,11 +3011,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;
> }
>
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 05/14] block: drop extra error propagation for bdrv_set_backing_hd
2020-09-09 18:59 ` [PATCH 05/14] block: drop extra error propagation for bdrv_set_backing_hd Vladimir Sementsov-Ogievskiy
2020-09-10 16:30 ` Greg Kurz
@ 2020-09-17 13:57 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Alberto Garcia @ 2020-09-17 13:57 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed 09 Sep 2020 08:59:21 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> bdrv_set_backing_hd now returns status, let's use it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 06/14] block/mirror: drop extra error propagation in commit_active_start()
2020-09-09 18:59 [PATCH 00/14] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (4 preceding siblings ...)
2020-09-09 18:59 ` [PATCH 05/14] block: drop extra error propagation for bdrv_set_backing_hd Vladimir Sementsov-Ogievskiy
@ 2020-09-09 18:59 ` Vladimir Sementsov-Ogievskiy
2020-09-10 16:32 ` Greg Kurz
2020-09-17 14:48 ` Alberto Garcia
2020-09-09 18:59 ` [PATCH 07/14] block/blklogwrites: drop error propagation Vladimir Sementsov-Ogievskiy
` (7 subsequent siblings)
13 siblings, 2 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-09 18:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
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>
---
block/mirror.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index ca250f765d..529ba12b2a 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1788,8 +1788,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);
@@ -1799,19 +1798,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] 57+ messages in thread* Re: [PATCH 06/14] block/mirror: drop extra error propagation in commit_active_start()
2020-09-09 18:59 ` [PATCH 06/14] block/mirror: drop extra error propagation in commit_active_start() Vladimir Sementsov-Ogievskiy
@ 2020-09-10 16:32 ` Greg Kurz
2020-09-17 14:48 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Greg Kurz @ 2020-09-10 16:32 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed, 9 Sep 2020 21:59:22 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 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>
> block/mirror.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index ca250f765d..529ba12b2a 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1788,8 +1788,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);
>
> @@ -1799,19 +1798,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
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 06/14] block/mirror: drop extra error propagation in commit_active_start()
2020-09-09 18:59 ` [PATCH 06/14] block/mirror: drop extra error propagation in commit_active_start() Vladimir Sementsov-Ogievskiy
2020-09-10 16:32 ` Greg Kurz
@ 2020-09-17 14:48 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Alberto Garcia @ 2020-09-17 14:48 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed 09 Sep 2020 08:59:22 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 07/14] block/blklogwrites: drop error propagation
2020-09-09 18:59 [PATCH 00/14] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (5 preceding siblings ...)
2020-09-09 18:59 ` [PATCH 06/14] block/mirror: drop extra error propagation in commit_active_start() Vladimir Sementsov-Ogievskiy
@ 2020-09-09 18:59 ` Vladimir Sementsov-Ogievskiy
2020-09-10 17:24 ` Greg Kurz
2020-09-10 21:01 ` Ari Sundholm
2020-09-09 18:59 ` [PATCH 08/14] blockjob: return status from block_job_set_speed() Vladimir Sementsov-Ogievskiy
` (6 subsequent siblings)
13 siblings, 2 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-09 18:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
It's simple to avoid error propagation in blk_log_writes_open(), we
just need to refactor blk_log_writes_find_cur_log_sector() a bit.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/blklogwrites.c | 23 +++++++++++------------
1 file changed, 11 insertions(+), 12 deletions(-)
diff --git a/block/blklogwrites.c b/block/blklogwrites.c
index 7ef046cee9..c7da507b2d 100644
--- a/block/blklogwrites.c
+++ b/block/blklogwrites.c
@@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
sector_size < (1ull << 24);
}
-static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
- uint32_t sector_size,
- uint64_t nr_entries,
- Error **errp)
+static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
+ uint32_t sector_size,
+ uint64_t nr_entries,
+ Error **errp)
{
uint64_t cur_sector = 1;
uint64_t cur_idx = 0;
@@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
if (read_ret < 0) {
error_setg_errno(errp, -read_ret,
"Failed to read log entry %"PRIu64, cur_idx);
- return (uint64_t)-1ull;
+ return read_ret;
}
if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
le64_to_cpu(cur_entry.flags), cur_idx);
- return (uint64_t)-1ull;
+ return -EINVAL;
}
/* Account for the sector of the entry itself */
@@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
{
BDRVBlkLogWritesState *s = bs->opaque;
QemuOpts *opts;
- Error *local_err = NULL;
int ret;
uint64_t log_sector_size;
bool log_append;
@@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
s->nr_entries = 0;
if (blk_log_writes_sector_size_valid(log_sector_size)) {
- s->cur_log_sector =
+ int64_t cur_log_sector =
blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
- le64_to_cpu(log_sb.nr_entries), &local_err);
- if (local_err) {
- ret = -EINVAL;
- error_propagate(errp, local_err);
+ le64_to_cpu(log_sb.nr_entries), errp);
+ if (cur_log_sector < 0) {
+ ret = cur_log_sector;
goto fail_log;
}
+ s->cur_log_sector = cur_log_sector;
s->nr_entries = le64_to_cpu(log_sb.nr_entries);
}
} else {
--
2.21.3
^ permalink raw reply related [flat|nested] 57+ messages in thread* Re: [PATCH 07/14] block/blklogwrites: drop error propagation
2020-09-09 18:59 ` [PATCH 07/14] block/blklogwrites: drop error propagation Vladimir Sementsov-Ogievskiy
@ 2020-09-10 17:24 ` Greg Kurz
2020-09-11 5:23 ` Markus Armbruster
2020-09-10 21:01 ` Ari Sundholm
1 sibling, 1 reply; 57+ messages in thread
From: Greg Kurz @ 2020-09-10 17:24 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed, 9 Sep 2020 21:59:23 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> It's simple to avoid error propagation in blk_log_writes_open(), we
> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/blklogwrites.c | 23 +++++++++++------------
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
> index 7ef046cee9..c7da507b2d 100644
> --- a/block/blklogwrites.c
> +++ b/block/blklogwrites.c
> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
> sector_size < (1ull << 24);
> }
>
> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> - uint32_t sector_size,
> - uint64_t nr_entries,
> - Error **errp)
> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> + uint32_t sector_size,
> + uint64_t nr_entries,
> + Error **errp)
> {
> uint64_t cur_sector = 1;
> uint64_t cur_idx = 0;
> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> if (read_ret < 0) {
> error_setg_errno(errp, -read_ret,
> "Failed to read log entry %"PRIu64, cur_idx);
> - return (uint64_t)-1ull;
> + return read_ret;
This changes the error status of blk_log_writes_open() from -EINVAL to
whatever is returned by bdrv_pread(). I guess this is an improvement
worth to be mentioned in the changelog.
> }
>
> if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
> error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
> le64_to_cpu(cur_entry.flags), cur_idx);
> - return (uint64_t)-1ull;
> + return -EINVAL;
> }
>
> /* Account for the sector of the entry itself */
> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
> {
> BDRVBlkLogWritesState *s = bs->opaque;
> QemuOpts *opts;
> - Error *local_err = NULL;
> int ret;
> uint64_t log_sector_size;
> bool log_append;
> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
> s->nr_entries = 0;
>
> if (blk_log_writes_sector_size_valid(log_sector_size)) {
> - s->cur_log_sector =
> + int64_t cur_log_sector =
> blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
> - le64_to_cpu(log_sb.nr_entries), &local_err);
> - if (local_err) {
> - ret = -EINVAL;
> - error_propagate(errp, local_err);
> + le64_to_cpu(log_sb.nr_entries), errp);
> + if (cur_log_sector < 0) {
> + ret = cur_log_sector;
This is converting int64_t to int, which is usually not recommended.
In practice this is probably okay because cur_log_sector is supposed
to be a negative errno (ie. an int) in this case.
Maybe make this clear with a an assert(cur_log_sector >= INT_MIN) ?
> goto fail_log;
> }
>
> + s->cur_log_sector = cur_log_sector;
> s->nr_entries = le64_to_cpu(log_sb.nr_entries);
> }
> } else {
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 07/14] block/blklogwrites: drop error propagation
2020-09-10 17:24 ` Greg Kurz
@ 2020-09-11 5:23 ` Markus Armbruster
2020-09-11 5:30 ` Markus Armbruster
0 siblings, 1 reply; 57+ messages in thread
From: Markus Armbruster @ 2020-09-11 5:23 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, Vladimir Sementsov-Ogievskiy, berto, stefanha, qemu-block,
qemu-devel, mreitz, pavel.dovgaluk, pbonzini, jsnow, ari
Greg Kurz <groug@kaod.org> writes:
> On Wed, 9 Sep 2020 21:59:23 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>
>> It's simple to avoid error propagation in blk_log_writes_open(), we
>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> block/blklogwrites.c | 23 +++++++++++------------
>> 1 file changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>> index 7ef046cee9..c7da507b2d 100644
>> --- a/block/blklogwrites.c
>> +++ b/block/blklogwrites.c
>> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
>> sector_size < (1ull << 24);
>> }
>>
>> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>> - uint32_t sector_size,
>> - uint64_t nr_entries,
>> - Error **errp)
>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>> + uint32_t sector_size,
>> + uint64_t nr_entries,
>> + Error **errp)
>> {
>> uint64_t cur_sector = 1;
>> uint64_t cur_idx = 0;
>> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>> if (read_ret < 0) {
>> error_setg_errno(errp, -read_ret,
>> "Failed to read log entry %"PRIu64, cur_idx);
>> - return (uint64_t)-1ull;
>> + return read_ret;
>
> This changes the error status of blk_log_writes_open() from -EINVAL to
> whatever is returned by bdrv_pread(). I guess this is an improvement
> worth to be mentioned in the changelog.
>
>> }
>>
>> if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>> error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
>> le64_to_cpu(cur_entry.flags), cur_idx);
>> - return (uint64_t)-1ull;
>> + return -EINVAL;
>> }
>>
>> /* Account for the sector of the entry itself */
>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>> {
>> BDRVBlkLogWritesState *s = bs->opaque;
>> QemuOpts *opts;
>> - Error *local_err = NULL;
>> int ret;
>> uint64_t log_sector_size;
>> bool log_append;
>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>> s->nr_entries = 0;
>>
>> if (blk_log_writes_sector_size_valid(log_sector_size)) {
>> - s->cur_log_sector =
>> + int64_t cur_log_sector =
>> blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
>> - le64_to_cpu(log_sb.nr_entries), &local_err);
>> - if (local_err) {
>> - ret = -EINVAL;
>> - error_propagate(errp, local_err);
>> + le64_to_cpu(log_sb.nr_entries), errp);
>> + if (cur_log_sector < 0) {
>> + ret = cur_log_sector;
>
> This is converting int64_t to int, which is usually not recommended.
> In practice this is probably okay because cur_log_sector is supposed
> to be a negative errno (ie. an int) in this case.
It isn't: blk_log_writes_find_cur_log_sector() returns (uint64_t)-1ull
on error.
Aside: returning uint64_t is awkward. We commonly use a signed integer
for non-negative offset or negative error.
> Maybe make this clear with a an assert(cur_log_sector >= INT_MIN) ?
Unless we change blk_log_writes_find_cur_log_sector() to return a
negative errno code, we need to ret = -EINVAL here. Let's keep this
series simple.
>> goto fail_log;
>> }
>>
>> + s->cur_log_sector = cur_log_sector;
>> s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>> }
>> } else {
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 07/14] block/blklogwrites: drop error propagation
2020-09-11 5:23 ` Markus Armbruster
@ 2020-09-11 5:30 ` Markus Armbruster
2020-09-11 6:19 ` Greg Kurz
0 siblings, 1 reply; 57+ messages in thread
From: Markus Armbruster @ 2020-09-11 5:30 UTC (permalink / raw)
To: Markus Armbruster
Cc: kwolf, Vladimir Sementsov-Ogievskiy, berto, stefanha, qemu-block,
Greg Kurz, qemu-devel, pavel.dovgaluk, pbonzini, mreitz, jsnow,
ari
Markus Armbruster <armbru@redhat.com> writes:
> Greg Kurz <groug@kaod.org> writes:
>
>> On Wed, 9 Sep 2020 21:59:23 +0300
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>
>>> It's simple to avoid error propagation in blk_log_writes_open(), we
>>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>> block/blklogwrites.c | 23 +++++++++++------------
>>> 1 file changed, 11 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>>> index 7ef046cee9..c7da507b2d 100644
>>> --- a/block/blklogwrites.c
>>> +++ b/block/blklogwrites.c
>>> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
>>> sector_size < (1ull << 24);
>>> }
>>>
>>> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>> - uint32_t sector_size,
>>> - uint64_t nr_entries,
>>> - Error **errp)
>>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>> + uint32_t sector_size,
>>> + uint64_t nr_entries,
>>> + Error **errp)
>>> {
>>> uint64_t cur_sector = 1;
>>> uint64_t cur_idx = 0;
>>> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>> if (read_ret < 0) {
>>> error_setg_errno(errp, -read_ret,
>>> "Failed to read log entry %"PRIu64, cur_idx);
>>> - return (uint64_t)-1ull;
>>> + return read_ret;
>>
>> This changes the error status of blk_log_writes_open() from -EINVAL to
>> whatever is returned by bdrv_pread(). I guess this is an improvement
>> worth to be mentioned in the changelog.
>>
>>> }
>>>
>>> if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>>> error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
>>> le64_to_cpu(cur_entry.flags), cur_idx);
>>> - return (uint64_t)-1ull;
>>> + return -EINVAL;
>>> }
>>>
>>> /* Account for the sector of the entry itself */
>>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>> {
>>> BDRVBlkLogWritesState *s = bs->opaque;
>>> QemuOpts *opts;
>>> - Error *local_err = NULL;
>>> int ret;
>>> uint64_t log_sector_size;
>>> bool log_append;
>>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>> s->nr_entries = 0;
>>>
>>> if (blk_log_writes_sector_size_valid(log_sector_size)) {
>>> - s->cur_log_sector =
>>> + int64_t cur_log_sector =
>>> blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
>>> - le64_to_cpu(log_sb.nr_entries), &local_err);
>>> - if (local_err) {
>>> - ret = -EINVAL;
>>> - error_propagate(errp, local_err);
>>> + le64_to_cpu(log_sb.nr_entries), errp);
>>> + if (cur_log_sector < 0) {
>>> + ret = cur_log_sector;
>>
>> This is converting int64_t to int, which is usually not recommended.
>> In practice this is probably okay because cur_log_sector is supposed
>> to be a negative errno (ie. an int) in this case.
>
> It isn't: blk_log_writes_find_cur_log_sector() returns (uint64_t)-1ull
> on error.
>
> Aside: returning uint64_t is awkward. We commonly use a signed integer
> for non-negative offset or negative error.
>
>> Maybe make this clear with a an assert(cur_log_sector >= INT_MIN) ?
>
> Unless we change blk_log_writes_find_cur_log_sector() to return a
> negative errno code,
Which the patch does. I shouldn't review patches before breakfast.
> negative errno code, we need to ret = -EINVAL here. Let's keep this
> series simple.
No, the patch is okay as is.
Dumbing it down to keep it simple would also be okay.
Regarding the proposed assertion: do we protect similar conversions from
over-wide negative errno int elsewhere?
>>> goto fail_log;
>>> }
>>>
>>> + s->cur_log_sector = cur_log_sector;
>>> s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>>> }
>>> } else {
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 07/14] block/blklogwrites: drop error propagation
2020-09-11 5:30 ` Markus Armbruster
@ 2020-09-11 6:19 ` Greg Kurz
2020-09-11 14:43 ` Markus Armbruster
0 siblings, 1 reply; 57+ messages in thread
From: Greg Kurz @ 2020-09-11 6:19 UTC (permalink / raw)
To: Markus Armbruster
Cc: kwolf, Vladimir Sementsov-Ogievskiy, berto, stefanha, qemu-block,
qemu-devel, mreitz, pavel.dovgaluk, pbonzini, jsnow, ari
On Fri, 11 Sep 2020 07:30:37 +0200
Markus Armbruster <armbru@redhat.com> wrote:
> Markus Armbruster <armbru@redhat.com> writes:
>
> > Greg Kurz <groug@kaod.org> writes:
> >
> >> On Wed, 9 Sep 2020 21:59:23 +0300
> >> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> >>
> >>> It's simple to avoid error propagation in blk_log_writes_open(), we
> >>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
> >>>
> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> >>> ---
> >>> block/blklogwrites.c | 23 +++++++++++------------
> >>> 1 file changed, 11 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
> >>> index 7ef046cee9..c7da507b2d 100644
> >>> --- a/block/blklogwrites.c
> >>> +++ b/block/blklogwrites.c
> >>> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
> >>> sector_size < (1ull << 24);
> >>> }
> >>>
> >>> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> >>> - uint32_t sector_size,
> >>> - uint64_t nr_entries,
> >>> - Error **errp)
> >>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> >>> + uint32_t sector_size,
> >>> + uint64_t nr_entries,
> >>> + Error **errp)
> >>> {
> >>> uint64_t cur_sector = 1;
> >>> uint64_t cur_idx = 0;
> >>> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> >>> if (read_ret < 0) {
> >>> error_setg_errno(errp, -read_ret,
> >>> "Failed to read log entry %"PRIu64, cur_idx);
> >>> - return (uint64_t)-1ull;
> >>> + return read_ret;
> >>
> >> This changes the error status of blk_log_writes_open() from -EINVAL to
> >> whatever is returned by bdrv_pread(). I guess this is an improvement
> >> worth to be mentioned in the changelog.
> >>
> >>> }
> >>>
> >>> if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
> >>> error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
> >>> le64_to_cpu(cur_entry.flags), cur_idx);
> >>> - return (uint64_t)-1ull;
> >>> + return -EINVAL;
> >>> }
> >>>
> >>> /* Account for the sector of the entry itself */
> >>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
> >>> {
> >>> BDRVBlkLogWritesState *s = bs->opaque;
> >>> QemuOpts *opts;
> >>> - Error *local_err = NULL;
> >>> int ret;
> >>> uint64_t log_sector_size;
> >>> bool log_append;
> >>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
> >>> s->nr_entries = 0;
> >>>
> >>> if (blk_log_writes_sector_size_valid(log_sector_size)) {
> >>> - s->cur_log_sector =
> >>> + int64_t cur_log_sector =
> >>> blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
> >>> - le64_to_cpu(log_sb.nr_entries), &local_err);
> >>> - if (local_err) {
> >>> - ret = -EINVAL;
> >>> - error_propagate(errp, local_err);
> >>> + le64_to_cpu(log_sb.nr_entries), errp);
> >>> + if (cur_log_sector < 0) {
> >>> + ret = cur_log_sector;
> >>
> >> This is converting int64_t to int, which is usually not recommended.
> >> In practice this is probably okay because cur_log_sector is supposed
> >> to be a negative errno (ie. an int) in this case.
> >
> > It isn't: blk_log_writes_find_cur_log_sector() returns (uint64_t)-1ull
> > on error.
> >
> > Aside: returning uint64_t is awkward. We commonly use a signed integer
> > for non-negative offset or negative error.
> >
> >> Maybe make this clear with a an assert(cur_log_sector >= INT_MIN) ?
> >
> > Unless we change blk_log_writes_find_cur_log_sector() to return a
> > negative errno code,
>
> Which the patch does. I shouldn't review patches before breakfast.
>
:)
> > negative errno code, we need to ret = -EINVAL here. Let's keep this
> > series simple.
>
> No, the patch is okay as is.
>
> Dumbing it down to keep it simple would also be okay.
>
What about Ari's proposal to add ERRP_GUARD() and check errors
with "if (*errp)" like we do for void functions ?
> Regarding the proposed assertion: do we protect similar conversions from
> over-wide negative errno int elsewhere?
>
We do protect int64_t->int conversions in a few places, eg.
qcow2_write_snapshots() or qemu_spice_create_host_primary(),
but I couldn't find one about a negarive errno.
> >>> goto fail_log;
> >>> }
> >>>
> >>> + s->cur_log_sector = cur_log_sector;
> >>> s->nr_entries = le64_to_cpu(log_sb.nr_entries);
> >>> }
> >>> } else {
>
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 07/14] block/blklogwrites: drop error propagation
2020-09-11 6:19 ` Greg Kurz
@ 2020-09-11 14:43 ` Markus Armbruster
0 siblings, 0 replies; 57+ messages in thread
From: Markus Armbruster @ 2020-09-11 14:43 UTC (permalink / raw)
To: Greg Kurz
Cc: kwolf, Vladimir Sementsov-Ogievskiy, berto, pavel.dovgaluk,
qemu-block, qemu-devel, mreitz, stefanha, pbonzini, jsnow, ari
Greg Kurz <groug@kaod.org> writes:
> On Fri, 11 Sep 2020 07:30:37 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
[...]
>> No, the patch is okay as is.
>>
>> Dumbing it down to keep it simple would also be okay.
>>
>
> What about Ari's proposal to add ERRP_GUARD() and check errors
> with "if (*errp)" like we do for void functions ?
Up to the maintainer. I prefer this patch.
>> Regarding the proposed assertion: do we protect similar conversions from
>> over-wide negative errno int elsewhere?
>>
>
> We do protect int64_t->int conversions in a few places, eg.
> qcow2_write_snapshots() or qemu_spice_create_host_primary(),
> but I couldn't find one about a negarive errno.
Then I'd not protect it here, either.
>> >>> goto fail_log;
>> >>> }
>> >>>
>> >>> + s->cur_log_sector = cur_log_sector;
>> >>> s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>> >>> }
>> >>> } else {
>>
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 07/14] block/blklogwrites: drop error propagation
2020-09-09 18:59 ` [PATCH 07/14] block/blklogwrites: drop error propagation Vladimir Sementsov-Ogievskiy
2020-09-10 17:24 ` Greg Kurz
@ 2020-09-10 21:01 ` Ari Sundholm
2020-09-11 8:03 ` Markus Armbruster
1 sibling, 1 reply; 57+ messages in thread
From: Ari Sundholm @ 2020-09-10 21:01 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, berto, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow
Hi Vladimir!
Thank you for working on this. My comments below.
On 9/9/20 9:59 PM, Vladimir Sementsov-Ogievskiy wrote:
> It's simple to avoid error propagation in blk_log_writes_open(), we
> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/blklogwrites.c | 23 +++++++++++------------
> 1 file changed, 11 insertions(+), 12 deletions(-)
>
> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
> index 7ef046cee9..c7da507b2d 100644
> --- a/block/blklogwrites.c
> +++ b/block/blklogwrites.c
> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
> sector_size < (1ull << 24);
> }
>
> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> - uint32_t sector_size,
> - uint64_t nr_entries,
> - Error **errp)
> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
I'd rather not change the return type for reasons detailed below.
> + uint32_t sector_size,
> + uint64_t nr_entries,
> + Error **errp)
> {
> uint64_t cur_sector = 1;
> uint64_t cur_idx = 0;
> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
> if (read_ret < 0) {
> error_setg_errno(errp, -read_ret,
> "Failed to read log entry %"PRIu64, cur_idx);
> - return (uint64_t)-1ull;
> + return read_ret;
This is OK, provided the change in return type signedness is necessary
in the first place.
> }
>
> if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
> error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
> le64_to_cpu(cur_entry.flags), cur_idx);
> - return (uint64_t)-1ull;
> + return -EINVAL;
This is OK, provided the return type signedness change is necessary,
although we already do have errp to indicate any errors.
> }
>
> /* Account for the sector of the entry itself */
> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
> {
> BDRVBlkLogWritesState *s = bs->opaque;
> QemuOpts *opts;
> - Error *local_err = NULL;
> int ret;
> uint64_t log_sector_size;
> bool log_append;
> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
> s->nr_entries = 0;
>
> if (blk_log_writes_sector_size_valid(log_sector_size)) {
> - s->cur_log_sector =
> + int64_t cur_log_sector =
> blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
> - le64_to_cpu(log_sb.nr_entries), &local_err);
> - if (local_err) {
> - ret = -EINVAL;
> - error_propagate(errp, local_err);
> + le64_to_cpu(log_sb.nr_entries), errp);
> + if (cur_log_sector < 0) {
> + ret = cur_log_sector;
This changes the semantics slightly. Changing the return type to int64
may theoretically cause valid return values to be interpreted as errors.
Since we already do have a dedicated out pointer for the error value
(errp), why not use it?
Assigning cur_log_sector to ret looks a bit odd to me. Why not use the
original -EINVAL - or maybe some more appropriate errno value than -EINVAL?
I think the desired effect of this change could be achieved with less
churn. How about just making just the following change in addition to
adding ERRP_GUARD() to the beginning of the function and getting rid of
local_err:
- le64_to_cpu(log_sb.nr_entries),
&local_err);
+ le64_to_cpu(log_sb.nr_entries), errp);
- if (local_err) {
+ if (*errp) {
ret = -EINVAL;
- error_propagate(errp, local_err);
> goto fail_log;
> }
>
> + s->cur_log_sector = cur_log_sector;
> s->nr_entries = le64_to_cpu(log_sb.nr_entries);
> }
> } else {
>
Best regards,
Ari Sundholm
ari@tuxera.com
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 07/14] block/blklogwrites: drop error propagation
2020-09-10 21:01 ` Ari Sundholm
@ 2020-09-11 8:03 ` Markus Armbruster
2020-09-16 16:52 ` Ari Sundholm
0 siblings, 1 reply; 57+ messages in thread
From: Markus Armbruster @ 2020-09-11 8:03 UTC (permalink / raw)
To: Ari Sundholm
Cc: kwolf, Vladimir Sementsov-Ogievskiy, berto, stefanha, qemu-block,
qemu-devel, groug, pavel.dovgaluk, pbonzini, mreitz, jsnow
Ari Sundholm <ari@tuxera.com> writes:
> Hi Vladimir!
>
> Thank you for working on this. My comments below.
>
> On 9/9/20 9:59 PM, Vladimir Sementsov-Ogievskiy wrote:
>> It's simple to avoid error propagation in blk_log_writes_open(), we
>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> <vsementsov@virtuozzo.com>
>> ---
>> block/blklogwrites.c | 23 +++++++++++------------
>> 1 file changed, 11 insertions(+), 12 deletions(-)
>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>> index 7ef046cee9..c7da507b2d 100644
>> --- a/block/blklogwrites.c
>> +++ b/block/blklogwrites.c
>> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
>> sector_size < (1ull << 24);
>> }
>> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild
>> *log,
>> - uint32_t sector_size,
>> - uint64_t nr_entries,
>> - Error **errp)
>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>
> I'd rather not change the return type for reasons detailed below.
>
>> + uint32_t sector_size,
>> + uint64_t nr_entries,
>> + Error **errp)
>> {
>> uint64_t cur_sector = 1;
>> uint64_t cur_idx = 0;
>> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>> if (read_ret < 0) {
>> error_setg_errno(errp, -read_ret,
>> "Failed to read log entry %"PRIu64, cur_idx);
>> - return (uint64_t)-1ull;
>> + return read_ret;
>
> This is OK, provided the change in return type signedness is necessary
> in the first place.
>
>> }
>> if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>> error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
>> le64_to_cpu(cur_entry.flags), cur_idx);
>> - return (uint64_t)-1ull;
>> + return -EINVAL;
>
> This is OK, provided the return type signedness change is necessary,
> although we already do have errp to indicate any errors.
>
>> }
>> /* Account for the sector of the entry itself */
>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>> {
>> BDRVBlkLogWritesState *s = bs->opaque;
>> QemuOpts *opts;
>> - Error *local_err = NULL;
>> int ret;
>> uint64_t log_sector_size;
>> bool log_append;
>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>> s->nr_entries = 0;
>> if (blk_log_writes_sector_size_valid(log_sector_size)) {
>> - s->cur_log_sector =
>> + int64_t cur_log_sector =
>> blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
>> - le64_to_cpu(log_sb.nr_entries), &local_err);
>> - if (local_err) {
>> - ret = -EINVAL;
>> - error_propagate(errp, local_err);
>> + le64_to_cpu(log_sb.nr_entries), errp);
>> + if (cur_log_sector < 0) {
>> + ret = cur_log_sector;
>
> This changes the semantics slightly. Changing the return type to int64
> may theoretically cause valid return values to be interpreted as
> errors. Since we already do have a dedicated out pointer for the error
> value (errp), why not use it?
Error.h's big comment:
* Error reporting system loosely patterned after Glib's GError.
*
* = Rules =
[...]
* - Whenever practical, also return a value that indicates success /
* failure. This can make the error checking more concise, and can
* avoid useless error object creation and destruction. Note that
* we still have many functions returning void. We recommend
* • bool-valued functions return true on success / false on failure,
* • pointer-valued functions return non-null / null pointer, and
* • integer-valued functions return non-negative / negative.
blk_log_writes_find_cur_log_sector() does return such an error value
before the patch: (uint64_t)-1.
The caller does not use it to check for errors. It uses @err instead.
Awkward, has to error_propagate().
Avoiding error_propagate() reduces the error handling boileplate. It
also improves behavior when @errp is &error_abort: we get the abort
right where the error happens instead of where we propagate it.
Furthermore, caller has to make an error code (-EINVAL), because
returning (uint64_t)-1 throws it away. Yes, a detailed error is stored
into @err, but you can't cleanly extract the error code.
Using a signed integer for returning "non-negative offset or negative
errno code" is pervasive, starting with read() and write(). It hasn't
been a problem there, and it hasn't been a problem in the block layer.
8 exbi-blocks should do for a while. Should it become troublesome, we
won't solve the problem by going unsigned and adding one bit, we'll
double the width and add 64.
Additional rationale for returning recognizable error values:
commit e3fe3988d7851cac30abffae06d2f555ff7bee62
Author: Markus Armbruster <armbru@redhat.com>
Date: Tue Jul 7 18:05:31 2020 +0200
error: Document Error API usage rules
This merely codifies existing practice, with one exception: the rule
advising against returning void, where existing practice is mixed.
When the Error API was created, we adopted the (unwritten) rule to
return void when the function returns no useful value on success,
unlike GError, which recommends to return true on success and false on
error then.
When a function returns a distinct error value, say false, a checked
call that passes the error up looks like
if (!frobnicate(..., errp)) {
handle the error...
}
When it returns void, we need
Error *err = NULL;
frobnicate(..., &err);
if (err) {
handle the error...
error_propagate(errp, err);
}
Not only is this more verbose, it also creates an Error object even
when @errp is null, &error_abort or &error_fatal.
People got tired of the additional boilerplate, and started to ignore
the unwritten rule. The result is confusion among developers about
the preferred usage.
Make the rule advising against returning void official by putting it
in writing. This will hopefully reduce confusion.
[...]
Additional rationale for avoiding error_propagate():
commit ae7c80a7bd73685437bf6ba9d7c26098351f4166
Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Date: Tue Jul 7 18:50:30 2020 +0200
error: New macro ERRP_GUARD()
Introduce a new ERRP_GUARD() macro, to be used at start of functions
with an errp OUT parameter.
It has three goals:
1. Fix issue with error_fatal and error_prepend/error_append_hint: the
user can't see this additional information, because exit() happens in
error_setg earlier than information is added. [Reported by Greg Kurz]
2. Fix issue with error_abort and error_propagate: when we wrap
error_abort by local_err+error_propagate, the resulting coredump will
refer to error_propagate and not to the place where error happened.
(the macro itself doesn't fix the issue, but it allows us to [3.] drop
the local_err+error_propagate pattern, which will definitely fix the
issue) [Reported by Kevin Wolf]
3. Drop local_err+error_propagate pattern, which is used to workaround
void functions with errp parameter, when caller wants to know resulting
status. (Note: actually these functions could be merely updated to
return int error code).
[...]
> Assigning cur_log_sector to ret looks a bit odd to me. Why not use the
> original -EINVAL - or maybe some more appropriate errno value than
> -EINVAL?
We assign the error code returned by
blk_log_writes_find_cur_log_sector(), which seems proper to me.
Care to suggest a better variable name?
> I think the desired effect of this change could be achieved with less
> churn. How about just making just the following change in addition to
> adding ERRP_GUARD() to the beginning of the function and getting rid
> of local_err:
>
> - le64_to_cpu(log_sb.nr_entries),
> &local_err);
> + le64_to_cpu(log_sb.nr_entries), errp);
> - if (local_err) {
> + if (*errp) {
> ret = -EINVAL;
> - error_propagate(errp, local_err);
>> goto fail_log;
>> }
>> + s->cur_log_sector = cur_log_sector;
>> s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>> }
>> } else {
I prefer this patch. But it's up to the block maintainers.
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 07/14] block/blklogwrites: drop error propagation
2020-09-11 8:03 ` Markus Armbruster
@ 2020-09-16 16:52 ` Ari Sundholm
2020-09-17 7:12 ` Markus Armbruster
0 siblings, 1 reply; 57+ messages in thread
From: Ari Sundholm @ 2020-09-16 16:52 UTC (permalink / raw)
To: Markus Armbruster
Cc: kwolf, Vladimir Sementsov-Ogievskiy, berto, stefanha, qemu-block,
qemu-devel, groug, pavel.dovgaluk, pbonzini, mreitz, jsnow
Hi,
On 9/11/20 11:03 AM, Markus Armbruster wrote:
> Ari Sundholm <ari@tuxera.com> writes:
>
>> Hi Vladimir!
>>
>> Thank you for working on this. My comments below.
>>
>> On 9/9/20 9:59 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> It's simple to avoid error propagation in blk_log_writes_open(), we
>>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>> <vsementsov@virtuozzo.com>
>>> ---
>>> block/blklogwrites.c | 23 +++++++++++------------
>>> 1 file changed, 11 insertions(+), 12 deletions(-)
>>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>>> index 7ef046cee9..c7da507b2d 100644
>>> --- a/block/blklogwrites.c
>>> +++ b/block/blklogwrites.c
>>> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
>>> sector_size < (1ull << 24);
>>> }
>>> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild
>>> *log,
>>> - uint32_t sector_size,
>>> - uint64_t nr_entries,
>>> - Error **errp)
>>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>
>> I'd rather not change the return type for reasons detailed below.
>>
>>> + uint32_t sector_size,
>>> + uint64_t nr_entries,
>>> + Error **errp)
>>> {
>>> uint64_t cur_sector = 1;
>>> uint64_t cur_idx = 0;
>>> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>> if (read_ret < 0) {
>>> error_setg_errno(errp, -read_ret,
>>> "Failed to read log entry %"PRIu64, cur_idx);
>>> - return (uint64_t)-1ull;
>>> + return read_ret;
>>
>> This is OK, provided the change in return type signedness is necessary
>> in the first place.
>>
>>> }
>>> if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>>> error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
>>> le64_to_cpu(cur_entry.flags), cur_idx);
>>> - return (uint64_t)-1ull;
>>> + return -EINVAL;
>>
>> This is OK, provided the return type signedness change is necessary,
>> although we already do have errp to indicate any errors.
>>
>>> }
>>> /* Account for the sector of the entry itself */
>>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>> {
>>> BDRVBlkLogWritesState *s = bs->opaque;
>>> QemuOpts *opts;
>>> - Error *local_err = NULL;
>>> int ret;
>>> uint64_t log_sector_size;
>>> bool log_append;
>>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>> s->nr_entries = 0;
>>> if (blk_log_writes_sector_size_valid(log_sector_size)) {
>>> - s->cur_log_sector =
>>> + int64_t cur_log_sector =
>>> blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
>>> - le64_to_cpu(log_sb.nr_entries), &local_err);
>>> - if (local_err) {
>>> - ret = -EINVAL;
>>> - error_propagate(errp, local_err);
>>> + le64_to_cpu(log_sb.nr_entries), errp);
>>> + if (cur_log_sector < 0) {
>>> + ret = cur_log_sector;
>>
>> This changes the semantics slightly. Changing the return type to int64
>> may theoretically cause valid return values to be interpreted as
>> errors. Since we already do have a dedicated out pointer for the error
>> value (errp), why not use it?
>
> Error.h's big comment:
>
> * Error reporting system loosely patterned after Glib's GError.
> *
> * = Rules =
> [...]
> * - Whenever practical, also return a value that indicates success /
> * failure. This can make the error checking more concise, and can
> * avoid useless error object creation and destruction. Note that
> * we still have many functions returning void. We recommend
> * • bool-valued functions return true on success / false on failure,
> * • pointer-valued functions return non-null / null pointer, and
> * • integer-valued functions return non-negative / negative.
>
> blk_log_writes_find_cur_log_sector() does return such an error value
> before the patch: (uint64_t)-1.
>
> The caller does not use it to check for errors. It uses @err instead.
> Awkward, has to error_propagate().
>
> Avoiding error_propagate() reduces the error handling boileplate. It
> also improves behavior when @errp is &error_abort: we get the abort
> right where the error happens instead of where we propagate it.
>
> Furthermore, caller has to make an error code (-EINVAL), because
> returning (uint64_t)-1 throws it away. Yes, a detailed error is stored
> into @err, but you can't cleanly extract the error code.
>
> Using a signed integer for returning "non-negative offset or negative
> errno code" is pervasive, starting with read() and write(). It hasn't
> been a problem there, and it hasn't been a problem in the block layer.
> 8 exbi-blocks should do for a while. Should it become troublesome, we
> won't solve the problem by going unsigned and adding one bit, we'll
> double the width and add 64.
>
I am in complete agreement with eliminating error propagation within the
blklogwrites driver. This was never a point of disagreement.
As error propagation is dropped in this patch, the awkwardness referred
to above will be no more, making that a moot point.
My main issue was that this patch does more than just the mechanical
transformation required. Changes that are not strictly necessary are
made, and they slightly change the semantics while duplicating the error
code and halving the range of the return type (instead of just returning
*some* value in the absence of a possibility to return nothing, which
will be thrown away by the caller anyway when an error has occurred).
However, as the consensus seems to be that it is best to change the
return type to int64_t for consistency with the rest of the codebase, I
will not object any further regarding that point. Having conceded that,
this makes the difference between my preferred minimalistic approach and
this patch insignificant, and it is not my intention to needlessly
obstruct a perfectly fine patch series.
BTW, I do not think it would be difficult at all to extend the code in
error.h to make it convenient to extract errno and/or win32 error values
that have been explicitly provided, but this is a matter best discussed
separately and left for a later patch.
As for general matters regarding error handling and separation between
results and errors, I am open to discussing these off-list.
Best regards,
Ari Sundholm
ari@tuxera.com
> Additional rationale for returning recognizable error values:
>
> commit e3fe3988d7851cac30abffae06d2f555ff7bee62
> Author: Markus Armbruster <armbru@redhat.com>
> Date: Tue Jul 7 18:05:31 2020 +0200
>
> error: Document Error API usage rules
>
> This merely codifies existing practice, with one exception: the rule
> advising against returning void, where existing practice is mixed.
>
> When the Error API was created, we adopted the (unwritten) rule to
> return void when the function returns no useful value on success,
> unlike GError, which recommends to return true on success and false on
> error then.
>
> When a function returns a distinct error value, say false, a checked
> call that passes the error up looks like
>
> if (!frobnicate(..., errp)) {
> handle the error...
> }
>
> When it returns void, we need
>
> Error *err = NULL;
>
> frobnicate(..., &err);
> if (err) {
> handle the error...
> error_propagate(errp, err);
> }
>
> Not only is this more verbose, it also creates an Error object even
> when @errp is null, &error_abort or &error_fatal.
>
> People got tired of the additional boilerplate, and started to ignore
> the unwritten rule. The result is confusion among developers about
> the preferred usage.
>
> Make the rule advising against returning void official by putting it
> in writing. This will hopefully reduce confusion.
> [...]
>
> Additional rationale for avoiding error_propagate():
>
> commit ae7c80a7bd73685437bf6ba9d7c26098351f4166
> Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Date: Tue Jul 7 18:50:30 2020 +0200
>
> error: New macro ERRP_GUARD()
>
> Introduce a new ERRP_GUARD() macro, to be used at start of functions
> with an errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal and error_prepend/error_append_hint: the
> user can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
>
> 2. Fix issue with error_abort and error_propagate: when we wrap
> error_abort by local_err+error_propagate, the resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows us to [3.] drop
> the local_err+error_propagate pattern, which will definitely fix the
> issue) [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
> [...]
>
>> Assigning cur_log_sector to ret looks a bit odd to me. Why not use the
>> original -EINVAL - or maybe some more appropriate errno value than
>> -EINVAL?
>
> We assign the error code returned by
> blk_log_writes_find_cur_log_sector(), which seems proper to me.
>
> Care to suggest a better variable name?
>
>> I think the desired effect of this change could be achieved with less
>> churn. How about just making just the following change in addition to
>> adding ERRP_GUARD() to the beginning of the function and getting rid
>> of local_err:
>>
>> - le64_to_cpu(log_sb.nr_entries),
>> &local_err);
>> + le64_to_cpu(log_sb.nr_entries), errp);
>> - if (local_err) {
>> + if (*errp) {
>> ret = -EINVAL;
>> - error_propagate(errp, local_err);
>>> goto fail_log;
>>> }
>>> + s->cur_log_sector = cur_log_sector;
>>> s->nr_entries = le64_to_cpu(log_sb.nr_entries);
>>> }
>>> } else {
>
> I prefer this patch. But it's up to the block maintainers.
>
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 07/14] block/blklogwrites: drop error propagation
2020-09-16 16:52 ` Ari Sundholm
@ 2020-09-17 7:12 ` Markus Armbruster
0 siblings, 0 replies; 57+ messages in thread
From: Markus Armbruster @ 2020-09-17 7:12 UTC (permalink / raw)
To: Ari Sundholm
Cc: kwolf, Vladimir Sementsov-Ogievskiy, berto, pavel.dovgaluk,
qemu-block, Markus Armbruster, qemu-devel, groug, stefanha,
pbonzini, mreitz, jsnow
Ari Sundholm <ari@tuxera.com> writes:
> Hi,
>
> On 9/11/20 11:03 AM, Markus Armbruster wrote:
>> Ari Sundholm <ari@tuxera.com> writes:
>>
>>> Hi Vladimir!
>>>
>>> Thank you for working on this. My comments below.
>>>
>>> On 9/9/20 9:59 PM, Vladimir Sementsov-Ogievskiy wrote:
>>>> It's simple to avoid error propagation in blk_log_writes_open(), we
>>>> just need to refactor blk_log_writes_find_cur_log_sector() a bit.
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>>>> <vsementsov@virtuozzo.com>
>>>> ---
>>>> block/blklogwrites.c | 23 +++++++++++------------
>>>> 1 file changed, 11 insertions(+), 12 deletions(-)
>>>> diff --git a/block/blklogwrites.c b/block/blklogwrites.c
>>>> index 7ef046cee9..c7da507b2d 100644
>>>> --- a/block/blklogwrites.c
>>>> +++ b/block/blklogwrites.c
>>>> @@ -96,10 +96,10 @@ static inline bool blk_log_writes_sector_size_valid(uint32_t sector_size)
>>>> sector_size < (1ull << 24);
>>>> }
>>>> -static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild
>>>> *log,
>>>> - uint32_t sector_size,
>>>> - uint64_t nr_entries,
>>>> - Error **errp)
>>>> +static int64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>>
>>> I'd rather not change the return type for reasons detailed below.
>>>
>>>> + uint32_t sector_size,
>>>> + uint64_t nr_entries,
>>>> + Error **errp)
>>>> {
>>>> uint64_t cur_sector = 1;
>>>> uint64_t cur_idx = 0;
>>>> @@ -112,13 +112,13 @@ static uint64_t blk_log_writes_find_cur_log_sector(BdrvChild *log,
>>>> if (read_ret < 0) {
>>>> error_setg_errno(errp, -read_ret,
>>>> "Failed to read log entry %"PRIu64, cur_idx);
>>>> - return (uint64_t)-1ull;
>>>> + return read_ret;
>>>
>>> This is OK, provided the change in return type signedness is necessary
>>> in the first place.
>>>
>>>> }
>>>> if (cur_entry.flags & ~cpu_to_le64(LOG_FLAG_MASK)) {
>>>> error_setg(errp, "Invalid flags 0x%"PRIx64" in log entry %"PRIu64,
>>>> le64_to_cpu(cur_entry.flags), cur_idx);
>>>> - return (uint64_t)-1ull;
>>>> + return -EINVAL;
>>>
>>> This is OK, provided the return type signedness change is necessary,
>>> although we already do have errp to indicate any errors.
>>>
>>>> }
>>>> /* Account for the sector of the entry itself */
>>>> @@ -143,7 +143,6 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>>> {
>>>> BDRVBlkLogWritesState *s = bs->opaque;
>>>> QemuOpts *opts;
>>>> - Error *local_err = NULL;
>>>> int ret;
>>>> uint64_t log_sector_size;
>>>> bool log_append;
>>>> @@ -215,15 +214,15 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags,
>>>> s->nr_entries = 0;
>>>> if (blk_log_writes_sector_size_valid(log_sector_size)) {
>>>> - s->cur_log_sector =
>>>> + int64_t cur_log_sector =
>>>> blk_log_writes_find_cur_log_sector(s->log_file, log_sector_size,
>>>> - le64_to_cpu(log_sb.nr_entries), &local_err);
>>>> - if (local_err) {
>>>> - ret = -EINVAL;
>>>> - error_propagate(errp, local_err);
>>>> + le64_to_cpu(log_sb.nr_entries), errp);
>>>> + if (cur_log_sector < 0) {
>>>> + ret = cur_log_sector;
>>>
>>> This changes the semantics slightly. Changing the return type to int64
>>> may theoretically cause valid return values to be interpreted as
>>> errors. Since we already do have a dedicated out pointer for the error
>>> value (errp), why not use it?
>> Error.h's big comment:
>> * Error reporting system loosely patterned after Glib's GError.
>> *
>> * = Rules =
>> [...]
>> * - Whenever practical, also return a value that indicates success /
>> * failure. This can make the error checking more concise, and can
>> * avoid useless error object creation and destruction. Note that
>> * we still have many functions returning void. We recommend
>> * • bool-valued functions return true on success / false on failure,
>> * • pointer-valued functions return non-null / null pointer, and
>> * • integer-valued functions return non-negative / negative.
>> blk_log_writes_find_cur_log_sector() does return such an error value
>> before the patch: (uint64_t)-1.
>> The caller does not use it to check for errors. It uses @err
>> instead.
>> Awkward, has to error_propagate().
>> Avoiding error_propagate() reduces the error handling boileplate.
>> It
>> also improves behavior when @errp is &error_abort: we get the abort
>> right where the error happens instead of where we propagate it.
>> Furthermore, caller has to make an error code (-EINVAL), because
>> returning (uint64_t)-1 throws it away. Yes, a detailed error is stored
>> into @err, but you can't cleanly extract the error code.
>> Using a signed integer for returning "non-negative offset or
>> negative
>> errno code" is pervasive, starting with read() and write(). It hasn't
>> been a problem there, and it hasn't been a problem in the block layer.
>> 8 exbi-blocks should do for a while. Should it become troublesome, we
>> won't solve the problem by going unsigned and adding one bit, we'll
>> double the width and add 64.
>>
>
> I am in complete agreement with eliminating error propagation within
> the blklogwrites driver. This was never a point of disagreement.
>
> As error propagation is dropped in this patch, the awkwardness
> referred to above will be no more, making that a moot point.
>
> My main issue was that this patch does more than just the mechanical
> transformation required. Changes that are not strictly necessary are
> made, and they slightly change the semantics while duplicating the
> error code and halving the range of the return type (instead of just
> returning *some* value in the absence of a possibility to return
> nothing, which will be thrown away by the caller anyway when an error
> has occurred).
You have a point: the return type change should perhaps be a separate
patch. Up to the maintainer.
> However, as the consensus seems to be that it is best to change the
> return type to int64_t for consistency with the rest of the codebase,
> I will not object any further regarding that point. Having conceded
> that, this makes the difference between my preferred minimalistic
> approach and this patch insignificant, and it is not my intention to
> needlessly obstruct a perfectly fine patch series.
>
> BTW, I do not think it would be difficult at all to extend the code in
> error.h to make it convenient to extract errno and/or win32 error
> values that have been explicitly provided, but this is a matter best
> discussed separately and left for a later patch.
Certainly feasible, but how would we deal with the fact that only some
Error objects have an error code?
> As for general matters regarding error handling and separation between
> results and errors, I am open to discussing these off-list.
Thanks!
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 08/14] blockjob: return status from block_job_set_speed()
2020-09-09 18:59 [PATCH 00/14] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (6 preceding siblings ...)
2020-09-09 18:59 ` [PATCH 07/14] block/blklogwrites: drop error propagation Vladimir Sementsov-Ogievskiy
@ 2020-09-09 18:59 ` Vladimir Sementsov-Ogievskiy
2020-09-11 8:34 ` Greg Kurz
2020-09-17 14:01 ` Alberto Garcia
2020-09-09 18:59 ` [PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation Vladimir Sementsov-Ogievskiy
` (5 subsequent siblings)
13 siblings, 2 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-09 18:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
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>
---
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 470facfd47..afddf7a1fb 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] 57+ messages in thread* Re: [PATCH 08/14] blockjob: return status from block_job_set_speed()
2020-09-09 18:59 ` [PATCH 08/14] blockjob: return status from block_job_set_speed() Vladimir Sementsov-Ogievskiy
@ 2020-09-11 8:34 ` Greg Kurz
2020-09-17 14:01 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Greg Kurz @ 2020-09-11 8:34 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed, 9 Sep 2020 21:59:24 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 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>
> 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 470facfd47..afddf7a1fb 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;
> }
> }
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 08/14] blockjob: return status from block_job_set_speed()
2020-09-09 18:59 ` [PATCH 08/14] blockjob: return status from block_job_set_speed() Vladimir Sementsov-Ogievskiy
2020-09-11 8:34 ` Greg Kurz
@ 2020-09-17 14:01 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Alberto Garcia @ 2020-09-17 14:01 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed 09 Sep 2020 08:59:24 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation
2020-09-09 18:59 [PATCH 00/14] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (7 preceding siblings ...)
2020-09-09 18:59 ` [PATCH 08/14] blockjob: return status from block_job_set_speed() Vladimir Sementsov-Ogievskiy
@ 2020-09-09 18:59 ` Vladimir Sementsov-Ogievskiy
2020-09-11 8:41 ` Greg Kurz
2020-09-17 16:32 ` Alberto Garcia
2020-09-09 18:59 ` [PATCH 10/14] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface Vladimir Sementsov-Ogievskiy
` (4 subsequent siblings)
13 siblings, 2 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-09 18:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
Don't use error propagation in qcow2_get_specific_info(). For this
refactor qcow2_get_bitmap_info_list, its current interface rather
weird.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2.h | 4 ++--
block/qcow2-bitmap.c | 27 +++++++++++++--------------
block/qcow2.c | 10 +++-------
3 files changed, 18 insertions(+), 23 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 065ec3df0b..ac6a2d3e2a 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -967,8 +967,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 8c34b2aef7..9b14c0791f 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1090,30 +1090,29 @@ 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 bm_list set (probably to NULL, if no bitmaps),
+ * 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);
@@ -1121,13 +1120,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 10175fa399..eb7c82120c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5056,12 +5056,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;
}
}
@@ -5078,9 +5076,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] 57+ messages in thread* Re: [PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation
2020-09-09 18:59 ` [PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation Vladimir Sementsov-Ogievskiy
@ 2020-09-11 8:41 ` Greg Kurz
2020-09-17 16:32 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Greg Kurz @ 2020-09-11 8:41 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed, 9 Sep 2020 21:59:25 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> Don't use error propagation in qcow2_get_specific_info(). For this
> refactor qcow2_get_bitmap_info_list, its current interface rather
... interface is rather
> weird.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> block/qcow2.h | 4 ++--
> block/qcow2-bitmap.c | 27 +++++++++++++--------------
> block/qcow2.c | 10 +++-------
> 3 files changed, 18 insertions(+), 23 deletions(-)
>
> diff --git a/block/qcow2.h b/block/qcow2.h
> index 065ec3df0b..ac6a2d3e2a 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -967,8 +967,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 8c34b2aef7..9b14c0791f 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1090,30 +1090,29 @@ 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 bm_list set (probably to NULL, if no bitmaps),
> + * 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);
> @@ -1121,13 +1120,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 10175fa399..eb7c82120c 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -5056,12 +5056,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;
> }
> }
> @@ -5078,9 +5076,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;
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation
2020-09-09 18:59 ` [PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation Vladimir Sementsov-Ogievskiy
2020-09-11 8:41 ` Greg Kurz
@ 2020-09-17 16:32 ` Alberto Garcia
2020-09-17 18:52 ` Vladimir Sementsov-Ogievskiy
1 sibling, 1 reply; 57+ messages in thread
From: Alberto Garcia @ 2020-09-17 16:32 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed 09 Sep 2020 08:59:25 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> + * On success return true with bm_list set (probably to NULL, if no bitmaps),
" probably " ? :-)
> + * 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;
So here 'list' points at NULL and 'plist' at &list.
> - *plist = obj;
> - plist = &obj->next;
In the original code 'plist' is updated when you add a new element, so
it always points at the end of the list. But 'list' is unchanged and it
still points at the first element.
So the caller receives a pointer to the first element.
> + *info_list = obj;
> + info_list = &obj->next;
But in the new code there is only one variable (passed by the caller),
which always points at the end of the list.
Berto
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation
2020-09-17 16:32 ` Alberto Garcia
@ 2020-09-17 18:52 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 18:52 UTC (permalink / raw)
To: Alberto Garcia, qemu-block
Cc: qemu-devel, armbru, eblake, jsnow, stefanha, pbonzini,
pavel.dovgaluk, ari, mreitz, kwolf, groug
17.09.2020 19:32, Alberto Garcia wrote:
> On Wed 09 Sep 2020 08:59:25 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>
>> + * On success return true with bm_list set (probably to NULL, if no bitmaps),
>
> " probably " ? :-)
I note this as "set to NULL" is not obvious thing (is it "unset" ? :).. And by "probably" I mean "may be", i.e. NULL is just one of possible cases. Probably I use "probably" in a wrong way?
>
>> + * 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;
>
> So here 'list' points at NULL and 'plist' at &list.
Hmm, to be precise, list _is_ NULL (and points nowhere), and plist points to list.
>
>> - *plist = obj;
>> - plist = &obj->next;
>
> In the original code 'plist' is updated when you add a new element, so
> it always points at the end of the list. But 'list' is unchanged and it
> still points at the first element.
>
> So the caller receives a pointer to the first element.
>
>> + *info_list = obj;
>> + info_list = &obj->next;
>
> But in the new code there is only one variable (passed by the caller),
> which always points at the end of the list.
>
No: at first "*info_list = obj", we set the result which user will get, users pointer now points to the first object in the list.
Then, at "info_list = &obj->next", we reassign info_list to another pointer: to "next" field of first list item. So, all further "*info_list = obj" are note visible to the caller.
Actually, the logic is not changed, just instead of plist we use info_list, and instead of list - a variable which should be defined in the caller. Look: in old code, first "*plist = obj" sets "list" variable, but all further "*plist = obj" don't change "list" variable.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 10/14] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface
2020-09-09 18:59 [PATCH 00/14] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (8 preceding siblings ...)
2020-09-09 18:59 ` [PATCH 09/14] block/qcow2: qcow2_get_specific_info(): drop error propagation Vladimir Sementsov-Ogievskiy
@ 2020-09-09 18:59 ` Vladimir Sementsov-Ogievskiy
2020-09-17 16:35 ` Alberto Garcia
2020-09-09 18:59 ` [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps Vladimir Sementsov-Ogievskiy
` (3 subsequent siblings)
13 siblings, 1 reply; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-09 18:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
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>
---
block/qcow2.h | 3 ++-
block/qcow2-bitmap.c | 22 +++++++++-------------
block/qcow2.c | 6 ++----
3 files changed, 13 insertions(+), 18 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index ac6a2d3e2a..e7e662533b 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -966,7 +966,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 9b14c0791f..f58923fce3 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -959,30 +959,24 @@ 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.
- */
-bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
+/* Return true on success, false on failure. */
+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 (s->nb_bitmaps == 0) {
/* No bitmaps - nothing to do */
- return false;
+ return true;
}
bm_list = bitmap_list_load(bs, s->bitmap_directory_offset,
s->bitmap_directory_size, errp);
- if (bm_list == NULL) {
+ if (!bm_list) {
return false;
}
@@ -1033,7 +1027,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)) {
@@ -1044,7 +1040,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 eb7c82120c..c2cd9434cc 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;
@@ -1786,9 +1785,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] 57+ messages in thread* Re: [PATCH 10/14] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface
2020-09-09 18:59 ` [PATCH 10/14] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface Vladimir Sementsov-Ogievskiy
@ 2020-09-17 16:35 ` Alberto Garcia
2020-09-17 18:58 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 57+ messages in thread
From: Alberto Garcia @ 2020-09-17 16:35 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed 09 Sep 2020 08:59:26 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> -/* 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.
> - */
> -bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> +/* Return true on success, false on failure. */
> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
> + Error **errp)
I think that the documentation should clarify under what conditions
'header_updated' is modified.
> if (s->nb_bitmaps == 0) {
> /* No bitmaps - nothing to do */
> - return false;
> + return true;
> }
Here is it not for example (should it be set to false?).
> - if (bm_list == NULL) {
> + if (!bm_list) {
> return false;
> }
This looks like a cosmetic change unrelated to the rest of the patch.
Berto
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 10/14] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface
2020-09-17 16:35 ` Alberto Garcia
@ 2020-09-17 18:58 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 18:58 UTC (permalink / raw)
To: Alberto Garcia, qemu-block
Cc: qemu-devel, armbru, eblake, jsnow, stefanha, pbonzini,
pavel.dovgaluk, ari, mreitz, kwolf, groug
17.09.2020 19:35, Alberto Garcia wrote:
> On Wed 09 Sep 2020 08:59:26 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> -/* 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.
>> - */
>> -bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>> +/* Return true on success, false on failure. */
>> +bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
>> + Error **errp)
>
> I think that the documentation should clarify under what conditions
> 'header_updated' is modified.
>
>> if (s->nb_bitmaps == 0) {
>> /* No bitmaps - nothing to do */
>> - return false;
>> + return true;
>> }
>
> Here is it not for example (should it be set to false?).
Ha, I think, it just shows that patch is wrong :) We should set header_updated at least on every success path. Or better always (if it is non-NULL of course). Thanks for careful review!
>
>> - if (bm_list == NULL) {
>> + if (!bm_list) {
>> return false;
>> }
>
> This looks like a cosmetic change unrelated to the rest of the patch.
>
> Berto
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps
2020-09-09 18:59 [PATCH 00/14] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (9 preceding siblings ...)
2020-09-09 18:59 ` [PATCH 10/14] block/qcow2-bitmap: improve qcow2_load_dirty_bitmaps() interface Vladimir Sementsov-Ogievskiy
@ 2020-09-09 18:59 ` Vladimir Sementsov-Ogievskiy
2020-09-11 9:38 ` Greg Kurz
2020-09-17 14:50 ` Alberto Garcia
2020-09-09 18:59 ` [PATCH 12/14] block/qcow2: read_cache_sizes: return status value Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
13 siblings, 2 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-09 18:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
It's better to return status together with setting errp. It makes
possible to avoid error propagation.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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 e7e662533b..49824be5c6 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1524,9 +1524,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;
@@ -1546,7 +1547,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;
}
}
@@ -1661,7 +1662,7 @@ success:
}
bitmap_list_free(bm_list);
- return;
+ return true;
fail:
QSIMPLEQ_FOREACH(bm, bm_list, entry) {
@@ -1679,16 +1680,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] 57+ messages in thread* Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps
2020-09-09 18:59 ` [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps Vladimir Sementsov-Ogievskiy
@ 2020-09-11 9:38 ` Greg Kurz
2020-09-11 10:18 ` Vladimir Sementsov-Ogievskiy
2020-09-17 14:50 ` Alberto Garcia
1 sibling, 1 reply; 57+ messages in thread
From: Greg Kurz @ 2020-09-11 9:38 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
s/startus/status
On Wed, 9 Sep 2020 21:59:27 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> It's better to return status together with setting errp. It makes
> possible to avoid error propagation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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 e7e662533b..49824be5c6 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -1524,9 +1524,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();
Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
an error_prepend(errp, ...) not visible in the patch context.
Anyway,
Reviewed-by: Greg Kurz <groug@kaod.org>
> BdrvDirtyBitmap *bitmap;
> BDRVQcow2State *s = bs->opaque;
> uint32_t new_nb_bitmaps = s->nb_bitmaps;
> @@ -1546,7 +1547,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;
> }
> }
>
> @@ -1661,7 +1662,7 @@ success:
> }
>
> bitmap_list_free(bm_list);
> - return;
> + return true;
>
> fail:
> QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> @@ -1679,16 +1680,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;
> }
>
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps
2020-09-11 9:38 ` Greg Kurz
@ 2020-09-11 10:18 ` Vladimir Sementsov-Ogievskiy
2020-09-11 11:21 ` Greg Kurz
0 siblings, 1 reply; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-11 10:18 UTC (permalink / raw)
To: Greg Kurz
Cc: qemu-block, qemu-devel, armbru, berto, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf
11.09.2020 12:38, Greg Kurz wrote:
> s/startus/status
>
> On Wed, 9 Sep 2020 21:59:27 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>
>> It's better to return status together with setting errp. It makes
>> possible to avoid error propagation.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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 e7e662533b..49824be5c6 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
>> --- a/block/qcow2-bitmap.c
>> +++ b/block/qcow2-bitmap.c
>> @@ -1524,9 +1524,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();
>
> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
> an error_prepend(errp, ...) not visible in the patch context.
Ah yes. Actually this is occasional thing I didn't want to include into this patch
(and int this part I). So we can just drop it and leave for part II or part III,
or add a note into commit message
>
> Anyway,
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
Thanks a lot for reviewing :)
Hmm.. With this series I understand the following:
1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
- it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
- reviewing is the hardest part of the process
So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.
2. So, the process turns into following steps:
- apply scripts/coccinelle/errp-guard.cocci
- look through patches and do obvious refactorings (like this series)
- keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)
3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)
The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).
So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..
>
>> BdrvDirtyBitmap *bitmap;
>> BDRVQcow2State *s = bs->opaque;
>> uint32_t new_nb_bitmaps = s->nb_bitmaps;
>> @@ -1546,7 +1547,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;
>> }
>> }
>>
>> @@ -1661,7 +1662,7 @@ success:
>> }
>>
>> bitmap_list_free(bm_list);
>> - return;
>> + return true;
>>
>> fail:
>> QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>> @@ -1679,16 +1680,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;
>> }
>>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps
2020-09-11 10:18 ` Vladimir Sementsov-Ogievskiy
@ 2020-09-11 11:21 ` Greg Kurz
2020-09-11 11:39 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 57+ messages in thread
From: Greg Kurz @ 2020-09-11 11:21 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
On Fri, 11 Sep 2020 13:18:32 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 11.09.2020 12:38, Greg Kurz wrote:
> > s/startus/status
> >
> > On Wed, 9 Sep 2020 21:59:27 +0300
> > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> >
> >> It's better to return status together with setting errp. It makes
> >> possible to avoid error propagation.
> >>
> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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 e7e662533b..49824be5c6 100644
> >> --- a/block/qcow2.h
> >> +++ b/block/qcow2.h
> >> @@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
> >> --- a/block/qcow2-bitmap.c
> >> +++ b/block/qcow2-bitmap.c
> >> @@ -1524,9 +1524,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();
> >
> > Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
> > an error_prepend(errp, ...) not visible in the patch context.
>
> Ah yes. Actually this is occasional thing I didn't want to include into this patch
> (and int this part I). So we can just drop it and leave for part II or part III,
> or add a note into commit message
>
> >
> > Anyway,
> >
> > Reviewed-by: Greg Kurz <groug@kaod.org>
>
> Thanks a lot for reviewing :)
>
Don't mention it :)
> Hmm.. With this series I understand the following:
>
> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
>
> - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
> - reviewing is the hardest part of the process
>
> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.
>
> 2. So, the process turns into following steps:
>
> - apply scripts/coccinelle/errp-guard.cocci
> - look through patches and do obvious refactorings (like this series)
> - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)
>
I've started to follow this process for the spapr code and, indeed, I
can come up with better changes by refactoring some code manually.
Some of these changes are not that obvious that they could be made
by someone who doesn't know the code, so I tend to agree with your
arguments in 1.
This is also the reason I didn't review patches 10, 13 and 14 because
they looked like I should understand the corresponding code a bit more.
> 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)
>
Ha ha :D ... as a consolation prize, maybe we can reach a fair number
of r-b by reviewing each other's _simple_ cleanups ;-)
> The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).
>
This sounds nice. My only concern would be to end up fixing code nobody
uses or cares for, so I guess it would be better that active maintainers
or supporters give impetus on that.
> So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..
>
I don't :) but the very massive series that were posted on the topic
the last few months look like an announcement to me, at least for
active maintainers and supporters.
> >
> >> BdrvDirtyBitmap *bitmap;
> >> BDRVQcow2State *s = bs->opaque;
> >> uint32_t new_nb_bitmaps = s->nb_bitmaps;
> >> @@ -1546,7 +1547,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;
> >> }
> >> }
> >>
> >> @@ -1661,7 +1662,7 @@ success:
> >> }
> >>
> >> bitmap_list_free(bm_list);
> >> - return;
> >> + return true;
> >>
> >> fail:
> >> QSIMPLEQ_FOREACH(bm, bm_list, entry) {
> >> @@ -1679,16 +1680,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;
> >> }
> >>
> >
>
>
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps
2020-09-11 11:21 ` Greg Kurz
@ 2020-09-11 11:39 ` Vladimir Sementsov-Ogievskiy
2020-09-11 15:22 ` Markus Armbruster
0 siblings, 1 reply; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-11 11:39 UTC (permalink / raw)
To: Greg Kurz
Cc: qemu-block, qemu-devel, armbru, berto, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf
11.09.2020 14:21, Greg Kurz wrote:
> On Fri, 11 Sep 2020 13:18:32 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>
>> 11.09.2020 12:38, Greg Kurz wrote:
>>> s/startus/status
>>>
>>> On Wed, 9 Sep 2020 21:59:27 +0300
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>
>>>> It's better to return status together with setting errp. It makes
>>>> possible to avoid error propagation.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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 e7e662533b..49824be5c6 100644
>>>> --- a/block/qcow2.h
>>>> +++ b/block/qcow2.h
>>>> @@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
>>>> --- a/block/qcow2-bitmap.c
>>>> +++ b/block/qcow2-bitmap.c
>>>> @@ -1524,9 +1524,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();
>>>
>>> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
>>> an error_prepend(errp, ...) not visible in the patch context.
>>
>> Ah yes. Actually this is occasional thing I didn't want to include into this patch
>> (and int this part I). So we can just drop it and leave for part II or part III,
>> or add a note into commit message
>>
>>>
>>> Anyway,
>>>
>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>
>> Thanks a lot for reviewing :)
>>
>
> Don't mention it :)
>
>> Hmm.. With this series I understand the following:
>>
>> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
>>
>> - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
>> - reviewing is the hardest part of the process
>>
>> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.
>>
>> 2. So, the process turns into following steps:
>>
>> - apply scripts/coccinelle/errp-guard.cocci
>> - look through patches and do obvious refactorings (like this series)
>> - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)
>>
>
> I've started to follow this process for the spapr code and, indeed, I
> can come up with better changes by refactoring some code manually.
> Some of these changes are not that obvious that they could be made
> by someone who doesn't know the code, so I tend to agree with your
> arguments in 1.
>
> This is also the reason I didn't review patches 10, 13 and 14 because
> they looked like I should understand the corresponding code a bit more.
>
>> 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)
>>
>
> Ha ha :D ... as a consolation prize, maybe we can reach a fair number
> of r-b by reviewing each other's _simple_ cleanups ;-)
>
>> The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).
>>
>
> This sounds nice. My only concern would be to end up fixing code nobody
> uses or cares for, so I guess it would be better that active maintainers
> or supporters give impetus on that.
>
>> So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..
>>
>
> I don't :) but the very massive series that were posted on the topic
> the last few months look like an announcement to me, at least for
> active maintainers and supporters.
Aha, I know. Better than announcement is improving checkpatch.
>
>>>
>>>> BdrvDirtyBitmap *bitmap;
>>>> BDRVQcow2State *s = bs->opaque;
>>>> uint32_t new_nb_bitmaps = s->nb_bitmaps;
>>>> @@ -1546,7 +1547,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;
>>>> }
>>>> }
>>>>
>>>> @@ -1661,7 +1662,7 @@ success:
>>>> }
>>>>
>>>> bitmap_list_free(bm_list);
>>>> - return;
>>>> + return true;
>>>>
>>>> fail:
>>>> QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>>>> @@ -1679,16 +1680,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;
>>>> }
>>>>
>>>
>>
>>
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps
2020-09-11 11:39 ` Vladimir Sementsov-Ogievskiy
@ 2020-09-11 15:22 ` Markus Armbruster
2020-09-11 17:20 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 57+ messages in thread
From: Markus Armbruster @ 2020-09-11 15:22 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, jsnow,
armbru, Greg Kurz, stefanha, pbonzini, mreitz, ari
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 11.09.2020 14:21, Greg Kurz wrote:
>> On Fri, 11 Sep 2020 13:18:32 +0300
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>
>>> 11.09.2020 12:38, Greg Kurz wrote:
>>>> s/startus/status
>>>>
>>>> On Wed, 9 Sep 2020 21:59:27 +0300
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>
>>>>> It's better to return status together with setting errp. It makes
>>>>> possible to avoid error propagation.
>>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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 e7e662533b..49824be5c6 100644
>>>>> --- a/block/qcow2.h
>>>>> +++ b/block/qcow2.h
>>>>> @@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
>>>>> --- a/block/qcow2-bitmap.c
>>>>> +++ b/block/qcow2-bitmap.c
>>>>> @@ -1524,9 +1524,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();
>>>>
>>>> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
>>>> an error_prepend(errp, ...) not visible in the patch context.
>>>
>>> Ah yes. Actually this is occasional thing I didn't want to include into this patch
>>> (and int this part I). So we can just drop it and leave for part II or part III,
>>> or add a note into commit message
Either works for me.
>>>>
>>>> Anyway,
>>>>
>>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>>
>>> Thanks a lot for reviewing :)
>>>
>> Don't mention it :)
>>
>>> Hmm.. With this series I understand the following:
>>>
>>> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
>>>
>>> - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
>>> - reviewing is the hardest part of the process
>>>
>>> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.
Yes, going the extra mile is better.
I recommend it for code that is actively maintained. Making the code
simpler and thus easier to maintain is an investment that'll pay off.
We may have code where it won't pay off. Do you think a blind
application of errp-guard.cocci might be better than nothing there?
>>> 2. So, the process turns into following steps:
>>>
>>> - apply scripts/coccinelle/errp-guard.cocci
>>> - look through patches and do obvious refactorings (like this series)
>>> - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)
>>>
>> I've started to follow this process for the spapr code and, indeed,
>> I
>> can come up with better changes by refactoring some code manually.
>> Some of these changes are not that obvious that they could be made
>> by someone who doesn't know the code, so I tend to agree with your
>> arguments in 1.
>> This is also the reason I didn't review patches 10, 13 and 14
>> because
>> they looked like I should understand the corresponding code a bit more.
>>
>>> 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)
LOL
A lesser craftsman than you would've peddled the generated commits
anyway. Props!
>> Ha ha :D ... as a consolation prize, maybe we can reach a fair
>> number
>> of r-b by reviewing each other's _simple_ cleanups ;-)
>>
>>> The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).
If people care enough about [2] to submit patches, I'll feel obliged to
help merging them.
>> This sounds nice. My only concern would be to end up fixing code
>> nobody
>> uses or cares for, so I guess it would be better that active maintainers
>> or supporters give impetus on that.
A bit of weeding on the side is always appreciated, but please don't
feel obliged to sink lots of time into code you don't care about.
>>> So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..
>
>> I don't :) but the very massive series that were posted on the topic
>> the last few months look like an announcement to me, at least for
>> active maintainers and supporters.
>
> Aha, I know. Better than announcement is improving checkpatch.
Yes, automated feedback works best.
Relentless pressure from reviewers can also work in the long, long run.
But it's tiresome.
Of course, checkpatch.pl checks only new or changed code. Any ideas on
how to make people aware of the opportunity to simplify their existing
code? Obvious: posting more patches simplifying existing code we care
about. Any other smart ideas?
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps
2020-09-11 15:22 ` Markus Armbruster
@ 2020-09-11 17:20 ` Vladimir Sementsov-Ogievskiy
2020-09-14 6:53 ` Markus Armbruster
0 siblings, 1 reply; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-11 17:20 UTC (permalink / raw)
To: Markus Armbruster
Cc: Greg Kurz, qemu-block, qemu-devel, berto, eblake, jsnow,
stefanha, pbonzini, pavel.dovgaluk, ari, mreitz, kwolf
11.09.2020 18:22, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>
>> 11.09.2020 14:21, Greg Kurz wrote:
>>> On Fri, 11 Sep 2020 13:18:32 +0300
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>
>>>> 11.09.2020 12:38, Greg Kurz wrote:
>>>>> s/startus/status
>>>>>
>>>>> On Wed, 9 Sep 2020 21:59:27 +0300
>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>>>
>>>>>> It's better to return status together with setting errp. It makes
>>>>>> possible to avoid error propagation.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.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 e7e662533b..49824be5c6 100644
>>>>>> --- a/block/qcow2.h
>>>>>> +++ b/block/qcow2.h
>>>>>> @@ -972,7 +972,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 f58923fce3..5eeff1cb1c 100644
>>>>>> --- a/block/qcow2-bitmap.c
>>>>>> +++ b/block/qcow2-bitmap.c
>>>>>> @@ -1524,9 +1524,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();
>>>>>
>>>>> Maybe worth mentioning in the changelog that this ERRP_GUARD() fixes
>>>>> an error_prepend(errp, ...) not visible in the patch context.
>>>>
>>>> Ah yes. Actually this is occasional thing I didn't want to include into this patch
>>>> (and int this part I). So we can just drop it and leave for part II or part III,
>>>> or add a note into commit message
>
> Either works for me.
>
>>>>>
>>>>> Anyway,
>>>>>
>>>>> Reviewed-by: Greg Kurz <groug@kaod.org>
>>>>
>>>> Thanks a lot for reviewing :)
>>>>
>>> Don't mention it :)
>>>
>>>> Hmm.. With this series I understand the following:
>>>>
>>>> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
>>>>
>>>> - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
>>>> - reviewing is the hardest part of the process
>>>>
>>>> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.
>
> Yes, going the extra mile is better.
>
> I recommend it for code that is actively maintained. Making the code
> simpler and thus easier to maintain is an investment that'll pay off.
>
> We may have code where it won't pay off. Do you think a blind
> application of errp-guard.cocci might be better than nothing there?
I think, careful review is needed anyway. And it may be too large effort for dead (or almost dead) code.
So, let's start from popular subsystems. And make a decision for the rest later.
>
>>>> 2. So, the process turns into following steps:
>>>>
>>>> - apply scripts/coccinelle/errp-guard.cocci
>>>> - look through patches and do obvious refactorings (like this series)
>>>> - keep ERRP_GUARD where necessary (appending info to error, or where refactoring of function return status is too invasive and not simple)
>>>>
>>> I've started to follow this process for the spapr code and, indeed,
>>> I
>>> can come up with better changes by refactoring some code manually.
>>> Some of these changes are not that obvious that they could be made
>>> by someone who doesn't know the code, so I tend to agree with your
>>> arguments in 1.
>>> This is also the reason I didn't review patches 10, 13 and 14
>>> because
>>> they looked like I should understand the corresponding code a bit more.
>>>
>>>> 3. Obviously, that's too much for me :) Of course, I will invest some time into making the series like this one, and reviewing them, but I can't do it for weeks and months. (My original сunning plan to simply push ~100 generated commits with my s-o-b and become the greatest contributor failed:)
>
> LOL
>
> A lesser craftsman than you would've peddled the generated commits
> anyway. Props!
>
>>> Ha ha :D ... as a consolation prize, maybe we can reach a fair
>>> number
>>> of r-b by reviewing each other's _simple_ cleanups ;-)
>>>
>>>> The good thing is that now, with ERRP_GUARD finally merged, we can produce parallel series like this, and they will be processed in parallel by different maintainers (and Markus will have to merge series for subsystems with unavailable maintainers).
>
> If people care enough about [2] to submit patches, I'll feel obliged to
> help merging them.
>
>>> This sounds nice. My only concern would be to end up fixing code
>>> nobody
>>> uses or cares for, so I guess it would be better that active maintainers
>>> or supporters give impetus on that.
>
> A bit of weeding on the side is always appreciated, but please don't
> feel obliged to sink lots of time into code you don't care about.
>
>>>> So, everybody is welcome to the process [2]. Probably we want to make a separate announcement in a list with short recommendations and instructions? But who read announcements..
>>
>>> I don't :) but the very massive series that were posted on the topic
>>> the last few months look like an announcement to me, at least for
>>> active maintainers and supporters.
>>
>> Aha, I know. Better than announcement is improving checkpatch.
>
> Yes, automated feedback works best.
>
> Relentless pressure from reviewers can also work in the long, long run.
> But it's tiresome.
>
> Of course, checkpatch.pl checks only new or changed code. Any ideas on
> how to make people aware of the opportunity to simplify their existing
> code? Obvious: posting more patches simplifying existing code we care
> about. Any other smart ideas?
>
I think it would be great for checkpatch to warn about some code problems in function touched by checked patch. Code-style, error-api, etc. This will attract developers to improve existing code they are interested in.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps
2020-09-11 17:20 ` Vladimir Sementsov-Ogievskiy
@ 2020-09-14 6:53 ` Markus Armbruster
0 siblings, 0 replies; 57+ messages in thread
From: Markus Armbruster @ 2020-09-14 6:53 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, Greg Kurz, qemu-devel,
stefanha, pbonzini, mreitz, jsnow, ari
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 11.09.2020 18:22, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>
>>> 11.09.2020 14:21, Greg Kurz wrote:
>>>> On Fri, 11 Sep 2020 13:18:32 +0300
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
[...]
>>>>> Hmm.. With this series I understand the following:
>>>>>
>>>>> 1. It's no sense in simple applying scripts/coccinelle/errp-guard.cocci to the whole code-base, because:
>>>>>
>>>>> - it produces a lot of "if (*errp)" in places where it is really simple to avoid error propagation at all, like in this series
>>>>> - reviewing is the hardest part of the process
>>>>>
>>>>> So, if we have to review these changes anyway, it's better to invest a bit more time into patch creation, and make code correspond to our modern error API recommendations.
>>
>> Yes, going the extra mile is better.
>>
>> I recommend it for code that is actively maintained. Making the code
>> simpler and thus easier to maintain is an investment that'll pay off.
>>
>> We may have code where it won't pay off. Do you think a blind
>> application of errp-guard.cocci might be better than nothing there?
>
> I think, careful review is needed anyway. And it may be too large effort for dead (or almost dead) code.
> So, let's start from popular subsystems. And make a decision for the rest later.
Makes sense.
[...]
^ permalink raw reply [flat|nested] 57+ messages in thread
* Re: [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps
2020-09-09 18:59 ` [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps Vladimir Sementsov-Ogievskiy
2020-09-11 9:38 ` Greg Kurz
@ 2020-09-17 14:50 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Alberto Garcia @ 2020-09-17 14:50 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed 09 Sep 2020 08:59:27 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> It's better to return status together with setting errp. It makes
> possible to avoid error propagation.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[...]
> -void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> +bool qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
> bool release_stored, Error **errp)
> {
> + ERRP_GUARD();
This needs an explanation.
Berto
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 12/14] block/qcow2: read_cache_sizes: return status value
2020-09-09 18:59 [PATCH 00/14] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (10 preceding siblings ...)
2020-09-09 18:59 ` [PATCH 11/14] block/qcow2-bitmap: return startus from qcow2_store_persistent_dirty_bitmaps Vladimir Sementsov-Ogievskiy
@ 2020-09-09 18:59 ` Vladimir Sementsov-Ogievskiy
2020-09-11 9:40 ` Greg Kurz
2020-09-17 14:51 ` Alberto Garcia
2020-09-09 18:59 ` [PATCH 13/14] block/qcow2: qcow2_do_open: deal with errp Vladimir Sementsov-Ogievskiy
2020-09-09 18:59 ` [PATCH 14/14] block/qed: bdrv_qed_do_open: " Vladimir Sementsov-Ogievskiy
13 siblings, 2 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-09 18:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
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>
---
block/qcow2.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index c2cd9434cc..31dd28d19e 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] 57+ messages in thread* Re: [PATCH 12/14] block/qcow2: read_cache_sizes: return status value
2020-09-09 18:59 ` [PATCH 12/14] block/qcow2: read_cache_sizes: return status value Vladimir Sementsov-Ogievskiy
@ 2020-09-11 9:40 ` Greg Kurz
2020-09-17 14:51 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Greg Kurz @ 2020-09-11 9:40 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, berto, pavel.dovgaluk, qemu-block, qemu-devel, armbru,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed, 9 Sep 2020 21:59:28 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 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>
> block/qcow2.c | 19 +++++++++----------
> 1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c2cd9434cc..31dd28d19e 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;
> }
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 12/14] block/qcow2: read_cache_sizes: return status value
2020-09-09 18:59 ` [PATCH 12/14] block/qcow2: read_cache_sizes: return status value Vladimir Sementsov-Ogievskiy
2020-09-11 9:40 ` Greg Kurz
@ 2020-09-17 14:51 ` Alberto Garcia
1 sibling, 0 replies; 57+ messages in thread
From: Alberto Garcia @ 2020-09-17 14:51 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed 09 Sep 2020 08:59:28 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
> 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: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 13/14] block/qcow2: qcow2_do_open: deal with errp
2020-09-09 18:59 [PATCH 00/14] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (11 preceding siblings ...)
2020-09-09 18:59 ` [PATCH 12/14] block/qcow2: read_cache_sizes: return status value Vladimir Sementsov-Ogievskiy
@ 2020-09-09 18:59 ` Vladimir Sementsov-Ogievskiy
2020-09-17 16:23 ` Alberto Garcia
2020-09-09 18:59 ` [PATCH 14/14] block/qed: bdrv_qed_do_open: " Vladimir Sementsov-Ogievskiy
13 siblings, 1 reply; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-09 18:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
1. Drop extra error propagation
2. 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 qcow2_do_open to not behave this
way. This allows to simplify code in qcow2_co_invalidate_cache().
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
block/qcow2.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 31dd28d19e..cc4e7dd461 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1292,6 +1292,7 @@ static int validate_compression_type(BDRVQcow2State *s, Error **errp)
static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
int flags, Error **errp)
{
+ ERRP_GUARD();
BDRVQcow2State *s = bs->opaque;
unsigned int len, i;
int ret = 0;
@@ -1426,6 +1427,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
report_unsupported_feature(errp, feature_table,
s->incompatible_features &
~QCOW2_INCOMPAT_MASK);
+ error_setg(errp,
+ "qcow2 header contains unknown incompatible_feature bits");
ret = -ENOTSUP;
g_free(feature_table);
goto fail;
@@ -2709,11 +2712,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;
/*
@@ -2731,16 +2734,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] 57+ messages in thread* Re: [PATCH 13/14] block/qcow2: qcow2_do_open: deal with errp
2020-09-09 18:59 ` [PATCH 13/14] block/qcow2: qcow2_do_open: deal with errp Vladimir Sementsov-Ogievskiy
@ 2020-09-17 16:23 ` Alberto Garcia
2020-09-17 17:44 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 57+ messages in thread
From: Alberto Garcia @ 2020-09-17 16:23 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed 09 Sep 2020 08:59:29 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 1. Drop extra error propagation
>
> 2. 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 qcow2_do_open to not behave this
> way. This allows to simplify code in qcow2_co_invalidate_cache().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/qcow2.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 31dd28d19e..cc4e7dd461 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1292,6 +1292,7 @@ static int validate_compression_type(BDRVQcow2State *s, Error **errp)
> static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> int flags, Error **errp)
> {
> + ERRP_GUARD();
Why is this necessary?
> BDRVQcow2State *s = bs->opaque;
> unsigned int len, i;
> int ret = 0;
> @@ -1426,6 +1427,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> report_unsupported_feature(errp, feature_table,
> s->incompatible_features &
> ~QCOW2_INCOMPAT_MASK);
> + error_setg(errp,
> + "qcow2 header contains unknown
> incompatible_feature bits");
I think that this is a mistake because the previous call to
report_unsupported_feature() already calls error_setg();
> @@ -2709,11 +2712,11 @@ static void qcow2_close(BlockDriverState *bs)
> static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
> Error **errp)
> {
> + ERRP_GUARD();
Again, why is this necessary?
Berto
^ permalink raw reply [flat|nested] 57+ messages in thread* Re: [PATCH 13/14] block/qcow2: qcow2_do_open: deal with errp
2020-09-17 16:23 ` Alberto Garcia
@ 2020-09-17 17:44 ` Vladimir Sementsov-Ogievskiy
0 siblings, 0 replies; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-17 17:44 UTC (permalink / raw)
To: Alberto Garcia, qemu-block
Cc: qemu-devel, armbru, eblake, jsnow, stefanha, pbonzini,
pavel.dovgaluk, ari, mreitz, kwolf, groug
17.09.2020 19:23, Alberto Garcia wrote:
> On Wed 09 Sep 2020 08:59:29 PM CEST, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> 1. Drop extra error propagation
>>
>> 2. 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 qcow2_do_open to not behave this
>> way. This allows to simplify code in qcow2_co_invalidate_cache().
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>> block/qcow2.c | 16 +++++++---------
>> 1 file changed, 7 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 31dd28d19e..cc4e7dd461 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1292,6 +1292,7 @@ static int validate_compression_type(BDRVQcow2State *s, Error **errp)
>> static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>> int flags, Error **errp)
>> {
>> + ERRP_GUARD();
>
> Why is this necessary?
Because error_append_hint() used in the function. Without ERRP_GUARD, error_append_hint won't work if errp = &error_fatal
Read more in include/qapi/error.h near ERRP_GUARD definition.
But yes, it's good to not it in commit message.
>
>> BDRVQcow2State *s = bs->opaque;
>> unsigned int len, i;
>> int ret = 0;
>> @@ -1426,6 +1427,8 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>> report_unsupported_feature(errp, feature_table,
>> s->incompatible_features &
>> ~QCOW2_INCOMPAT_MASK);
>> + error_setg(errp,
>> + "qcow2 header contains unknown
>> incompatible_feature bits");
>
> I think that this is a mistake because the previous call to
> report_unsupported_feature() already calls error_setg();
Oops, you are right.
>
>> @@ -2709,11 +2712,11 @@ static void qcow2_close(BlockDriverState *bs)
>> static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,
>> Error **errp)
>> {
>> + ERRP_GUARD();
>
> Again, why is this necessary?
>
Because it uses error_prepend() after conversion (same reason as for error_append_hint()).
Thanks for review! I'll post v2 soon.
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 57+ messages in thread
* [PATCH 14/14] block/qed: bdrv_qed_do_open: deal with errp
2020-09-09 18:59 [PATCH 00/14] block: deal with errp: part I Vladimir Sementsov-Ogievskiy
` (12 preceding siblings ...)
2020-09-09 18:59 ` [PATCH 13/14] block/qcow2: qcow2_do_open: deal with errp Vladimir Sementsov-Ogievskiy
@ 2020-09-09 18:59 ` Vladimir Sementsov-Ogievskiy
2020-09-17 16:25 ` Alberto Garcia
13 siblings, 1 reply; 57+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2020-09-09 18:59 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, armbru, berto, vsementsov, eblake, jsnow, stefanha,
pbonzini, pavel.dovgaluk, ari, mreitz, kwolf, groug
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>
---
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] 57+ messages in thread* Re: [PATCH 14/14] block/qed: bdrv_qed_do_open: deal with errp
2020-09-09 18:59 ` [PATCH 14/14] block/qed: bdrv_qed_do_open: " Vladimir Sementsov-Ogievskiy
@ 2020-09-17 16:25 ` Alberto Garcia
0 siblings, 0 replies; 57+ messages in thread
From: Alberto Garcia @ 2020-09-17 16:25 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy, qemu-block
Cc: kwolf, vsementsov, pavel.dovgaluk, qemu-devel, armbru, groug,
stefanha, pbonzini, mreitz, jsnow, ari
On Wed 09 Sep 2020 08:59:30 PM CEST, 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>
Berto
^ permalink raw reply [flat|nested] 57+ messages in thread