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

[Vladimir Sementsov-Ogievskiy]

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

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


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

>   
>       ZeroMemory(&sys_pwr_caps, sizeof(sys_pwr_caps));
>       if (!GetPwrCapabilities(&sys_pwr_caps)) {
> -        error_setg(&local_err, QERR_QGA_COMMAND_FAILED,
> +        error_setg(errp, QERR_QGA_COMMAND_FAILED,
>                      "failed to determine guest suspend capabilities");
> -        goto out;
> +        return;
>       }
>   
>       switch (mode) {
>       case GUEST_SUSPEND_MODE_DISK:
>           if (!sys_pwr_caps.SystemS4) {
> -            error_setg(&local_err, QERR_QGA_COMMAND_FAILED,
> +            error_setg(errp, QERR_QGA_COMMAND_FAILED,
>                          "suspend-to-disk not supported by OS");
>           }
>           break;
>       case GUEST_SUSPEND_MODE_RAM:
>           if (!sys_pwr_caps.SystemS3) {
> -            error_setg(&local_err, QERR_QGA_COMMAND_FAILED,
> +            error_setg(errp, QERR_QGA_COMMAND_FAILED,
>                          "suspend-to-ram not supported by OS");
>           }
>           break;
>       default:
> -        error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "mode",
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "mode",
>                      "GuestSuspendMode");
>       }
> -
> -out:
> -    error_propagate(errp, local_err);
>   }
>   
>   static DWORD WINAPI do_suspend(LPVOID opaque)


--
Best regards,
Vladimir