diff mbox series

[RFC,v5,117/126] Replication: introduce ERRP_AUTO_PROPAGATE

Message ID 20191011160552.22907-118-vsementsov@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series error: auto propagated local_err | expand

Commit Message

Vladimir Sementsov-Ogievskiy Oct. 11, 2019, 4:05 p.m. UTC
If we want to add some info to errp (by error_prepend() or
error_append_hint()), we must use the ERRP_AUTO_PROPAGATE macro.
Otherwise, this info will not be added when errp == &fatal_err
(the program will exit prior to the error_append_hint() or
error_prepend() call).  Fix such cases.

If we want to check error after errp-function call, we need to
introduce local_err and than propagate it to errp. Instead, use
ERRP_AUTO_PROPAGATE macro, benefits are:
1. No need of explicit error_propagate call
2. No need of explicit local_err variable: use errp directly
3. ERRP_AUTO_PROPAGATE leaves errp as is if it's not NULL or
   &error_fatel, this means that we don't break error_abort
   (we'll abort on error_set, not on error_propagate)

This commit (together with its neighbors) was generated by

for f in $(git grep -l errp \*.[ch]); do \
    spatch --sp-file scripts/coccinelle/auto-propagated-errp.cocci \
    --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff $f; \
done;

then fix a bit of compilation problems: coccinelle for some reason
leaves several
f() {
    ...
    goto out;
    ...
    out:
}
patterns, with "out:" at function end.

then
./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"

(auto-msg was a file with this commit message)

Still, for backporting it may be more comfortable to use only the first
command and then do one huge commit.

Reported-by: Kevin Wolf <kwolf@redhat.com>
Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/replication.c | 40 ++++++++++++++++++----------------------
 replication.c       | 28 ++++++++++++----------------
 2 files changed, 30 insertions(+), 38 deletions(-)
diff mbox series

Patch

diff --git a/block/replication.c b/block/replication.c
index 936b2f8b5a..f5e7c5a8ec 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -83,9 +83,9 @@  static ReplicationOps replication_ops = {
 static int replication_open(BlockDriverState *bs, QDict *options,
                             int flags, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     int ret;
     BDRVReplicationState *s = bs->opaque;
-    Error *local_err = NULL;
     QemuOpts *opts = NULL;
     const char *mode;
     const char *top_id;
@@ -98,14 +98,14 @@  static int replication_open(BlockDriverState *bs, QDict *options,
 
     ret = -EINVAL;
     opts = qemu_opts_create(&replication_runtime_opts, NULL, 0, &error_abort);
-    qemu_opts_absorb_qdict(opts, options, &local_err);
-    if (local_err) {
+    qemu_opts_absorb_qdict(opts, options, errp);
+    if (*errp) {
         goto fail;
     }
 
     mode = qemu_opt_get(opts, REPLICATION_MODE);
     if (!mode) {
-        error_setg(&local_err, "Missing the option mode");
+        error_setg(errp, "Missing the option mode");
         goto fail;
     }
 
@@ -113,7 +113,8 @@  static int replication_open(BlockDriverState *bs, QDict *options,
         s->mode = REPLICATION_MODE_PRIMARY;
         top_id = qemu_opt_get(opts, REPLICATION_TOP_ID);
         if (top_id) {
-            error_setg(&local_err, "The primary side does not support option top-id");
+            error_setg(errp,
+                       "The primary side does not support option top-id");
             goto fail;
         }
     } else if (!strcmp(mode, "secondary")) {
@@ -121,11 +122,11 @@  static int replication_open(BlockDriverState *bs, QDict *options,
         top_id = qemu_opt_get(opts, REPLICATION_TOP_ID);
         s->top_id = g_strdup(top_id);
         if (!s->top_id) {
-            error_setg(&local_err, "Missing the option top-id");
+            error_setg(errp, "Missing the option top-id");
             goto fail;
         }
     } else {
-        error_setg(&local_err,
+        error_setg(errp,
                    "The option mode's value should be primary or secondary");
         goto fail;
     }
@@ -136,7 +137,6 @@  static int replication_open(BlockDriverState *bs, QDict *options,
 
 fail:
     qemu_opts_del(opts);
-    error_propagate(errp, local_err);
 
     return ret;
 }
@@ -314,7 +314,7 @@  static bool replication_recurse_is_first_non_filter(BlockDriverState *bs,
 
 static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
 {
-    Error *local_err = NULL;
+    ERRP_AUTO_PROPAGATE();
     int ret;
 
     if (!s->backup_job) {
@@ -322,9 +322,8 @@  static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
         return;
     }
 
-    backup_do_checkpoint(s->backup_job, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    backup_do_checkpoint(s->backup_job, errp);
+    if (*errp) {
         return;
     }
 
@@ -361,9 +360,9 @@  static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
 static void reopen_backing_file(BlockDriverState *bs, bool writable,
                                 Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BDRVReplicationState *s = bs->opaque;
     BlockReopenQueue *reopen_queue = NULL;
-    Error *local_err = NULL;
 
     if (writable) {
         s->orig_hidden_read_only = bdrv_is_read_only(s->hidden_disk->bs);
@@ -388,8 +387,7 @@  static void reopen_backing_file(BlockDriverState *bs, bool writable,
     }
 
     if (reopen_queue) {
-        bdrv_reopen_multiple(reopen_queue, &local_err);
-        error_propagate(errp, local_err);
+        bdrv_reopen_multiple(reopen_queue, errp);
     }
 
     bdrv_subtree_drained_end(s->hidden_disk->bs);
@@ -445,12 +443,12 @@  static bool check_top_bs(BlockDriverState *top_bs, BlockDriverState *bs)
 static void replication_start(ReplicationState *rs, ReplicationMode mode,
                               Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BlockDriverState *bs = rs->opaque;
     BDRVReplicationState *s;
     BlockDriverState *top_bs;
     int64_t active_length, hidden_length, disk_length;
     AioContext *aio_context;
-    Error *local_err = NULL;
 
     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
@@ -519,9 +517,8 @@  static void replication_start(ReplicationState *rs, ReplicationMode mode,
         }
 
         /* reopen the backing file in r/w mode */
-        reopen_backing_file(bs, true, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+        reopen_backing_file(bs, true, errp);
+        if (*errp) {
             aio_context_release(aio_context);
             return;
         }
@@ -546,9 +543,8 @@  static void replication_start(ReplicationState *rs, ReplicationMode mode,
                                 0, MIRROR_SYNC_MODE_NONE, NULL, 0, false,
                                 BLOCKDEV_ON_ERROR_REPORT,
                                 BLOCKDEV_ON_ERROR_REPORT, JOB_INTERNAL,
-                                backup_job_completed, bs, NULL, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
+                                backup_job_completed, bs, NULL, errp);
+        if (*errp) {
             backup_job_cleanup(bs);
             aio_context_release(aio_context);
             return;
diff --git a/replication.c b/replication.c
index be3a42f9c9..2e5ea7f537 100644
--- a/replication.c
+++ b/replication.c
@@ -44,15 +44,14 @@  void replication_remove(ReplicationState *rs)
  */
 void replication_start_all(ReplicationMode mode, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     ReplicationState *rs, *next;
-    Error *local_err = NULL;
 
     QLIST_FOREACH_SAFE(rs, &replication_states, node, next) {
         if (rs->ops && rs->ops->start) {
-            rs->ops->start(rs, mode, &local_err);
+            rs->ops->start(rs, mode, errp);
         }
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (*errp) {
             return;
         }
     }
@@ -60,15 +59,14 @@  void replication_start_all(ReplicationMode mode, Error **errp)
 
 void replication_do_checkpoint_all(Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     ReplicationState *rs, *next;
-    Error *local_err = NULL;
 
     QLIST_FOREACH_SAFE(rs, &replication_states, node, next) {
         if (rs->ops && rs->ops->checkpoint) {
-            rs->ops->checkpoint(rs, &local_err);
+            rs->ops->checkpoint(rs, errp);
         }
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (*errp) {
             return;
         }
     }
@@ -76,15 +74,14 @@  void replication_do_checkpoint_all(Error **errp)
 
 void replication_get_error_all(Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     ReplicationState *rs, *next;
-    Error *local_err = NULL;
 
     QLIST_FOREACH_SAFE(rs, &replication_states, node, next) {
         if (rs->ops && rs->ops->get_error) {
-            rs->ops->get_error(rs, &local_err);
+            rs->ops->get_error(rs, errp);
         }
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (*errp) {
             return;
         }
     }
@@ -92,15 +89,14 @@  void replication_get_error_all(Error **errp)
 
 void replication_stop_all(bool failover, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     ReplicationState *rs, *next;
-    Error *local_err = NULL;
 
     QLIST_FOREACH_SAFE(rs, &replication_states, node, next) {
         if (rs->ops && rs->ops->stop) {
-            rs->ops->stop(rs, failover, &local_err);
+            rs->ops->stop(rs, failover, errp);
         }
-        if (local_err) {
-            error_propagate(errp, local_err);
+        if (*errp) {
             return;
         }
     }