| Message ID | 20210109125811.209870-15-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: deal with errp: part I | expand |
On Sat 09 Jan 2021 01:58:11 PM CET, Vladimir Sementsov-Ogievskiy wrote: > Keep setting ret close to setting errp and don't merge different error > paths into one. This way it's more obvious that we don't return > error without setting errp. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> I get the idea, but I feel the code is getting innecessarily verbose. One alternative would be to get rid of all -EINVAL inside the switch block, take advantage of the existing local_err and follow the block with: if (local_err) { error_propagate(errp, local_err); ret = -EINVAL; goto fail; } But otherwise your solution is correct, so you can keep it if you prefer: Reviewed-by: Alberto Garcia <berto@igalia.com> Berto
11.01.2021 19:08, Alberto Garcia wrote: > On Sat 09 Jan 2021 01:58:11 PM CET, Vladimir Sementsov-Ogievskiy wrote: >> Keep setting ret close to setting errp and don't merge different error >> paths into one. This way it's more obvious that we don't return >> error without setting errp. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > > I get the idea, but I feel the code is getting innecessarily verbose. > > One alternative would be to get rid of all -EINVAL inside the switch > block, take advantage of the existing local_err and follow the block > with: > > if (local_err) { > error_propagate(errp, local_err); > ret = -EINVAL; > goto fail; > } Actually in our new paradigm of avoiding error propagation (as well as void functions with errp argument), we should first update read_cache_sizes() to return the value together with setting errp, then we will update read_cache_sizes() call in qcow2_update_options_prepare() and drop local_err from qcow2_update_options_prepare() at all. > > But otherwise your solution is correct, so you can keep it if you > prefer: > > Reviewed-by: Alberto Garcia <berto@igalia.com> > Thanks!
12.01.2021 00:17, Vladimir Sementsov-Ogievskiy wrote: > 11.01.2021 19:08, Alberto Garcia wrote: >> On Sat 09 Jan 2021 01:58:11 PM CET, Vladimir Sementsov-Ogievskiy wrote: >>> Keep setting ret close to setting errp and don't merge different error >>> paths into one. This way it's more obvious that we don't return >>> error without setting errp. >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> >> I get the idea, but I feel the code is getting innecessarily verbose. >> >> One alternative would be to get rid of all -EINVAL inside the switch >> block, take advantage of the existing local_err and follow the block >> with: >> >> if (local_err) { >> error_propagate(errp, local_err); >> ret = -EINVAL; >> goto fail; >> } > > Actually in our new paradigm of avoiding error propagation (as well as void functions with errp argument), we should first update read_cache_sizes() to return the value together with setting errp, then we will update read_cache_sizes() call in qcow2_update_options_prepare() and drop local_err from qcow2_update_options_prepare() at all. Which is actually done during the series, so there is no local_err ;) > >> >> But otherwise your solution is correct, so you can keep it if you >> prefer: >> >> Reviewed-by: Alberto Garcia <berto@igalia.com> >> > > Thanks! > >
diff --git a/block/qcow2.c b/block/qcow2.c index 436bcf0a97..0aa6ae1e1f 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1158,6 +1158,10 @@ static int qcow2_update_options_prepare(BlockDriverState *bs, } qdict_put_str(encryptopts, "format", "qcow"); r->crypto_opts = block_crypto_open_opts_init(encryptopts, errp); + if (!r->crypto_opts) { + ret = -EINVAL; + goto fail; + } break; case QCOW_CRYPT_LUKS: @@ -1170,14 +1174,15 @@ static int qcow2_update_options_prepare(BlockDriverState *bs, } qdict_put_str(encryptopts, "format", "luks"); r->crypto_opts = block_crypto_open_opts_init(encryptopts, errp); + if (!r->crypto_opts) { + ret = -EINVAL; + goto fail; + } break; default: error_setg(errp, "Unsupported encryption method %d", s->crypt_method_header); - break; - } - if (s->crypt_method_header != QCOW_CRYPT_NONE && !r->crypto_opts) { ret = -EINVAL; goto fail; }
Keep setting ret close to setting errp and don't merge different error paths into one. This way it's more obvious that we don't return error without setting errp. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/qcow2.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)