Re: [PATCH 08/46] error: Avoid unnecessary error_propagate() after error_setg()

[Markus Armbruster]

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

> 24.06.2020 19:43, Markus Armbruster wrote:
>> Replace
>>
>>      error_setg(&err, ...);
>>      error_propagate(errp, err);
>>
>> by
>>
>>      error_setg(errp, ...);
>>
>> Related pattern:
>>
>>      if (...) {
>>          error_setg(&err, ...);
>>          goto out;
>>      }
>>      ...
>>   out:
>>      error_propagate(errp, err);
>>      return;
>>
>> When all paths to label out are that way, replace by
>>
>>      if (...) {
>>          error_setg(errp, ...);
>>          return;
>>      }
>>
>> and delete the label along with the error_propagate().
>>
>> When we have at most one other path that actually needs to propagate,
>> and maybe one at the end that where propagation is unnecessary, e.g.
>>
>>      foo(..., &err);
>>      if (err) {
>>          goto out;
>>      }
>>      ...
>>      bar(..., &err);
>>   out:
>>      error_propagate(errp, err);
>>      return;
>>
>> move the error_propagate() to where it's needed, like
>>
>>      if (...) {
>>          foo(..., &err);
>>          error_propagate(errp, err);
>>          return;
>>      }
>>      ...
>>      bar(..., errp);
>>      return;
>>
>> and transform the error_setg() as above.
>>
>> Bonus: the elimination of gotos will make later patches in this series
>> easier to review.
>>
>> 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>
>> ---
>>   backends/cryptodev.c        | 11 +++---
>>   backends/hostmem-file.c     | 19 +++-------
>>   backends/hostmem-memfd.c    | 15 ++++----
>>   backends/hostmem.c          | 27 ++++++--------
>>   block/throttle-groups.c     | 22 +++++------
>>   hw/hyperv/vmbus.c           |  5 +--
>>   hw/i386/pc.c                | 35 ++++++------------
>>   hw/mem/nvdimm.c             | 17 ++++-----
>>   hw/mem/pc-dimm.c            | 14 +++----
>>   hw/misc/aspeed_sdmc.c       |  3 +-
>>   hw/ppc/rs6000_mc.c          |  9 ++---
>>   hw/ppc/spapr.c              | 73 ++++++++++++++++---------------------
>>   hw/ppc/spapr_pci.c          | 14 +++----
>>   hw/s390x/ipl.c              | 23 +++++-------
>>   hw/s390x/sclp.c             | 12 ++----
>>   hw/xen/xen_pt_config_init.c |  3 +-
>>   iothread.c                  | 12 +++---
>>   net/colo-compare.c          | 20 ++++------
>>   net/dump.c                  | 10 ++---
>>   net/filter-buffer.c         | 10 ++---
>>   qga/commands-win32.c        | 16 +++-----
>>   21 files changed, 151 insertions(+), 219 deletions(-)
>>
>
> [..]
>
>> --- a/qga/commands-win32.c
>> +++ b/qga/commands-win32.c
>> @@ -282,9 +282,8 @@ static void execute_async(DWORD WINAPI (*func)(LPVOID), 
>> LPVOID opaque,
>
> You forget to remove unused local_err variable

In my haste to get this series out for review, I took undadvisable
shortcuts on Error * variable cleanup.  Need to do better for v2.

>>         HANDLE thread = CreateThread(NULL, 0, func, opaque, 0,
>> NULL);
>>       if (!thread) {
>> -        error_setg(&local_err, QERR_QGA_COMMAND_FAILED,
>> +        error_setg(errp, QERR_QGA_COMMAND_FAILED,
>>                      "failed to dispatch asynchronous command");
>> -        error_propagate(errp, local_err);
>>       }
>>   }
>>   @@ -1274,31 +1273,28 @@ static void
>> check_suspend_mode(GuestSuspendMode mode, Error **errp)
>
> and here (I assume, you remove unused variables with help of compiler, but 
> don't compile for win32 :)
>
>
> with these two local_err removed:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Thanks!

> Ohh, my brain is broken, I'd prefer to create such patches than to review 
> them :)

Rrrrrevenge!  ;)

[...]