Re: [PATCH v2 33/44] error: Avoid unnecessary error_propagate() after error_setg()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 7/2/20 10:49 AM, Markus Armbruster wrote:
>> Replace
>>
>>      error_setg(&err, ...);
>>      error_propagate(errp, err);
>>
>> by
>>
>>      error_setg(errp, ...);
>>
>
>>
>> Candidates for conversion tracked down with this Coccinelle script:
>>
>>      @@
>>      identifier err, errp;
>>      expression list args;
>>      @@
>>      -    error_setg(&err, args);
>>      +    error_setg(errp, args);
>>       ... when != err
>>       error_propagate(errp, err);
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +++ b/backends/cryptodev.c
>> @@ -158,16 +158,15 @@ cryptodev_backend_set_queues(Object *obj, Visitor *v, 
>> const char *name,
>>       uint32_t value;
>>         if (!visit_type_uint32(v, name, &value, &local_err)) {
>> -        goto out;
>> +        error_propagate(errp, local_err);
>> +        return;
>>       }
>
> Looks like this error_propgate is spurious if you just use
>  if (!visit_type_uint32(..., errp)) {
>
> Oh - that's not the pattern you flagged, and a later patch then
> addresses it.  It might help if the commit message for this patch
> mentions that further cleanups are still forthcoming.

Perhaps:

    In some places, the transformation results in obviously unnecessary
    error_propagate().  The next few commits will eliminate them.

>> +++ b/backends/hostmem-file.c
>> @@ -114,18 +114,16 @@ static void file_memory_backend_set_align(Object *o, 
>> Visitor *v,
>>       uint64_t val;
>>         if (host_memory_backend_mr_inited(backend)) {
>> -        error_setg(&local_err, "cannot change property '%s' of %s",
>> -                   name, object_get_typename(o));
>> -        goto out;
>> +        error_setg(errp, "cannot change property '%s' of %s", name,
>> +                   object_get_typename(o));
>> +        return;
>>       }
>>         if (!visit_type_size(v, name, &val, &local_err)) {
>> -        goto out;
>> +        error_propagate(errp, local_err);
>> +        return;
>>       }
>
> Another case where the first 'if' matches the subject of this patch,
> and the second 'if' can avoid local_err but that the change will be
> done in a later patch.  And several more later on, but this is how far
> it took me to realize that you intentionally saved them for later.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!