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

[Vladimir Sementsov-Ogievskiy]

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