diff mbox series

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

Message ID 20200702155000.3455325-20-armbru@redhat.com (mailing list archive)
State New, archived
Headers show
Series Less clumsy error checking | expand

Commit Message

Markus Armbruster July 2, 2020, 3:49 p.m. UTC 
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(-)

Comments

Vladimir Sementsov-Ogievskiy July 3, 2020, 3:29 p.m. UTC | #1 
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>
Markus Armbruster July 4, 2020, 1:28 p.m. UTC | #2 
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!
diff mbox series

Patch

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