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

[Markus Armbruster]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

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

You're right, that's wrong.  Missed when I reordered my patches.

This PATCH needs to go after "[PATCH v2 37/44] error: Reduce unnecessary
error propagation", which has this hunk.

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

I'll keep your R-by there, hope that's okay.

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

This additional hunk is part of PATCH 41.
>
>
> with it, as well:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks!