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

[Markus Armbruster]

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