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(-)

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;
     }
-- 
2.29.2

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!


-- 
Best regards,
Vladimir

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!
> 
> 


-- 
Best regards,
Vladimir