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 <arm...@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 <vsement...@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