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

[Markus Armbruster <armbru@redhat.com>]

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) {