| Message ID | 20210202124956.63146-4-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: deal with errp: part I | expand |
On 2/2/21 6:49 AM, 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 With this patch applied, 'check unit-test' fails with: Running test test-replication Unexpected error in bdrv_open_driver() at ../block.c:1481: Could not open '/tmp/p_local_disk.z1Ugyc': Invalid argument ERROR test-replication - missing test plan Directly reverting it has ripple effect on later patches in the series. Running test-replication under gdb gives this backtrace: Thread 1 "test-replicatio" received signal SIGABRT, Aborted. 0x00007ffff6f6f9d5 in raise () from /lib64/libc.so.6 (gdb) bt #0 0x00007ffff6f6f9d5 in raise () from /lib64/libc.so.6 #1 0x00007ffff6f588a4 in abort () from /lib64/libc.so.6 #2 0x00005555556ad820 in error_handle_fatal ( errp=0x555555790568 <error_abort>, err=0x555555859010) at ../util/error.c:40 #3 0x00005555556ae3cf in error_propagate ( dst_errp=0x555555790568 <error_abort>, local_err=0x555555859010) at ../util/error.c:286 #4 0x000055555558cc9e in bdrv_img_create ( filename=0x555555822500 "/tmp/p_local_disk.DVFoWt", fmt=0x5555556e809a "qcow2", base_filename=0x0, base_fmt=0x0, options=0x0, img_size=67108864, flags=2, quiet=true, errp=0x555555790568 <error_abort>) at ../block.c:6312 > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Greg Kurz <groug@kaod.org> > Reviewed-by: Alberto Garcia <berto@igalia.com> > --- > block/blkdebug.c | 6 ++---- > block/blklogwrites.c | 10 ++++------ > block/blkreplay.c | 6 ++---- > block/blkverify.c | 11 ++++------- > block/qcow2.c | 5 ++--- > block/quorum.c | 6 ++---- > 6 files changed, 16 insertions(+), 28 deletions(-) And this diffstat doesn't immediately tell me what ended up violating the assumptions of error_abort. As such, at this point I'm temporarily dropping the remainder of the series from my bitmaps queue, and only including patches 1 and 2 in my next pull request. Looking forward to v8.
Am 13.02.2021 um 00:13 hat Eric Blake geschrieben: > On 2/2/21 6:49 AM, 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 > > With this patch applied, 'check unit-test' fails with: > > Running test test-replication > Unexpected error in bdrv_open_driver() at ../block.c:1481: > Could not open '/tmp/p_local_disk.z1Ugyc': Invalid argument > ERROR test-replication - missing test plan > > Directly reverting it has ripple effect on later patches in the series. > > Running test-replication under gdb gives this backtrace: > > Thread 1 "test-replicatio" received signal SIGABRT, Aborted. > 0x00007ffff6f6f9d5 in raise () from /lib64/libc.so.6 > (gdb) bt > #0 0x00007ffff6f6f9d5 in raise () from /lib64/libc.so.6 > #1 0x00007ffff6f588a4 in abort () from /lib64/libc.so.6 > #2 0x00005555556ad820 in error_handle_fatal ( > errp=0x555555790568 <error_abort>, err=0x555555859010) > at ../util/error.c:40 > #3 0x00005555556ae3cf in error_propagate ( > dst_errp=0x555555790568 <error_abort>, local_err=0x555555859010) > at ../util/error.c:286 > #4 0x000055555558cc9e in bdrv_img_create ( > filename=0x555555822500 "/tmp/p_local_disk.DVFoWt", > fmt=0x5555556e809a "qcow2", base_filename=0x0, base_fmt=0x0, > options=0x0, > img_size=67108864, flags=2, quiet=true, errp=0x555555790568 > <error_abort>) > at ../block.c:6312 The problem is this hunk: diff --git a/block/qcow2.c b/block/qcow2.c index 5d94f45be9..e8dd42d73b 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1611,9 +1611,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; } bdrv_open_child() can return NULL in non-error cases, when the child is optional and it isn't given. The test doesn't use an external data file, so this returns NULL without setting an error, which now gets turned into -EINVAL instead. This makes the most basic tests fail with qcow2 (iotests 001 is enough). The other hunks in this patch don't suffer from the same problem because they pass allow_none=false. Kevin
On 2/15/21 3:22 AM, Kevin Wolf wrote: >> With this patch applied, 'check unit-test' fails with: >> >> Running test test-replication >> Unexpected error in bdrv_open_driver() at ../block.c:1481: >> Could not open '/tmp/p_local_disk.z1Ugyc': Invalid argument >> ERROR test-replication - missing test plan >> > The problem is this hunk: > > diff --git a/block/qcow2.c b/block/qcow2.c > index 5d94f45be9..e8dd42d73b 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -1611,9 +1611,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; > } > > bdrv_open_child() can return NULL in non-error cases, when the child is > optional and it isn't given. The test doesn't use an external data file, > so this returns NULL without setting an error, which now gets turned > into -EINVAL instead. > > This makes the most basic tests fail with qcow2 (iotests 001 is enough). > > The other hunks in this patch don't suffer from the same problem because > they pass allow_none=false. Thanks; that's enough to figure out how to repair the patch: diff --git i/block/qcow2.c w/block/qcow2.c index e8dd42d73b4c..38137ca30eb0 100644 --- i/block/qcow2.c +++ w/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; @@ -1612,7 +1613,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, s->data_file = bdrv_open_child(NULL, options, "data-file", bs, &child_of_bds, BDRV_CHILD_DATA, true, errp); - if (!s->data_file) { + if (*errp) { ret = -EINVAL; goto fail; }
15.02.2021 23:04, Eric Blake wrote: > On 2/15/21 3:22 AM, Kevin Wolf wrote: > >>> With this patch applied, 'check unit-test' fails with: >>> >>> Running test test-replication >>> Unexpected error in bdrv_open_driver() at ../block.c:1481: >>> Could not open '/tmp/p_local_disk.z1Ugyc': Invalid argument >>> ERROR test-replication - missing test plan >>> > >> The problem is this hunk: >> >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 5d94f45be9..e8dd42d73b 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -1611,9 +1611,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; >> } >> >> bdrv_open_child() can return NULL in non-error cases, when the child is >> optional and it isn't given. The test doesn't use an external data file, >> so this returns NULL without setting an error, which now gets turned >> into -EINVAL instead. >> >> This makes the most basic tests fail with qcow2 (iotests 001 is enough). >> >> The other hunks in this patch don't suffer from the same problem because >> they pass allow_none=false. > > Thanks; that's enough to figure out how to repair the patch: > > diff --git i/block/qcow2.c w/block/qcow2.c > index e8dd42d73b4c..38137ca30eb0 100644 > --- i/block/qcow2.c > +++ w/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; > @@ -1612,7 +1613,7 @@ static int coroutine_fn > qcow2_do_open(BlockDriverState *bs, QDict *options, > s->data_file = bdrv_open_child(NULL, options, "data-file", bs, > &child_of_bds, BDRV_CHILD_DATA, > true, errp); > - if (!s->data_file) { > + if (*errp) { > ret = -EINVAL; > goto fail; > } > > Agree.. Or better refactor bdrv_open_child to follow more common (and recommended) semantics (i.e. NULL + errp on error, non-null on succsess).. But this will require more investigation. I'm busy now with our internal deadline (this week), after this will return to handle upstream things.
Am 16.02.2021 um 06:03 hat Vladimir Sementsov-Ogievskiy geschrieben: > 15.02.2021 23:04, Eric Blake wrote: > > On 2/15/21 3:22 AM, Kevin Wolf wrote: > > > > > > With this patch applied, 'check unit-test' fails with: > > > > > > > > Running test test-replication > > > > Unexpected error in bdrv_open_driver() at ../block.c:1481: > > > > Could not open '/tmp/p_local_disk.z1Ugyc': Invalid argument > > > > ERROR test-replication - missing test plan > > > > > > > > > The problem is this hunk: > > > > > > diff --git a/block/qcow2.c b/block/qcow2.c > > > index 5d94f45be9..e8dd42d73b 100644 > > > --- a/block/qcow2.c > > > +++ b/block/qcow2.c > > > @@ -1611,9 +1611,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; > > > } > > > > > > bdrv_open_child() can return NULL in non-error cases, when the child is > > > optional and it isn't given. The test doesn't use an external data file, > > > so this returns NULL without setting an error, which now gets turned > > > into -EINVAL instead. > > > > > > This makes the most basic tests fail with qcow2 (iotests 001 is enough). > > > > > > The other hunks in this patch don't suffer from the same problem because > > > they pass allow_none=false. > > > > Thanks; that's enough to figure out how to repair the patch: > > > > diff --git i/block/qcow2.c w/block/qcow2.c > > index e8dd42d73b4c..38137ca30eb0 100644 > > --- i/block/qcow2.c > > +++ w/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; > > @@ -1612,7 +1613,7 @@ static int coroutine_fn > > qcow2_do_open(BlockDriverState *bs, QDict *options, > > s->data_file = bdrv_open_child(NULL, options, "data-file", bs, > > &child_of_bds, BDRV_CHILD_DATA, > > true, errp); > > - if (!s->data_file) { > > + if (*errp) { > > ret = -EINVAL; > > goto fail; > > } > > Agree.. Or better refactor bdrv_open_child to follow more common (and > recommended) semantics (i.e. NULL + errp on error, non-null on > succsess).. But this will require more investigation. But what non-NULL value to return when there is no BdrvChild object? If anything, you could switch to an int return value and pass the BdrvChild pointer by reference. I'm not sure if that would be a massive improvement, though. Kevin
diff --git a/block/blkdebug.c b/block/blkdebug.c index 5fe6172da9..315965a013 100644 --- a/block/blkdebug.c +++ b/block/blkdebug.c @@ -465,7 +465,6 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, { BDRVBlkdebugState *s = bs->opaque; QemuOpts *opts; - Error *local_err = NULL; int ret; uint64_t align; @@ -495,10 +494,9 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags, bs->file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "image", bs, &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, - false, &local_err); - if (local_err) { + false, errp); + if (!bs->file) { ret = -EINVAL; - error_propagate(errp, local_err); goto out; } diff --git a/block/blklogwrites.c b/block/blklogwrites.c index 13ae63983b..b7579370a3 100644 --- a/block/blklogwrites.c +++ b/block/blklogwrites.c @@ -157,19 +157,17 @@ static int blk_log_writes_open(BlockDriverState *bs, QDict *options, int flags, /* Open the file */ bs->file = bdrv_open_child(NULL, options, "file", bs, &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, false, - &local_err); - if (local_err) { + errp); + if (!bs->file) { ret = -EINVAL; - error_propagate(errp, local_err); goto fail; } /* Open the log file */ s->log_file = bdrv_open_child(NULL, options, "log", bs, &child_of_bds, - BDRV_CHILD_METADATA, false, &local_err); - if (local_err) { + BDRV_CHILD_METADATA, false, errp); + if (!s->log_file) { ret = -EINVAL; - error_propagate(errp, local_err); goto fail; } diff --git a/block/blkreplay.c b/block/blkreplay.c index 30a0f5d57a..4a247752fd 100644 --- a/block/blkreplay.c +++ b/block/blkreplay.c @@ -23,16 +23,14 @@ typedef struct Request { static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { - Error *local_err = NULL; int ret; /* Open the image file */ bs->file = bdrv_open_child(NULL, options, "image", bs, &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, - false, &local_err); - if (local_err) { + false, errp); + if (!bs->file) { ret = -EINVAL; - error_propagate(errp, local_err); goto fail; } diff --git a/block/blkverify.c b/block/blkverify.c index 4aed53ab59..95ae73e2aa 100644 --- a/block/blkverify.c +++ b/block/blkverify.c @@ -112,7 +112,6 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags, { BDRVBlkverifyState *s = bs->opaque; QemuOpts *opts; - Error *local_err = NULL; int ret; opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); @@ -125,20 +124,18 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags, bs->file = bdrv_open_child(qemu_opt_get(opts, "x-raw"), options, "raw", bs, &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, - false, &local_err); - if (local_err) { + false, errp); + if (!bs->file) { ret = -EINVAL; - error_propagate(errp, local_err); goto fail; } /* Open the test file */ s->test_file = bdrv_open_child(qemu_opt_get(opts, "x-image"), options, "test", bs, &child_of_bds, BDRV_CHILD_DATA, - false, &local_err); - if (local_err) { + false, errp); + if (!s->test_file) { ret = -EINVAL; - error_propagate(errp, local_err); goto fail; } diff --git a/block/qcow2.c b/block/qcow2.c index 5d94f45be9..e8dd42d73b 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1611,9 +1611,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 0bd75450de..cfc1436abb 100644 --- a/block/quorum.c +++ b/block/quorum.c @@ -929,7 +929,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; @@ -1007,9 +1006,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; }
This patch is generated by cocci script: @@ symbol bdrv_open_child, errp, local_err; expression file; @@ file = bdrv_open_child(..., - &local_err + errp ); - if (local_err) + if (!file) { ... - error_propagate(errp, local_err); ... } with command spatch --sp-file x.cocci --macro-file scripts/cocci-macro-file.h \ --in-place --no-show-diff --max-width 80 --use-gitgrep block Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Reviewed-by: Greg Kurz <groug@kaod.org> Reviewed-by: Alberto Garcia <berto@igalia.com> --- block/blkdebug.c | 6 ++---- block/blklogwrites.c | 10 ++++------ block/blkreplay.c | 6 ++---- block/blkverify.c | 11 ++++------- block/qcow2.c | 5 ++--- block/quorum.c | 6 ++---- 6 files changed, 16 insertions(+), 28 deletions(-)