Re: [PATCH v2 19/44] block/parallels: Simplify parallels_open() after previous commit

[Vladimir Sementsov-Ogievskiy]

02.07.2020 18:49, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   block/parallels.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/block/parallels.c b/block/parallels.c
> index 32d0ecd398..e0ec819550 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -843,6 +843,7 @@ static int parallels_open(BlockDriverState *bs, QDict 
> *options, int flags,
>                                          &local_err);
>       g_free(buf);
>       if (local_err != NULL) {
> +        error_propagate(errp, local_err);
>           goto fail_options;
>       }
>   
> @@ -873,15 +874,11 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
>   
>   fail_format:
>       error_setg(errp, "Image not in Parallels format");
> +fail_options:
>       ret = -EINVAL;
>   fail:
>       qemu_vfree(s->header);
>       return ret;
> -
> -fail_options:
> -    error_propagate(errp, local_err);
> -    ret = -EINVAL;
> -    goto fail;
>   }
>   
>   

You leak local_err in one case. With at least:

diff --git a/block/parallels.c b/block/parallels.c
index e0ec819550..5c1940ee02 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -829,7 +829,7 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
         goto fail_options;
     }
 
-    if (!qemu_opts_absorb_qdict(opts, options, &local_err)) {
+    if (!qemu_opts_absorb_qdict(opts, options, errp)) {
         goto fail_options;
     }


squashed-in:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


Still, if we have a special patch for the this function, we can get rid of one 
more propagation:

diff --git a/block/parallels.c b/block/parallels.c
index e0ec819550..d4ad83ac19 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -829,7 +829,7 @@ static int parallels_open(BlockDriverState *bs, QDict 
*options, int flags,
         goto fail_options;
     }
 
-    if (!qemu_opts_absorb_qdict(opts, options, &local_err)) {
+    if (!qemu_opts_absorb_qdict(opts, options, errp)) {
         goto fail_options;
     }
 
@@ -863,9 +863,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
     error_setg(&s->migration_blocker, "The Parallels format used by node '%s' "
                "does not support live migration",
                bdrv_get_device_or_node_name(bs));
-    ret = migrate_add_blocker(s->migration_blocker, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    ret = migrate_add_blocker(s->migration_blocker, errp);
+    if (ret < 0) {
         error_free(s->migration_blocker);
         goto fail;
     }


with it, as well:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

--
Best regards,
Vladimir