[v2] Less clumsy error checking

[PATCH v2 25/44] qom: Use return values to check for error where that's simpler

Markus Armbruster posted 44 patches 5 years, 7 months ago

Diff against v1 v3 v4
Download mbox

Maintainers: Andrey Smirnov <andrew.smirnov@gmail.com>, Antony Pavlov <antonynpavlov@gmail.com>, Eric Blake <eblake@redhat.com>, Matthew Rosato <mjrosato@linux.ibm.com>, John Snow <jsnow@redhat.com>, Jason Wang <jasowang@redhat.com>, Richard Henderson <rth@twiddle.net>, Kevin Wolf <kwolf@redhat.com>, Paul Burton <pburton@wavecomp.com>, Artyom Tarasenko <atar4qemu@gmail.com>, Zhang Chen <chen.zhang@intel.com>, "Michael S. Tsirkin" <mst@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Paul Durrant <paul@xen.org>, Christian Borntraeger <borntraeger@de.ibm.com>, David Hildenbrand <david@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>, Max Reitz <mreitz@redhat.com>, "Dr. David Alan Gilbert" <dgilbert@redhat.com>, Joel Stanley <joel@jms.id.au>, Igor Mammedov <imammedo@redhat.com>, Andrew Baumann <Andrew.Baumann@microsoft.com>, Wen Congyang <wencongyang2@huawei.com>, Liu Yuan <namei.unix@gmail.com>, Laurent Vivier <lvivier@redhat.com>, Jeff Cody <codyprime@gmail.com>, Stefan Berger <stefanb@linux.ibm.com>, Riku Voipio <riku.voipio@iki.fi>, Ronnie Sahlberg <ronniesahlberg@gmail.com>, Ari Sundholm <ari@tuxera.com>, "Philippe Mathieu-Daudé" <philmd@redhat.com>, Jan Kiszka <jan.kiszka@web.de>, Michael Roth <mdroth@linux.vnet.ibm.com>, Alex Williamson <alex.williamson@redhat.com>, Li Zhijian <lizhijian@cn.fujitsu.com>, Beniamino Galvani <b.galvani@gmail.com>, Alistair Francis <alistair@alistair23.me>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Stefan Weil <sw@weilnetz.de>, Yoshinori Sato <ysato@users.sourceforge.jp>, Aurelien Jarno <aurelien@aurel32.net>, "Gonglei (Arei)" <arei.gonglei@huawei.com>, Leif Lindholm <leif@nuviainc.com>, "Hervé Poussineau" <hpoussin@reactos.org>, "Edgar E. Iglesias" <edgar.iglesias@gmail.com>, Palmer Dabbelt <palmer@dabbelt.com>, Peter Lieven <pl@kamp.de>, Eric Auger <eric.auger@redhat.com>, Xiao Guangrong <xiaoguangrong.eric@gmail.com>, Alberto Garcia <berto@igalia.com>, Niek Linnenbank <nieklinnenbank@gmail.com>, Xie Changlong <xiechanglong.d@gmail.com>, Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>, Markus Armbruster <armbru@redhat.com>, Stefano Stabellini <sstabellini@kernel.org>, Fam Zheng <fam@euphon.net>, "Daniel P. Berrangé" <berrange@redhat.com>, Sagar Karandikar <sagark@eecs.berkeley.edu>, Igor Mitsyanko <i.mitsyanko@gmail.com>, Gerd Hoffmann <kraxel@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>, "Marc-André Lureau" <marcandre.lureau@redhat.com>, Peter Chubb <peter.chubb@nicta.com.au>, "Denis V. Lunev" <den@openvz.org>, Halil Pasic <pasic@linux.ibm.com>, Anthony Perard <anthony.perard@citrix.com>, Andrew Jeffery <andrew@aj.id.au>, Jason Dillaman <dillaman@redhat.com>, Rob Herring <robh@kernel.org>, "Richard W.M. Jones" <rjones@redhat.com>, "Cédric Le Goater" <clg@kaod.org>, Subbaraya Sundeep <sundeep.lkml@gmail.com>, Peter Maydell <peter.maydell@linaro.org>, Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>, Jean-Christophe Dubois <jcd@tribudubois.net>, David Gibson <david@gibson.dropbear.id.au>, Bastian Koppelmann <kbastian@mail.uni-paderborn.de>, Radoslaw Biernacki <radoslaw.biernacki@linaro.org>, Thomas Huth <thuth@redhat.com>, Amit Shah <amit@kernel.org>, Paolo Bonzini <pbonzini@redhat.com>

There is a newer version of this series

Expand all Fold all

[PATCH v2 25/44] qom: Use return values to check for error where that's simpler

Posted by Markus Armbruster 5 years, 7 months ago

When using the Error object to check for error, we need to receive it
into a local variable, then propagate() it to @errp.

Using the return value permits allows receiving it straight to @errp.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 qom/object.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/qom/object.c b/qom/object.c
index 0808da2767..56d858b6a5 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -549,8 +549,7 @@ void object_initialize_child_with_propsv(Object *parentobj,
     object_initialize(childobj, size, type);
     obj = OBJECT(childobj);
 
-    object_set_propv(obj, &local_err, vargs);
-    if (local_err) {
+    if (object_set_propv(obj, errp, vargs) < 0) {
         goto out;
     }
 
@@ -743,7 +742,7 @@ Object *object_new_with_propv(const char *typename,
     }
     obj = object_new_with_type(klass->type);
 
-    if (object_set_propv(obj, &local_err, vargs) < 0) {
+    if (object_set_propv(obj, errp, vargs) < 0) {
         goto error;
     }
 
@@ -1767,14 +1766,17 @@ static void object_set_link_property(Object *obj, Visitor *v,
     char *path = NULL;
 
     visit_type_str(v, name, &path, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
 
-    if (!local_err && strcmp(path, "") != 0) {
-        new_target = object_resolve_link(obj, name, path, &local_err);
+    if (strcmp(path, "") != 0) {
+        new_target = object_resolve_link(obj, name, path, errp);
     }
 
     g_free(path);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    if (!new_target) {
         return;
     }
 
-- 
2.26.2

Re: [PATCH v2 25/44] qom: Use return values to check for error where that's simpler

Posted by Vladimir Sementsov-Ogievskiy 5 years, 7 months ago

02.07.2020 18:49, Markus Armbruster wrote:
> When using the Error object to check for error, we need to receive it
> into a local variable, then propagate() it to @errp.
> 
> Using the return value permits allows receiving it straight to @errp.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>   qom/object.c | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 0808da2767..56d858b6a5 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -549,8 +549,7 @@ void object_initialize_child_with_propsv(Object *parentobj,
>       object_initialize(childobj, size, type);
>       obj = OBJECT(childobj);
>   
> -    object_set_propv(obj, &local_err, vargs);
> -    if (local_err) {
> +    if (object_set_propv(obj, errp, vargs) < 0) {
>           goto out;
>       }
>   
> @@ -743,7 +742,7 @@ Object *object_new_with_propv(const char *typename,
>       }
>       obj = object_new_with_type(klass->type);
>   
> -    if (object_set_propv(obj, &local_err, vargs) < 0) {
> +    if (object_set_propv(obj, errp, vargs) < 0) {
>           goto error;
>       }
>   
> @@ -1767,14 +1766,17 @@ static void object_set_link_property(Object *obj, Visitor *v,
>       char *path = NULL;
>   
>       visit_type_str(v, name, &path, &local_err);

why not use return value of visit_type_str ?

> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        return;
> +    }
>   
> -    if (!local_err && strcmp(path, "") != 0) {
> -        new_target = object_resolve_link(obj, name, path, &local_err);
> +    if (strcmp(path, "") != 0) {
> +        new_target = object_resolve_link(obj, name, path, errp);
>       }

Hmmm. You actually change the logic when visit_type_str succeeded but path equal to "":

prepatch, we continue processing with new_target == NULL, after the patch we just do nothing and report success (errp == NULL).

I don't know whether pre-patch or after-patch behavior is correct, but if it is a logic change, let's note it in the commit message, if path equal to "" actually impossible, let's assert it. Or just keep old logic as is, by moving return (together with duplicated g_free(path) of course) into "if (strcmp(path, "") != 0) {".

>   
>       g_free(path);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    if (!new_target) {
>           return;
>       }
>   
> 


-- 
Best regards,
Vladimir

Re: [PATCH v2 25/44] qom: Use return values to check for error where that's simpler

Posted by Markus Armbruster 5 years, 7 months ago

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

> 02.07.2020 18:49, Markus Armbruster wrote:
>> When using the Error object to check for error, we need to receive it
>> into a local variable, then propagate() it to @errp.
>>
>> Using the return value permits allows receiving it straight to @errp.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   qom/object.c | 16 +++++++++-------
>>   1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 0808da2767..56d858b6a5 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -549,8 +549,7 @@ void object_initialize_child_with_propsv(Object *parentobj,
>>       object_initialize(childobj, size, type);
>>       obj = OBJECT(childobj);
>>   -    object_set_propv(obj, &local_err, vargs);
>> -    if (local_err) {
>> +    if (object_set_propv(obj, errp, vargs) < 0) {
>>           goto out;
>>       }
>>   @@ -743,7 +742,7 @@ Object *object_new_with_propv(const char
>> *typename,
>>       }
>>       obj = object_new_with_type(klass->type);
>>   -    if (object_set_propv(obj, &local_err, vargs) < 0) {
>> +    if (object_set_propv(obj, errp, vargs) < 0) {
>>           goto error;
>>       }
>>   @@ -1767,14 +1766,17 @@ static void
>> object_set_link_property(Object *obj, Visitor *v,
>>       char *path = NULL;
>>         visit_type_str(v, name, &path, &local_err);
>
> why not use return value of visit_type_str ?

Yes, that's better.

>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>>   -    if (!local_err && strcmp(path, "") != 0) {
>> -        new_target = object_resolve_link(obj, name, path, &local_err);
>> +    if (strcmp(path, "") != 0) {
>> +        new_target = object_resolve_link(obj, name, path, errp);
>>       }
>
> Hmmm. You actually change the logic when visit_type_str succeeded but path equal to "":
>
> prepatch, we continue processing with new_target == NULL, after the patch we just do nothing and report success (errp == NULL).
>
> I don't know whether pre-patch or after-patch behavior is correct, but if it is a logic change, let's note it in the commit message, if path equal to "" actually impossible, let's assert it. Or just keep old logic as is, by moving return (together with duplicated g_free(path) of course) into "if (strcmp(path, "") != 0) {".

After having another stare at the function, I conclude it's awful before
the patch, and only slightly less awful but also wrong after.

>>         g_free(path);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> +    if (!new_target) {
>>           return;
>>       }

What about this:

   @@ -1763,20 +1762,24 @@ static void object_set_link_property(Object *obj, Visitor *v,
        LinkProperty *prop = opaque;
        Object **targetp = object_link_get_targetp(obj, prop);
        Object *old_target = *targetp;
   -    Object *new_target = NULL;
   +    Object *new_target;
        char *path = NULL;

   -    visit_type_str(v, name, &path, &local_err);
   +    if (!visit_type_str(v, name, &path, errp)) {
   +        return;
   +    }

   -    if (!local_err && strcmp(path, "") != 0) {
   -        new_target = object_resolve_link(obj, name, path, &local_err);
   +    if (*path) {
   +        new_target = object_resolve_link(obj, name, path, errp);
   +        if (!new_target) {
   +            g_free(path);
   +            return;
   +        }
   +    } else {
   +        new_target = NULL;
        }

        g_free(path);
   -    if (local_err) {
   -        error_propagate(errp, local_err);
   -        return;
   -    }

        prop->check(obj, name, new_target, &local_err);
        if (local_err) {