[PATCH 0/4] error: Mechanical fixes & cleanups

[PATCH 0/4] error: Mechanical fixes & cleanups

[Markus Armbruster]

Markus Armbruster (4):
  coccinelle/err-bad-newline: Fix for Python 3, and add patterns
  error: Strip trailing '\n' from error string arguments (again)
  error: Remove NULL checks on error_propagate() calls (again)
  error: Use error_fatal to simplify obvious fatal errors (again)

 scripts/coccinelle/err-bad-newline.cocci | 24 ++++++++++++++++++++++--
 exec.c                                   | 11 +++--------
 hw/i386/intel_iommu.c                    |  6 +++---
 hw/net/virtio-net.c                      |  8 ++------
 hw/s390x/s390-virtio-ccw.c               |  6 +-----
 hw/virtio/vhost.c                        | 10 +++-------
 migration/colo.c                         |  4 +---
 migration/migration.c                    |  8 ++------
 target/ppc/mmu-hash64.c                  |  2 +-
 9 files changed, 38 insertions(+), 41 deletions(-)

-- 
2.26.2

[PATCH 1/4] coccinelle/err-bad-newline: Fix for Python 3, and add patterns

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/coccinelle/err-bad-newline.cocci | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/scripts/coccinelle/err-bad-newline.cocci b/scripts/coccinelle/err-bad-newline.cocci
index 1316cc86a6..5394421873 100644
--- a/scripts/coccinelle/err-bad-newline.cocci
+++ b/scripts/coccinelle/err-bad-newline.cocci
@@ -1,22 +1,42 @@
 // Error messages should not contain newlines.  This script finds
 // messages that do.  Fixing them is manual.
 @r@
-expression errp, eno, cls, fmt;
+expression errp, err, eno, cls, fmt, ap;
 position p;
 @@
 (
+error_vreport(fmt, ap)@p
+|
+warn_vreport(fmt, ap)@p
+|
+info_vreport(fmt, ap)@p
+|
 error_report(fmt, ...)@p
 |
+warn_report(fmt, ...)@p
+|
+info_report(fmt, ...)@p
+|
+error_report_once(fmt, ...)@p
+|
+warn_report_once(fmt, ...)@p
+|
 error_setg(errp, fmt, ...)@p
 |
 error_setg_errno(errp, eno, fmt, ...)@p
 |
 error_setg_win32(errp, eno, cls, fmt, ...)@p
 |
+error_propagate_prepend(errp, err, fmt, ...)@p
+|
+error_vprepend(errp, fmt, ap)@p
+|
 error_prepend(errp, fmt, ...)@p
 |
 error_setg_file_open(errp, eno, cls, fmt, ...)@p
 |
+warn_reportf_err(errp, fmt, ...)@p
+|
 error_reportf_err(errp, fmt, ...)@p
 |
 error_set(errp, cls, fmt, ...)@p
@@ -26,4 +46,4 @@ fmt << r.fmt;
 p << r.p;
 @@
 if "\\n" in str(fmt):
-    print "%s:%s:%s:%s" % (p[0].file, p[0].line, p[0].column, fmt)
+    print("%s:%s:%s:%s" % (p[0].file, p[0].line, p[0].column, fmt))
-- 
2.26.2

[PATCH 2/4] error: Strip trailing '\n' from error string arguments (again)

[Markus Armbruster]

Tracked down with scripts/coccinelle/err-bad-newline.cocci.

Cc: Peter Xu <peterx@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/intel_iommu.c   | 6 +++---
 target/ppc/mmu-hash64.c | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index c56398e991..8cd9ed8d3b 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -2356,7 +2356,7 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
     if ((inv_desc->lo & VTD_INV_DESC_IOTLB_RSVD_LO) ||
         (inv_desc->hi & VTD_INV_DESC_IOTLB_RSVD_HI)) {
         error_report_once("%s: invalid iotlb inv desc: hi=0x%"PRIx64
-                          ", lo=0x%"PRIx64" (reserved bits unzero)\n",
+                          ", lo=0x%"PRIx64" (reserved bits unzero)",
                           __func__, inv_desc->hi, inv_desc->lo);
         return false;
     }
@@ -2377,7 +2377,7 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
         am = VTD_INV_DESC_IOTLB_AM(inv_desc->hi);
         if (am > VTD_MAMV) {
             error_report_once("%s: invalid iotlb inv desc: hi=0x%"PRIx64
-                              ", lo=0x%"PRIx64" (am=%u > VTD_MAMV=%u)\n",
+                              ", lo=0x%"PRIx64" (am=%u > VTD_MAMV=%u)",
                               __func__, inv_desc->hi, inv_desc->lo,
                               am, (unsigned)VTD_MAMV);
             return false;
@@ -2387,7 +2387,7 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
 
     default:
         error_report_once("%s: invalid iotlb inv desc: hi=0x%"PRIx64
-                          ", lo=0x%"PRIx64" (type mismatch: 0x%llx)\n",
+                          ", lo=0x%"PRIx64" (type mismatch: 0x%llx)",
                           __func__, inv_desc->hi, inv_desc->lo,
                           inv_desc->lo & VTD_INV_DESC_IOTLB_G);
         return false;
diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
index e5baabf0e1..c31d21e6a9 100644
--- a/target/ppc/mmu-hash64.c
+++ b/target/ppc/mmu-hash64.c
@@ -859,7 +859,7 @@ static int build_vrma_slbe(PowerPCCPU *cpu, ppc_slb_t *slb)
     }
 
     error_report("Bad page size encoding in LPCR[VRMASD]; LPCR=0x"
-                 TARGET_FMT_lx"\n", lpcr);
+                 TARGET_FMT_lx, lpcr);
 
     return -1;
 }
-- 
2.26.2

[PATCH 3/4] error: Remove NULL checks on error_propagate() calls (again)

[Markus Armbruster]

Patch created mechanically by rerunning:

    $ spatch --sp-file scripts/coccinelle/error_propagate_null.cocci \
             --macro-file scripts/cocci-macro-file.h \
             --use-gitgrep .

Cc: Jens Freimann <jfreimann@redhat.com>
Cc: Hailiang Zhang <zhang.zhanghailiang@huawei.com>
Cc: Juan Quintela <quintela@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/net/virtio-net.c   | 8 ++------
 migration/colo.c      | 4 +---
 migration/migration.c | 8 ++------
 3 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 4895af1cbe..21b9a315ed 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -809,9 +809,7 @@ static void failover_add_primary(VirtIONet *n, Error **errp)
             "sure primary device has parameter"
             " failover_pair_id=<virtio-net-id>\n");
 }
-    if (err) {
-        error_propagate(errp, err);
-    }
+    error_propagate(errp, err);
 }
 
 static int is_my_primary(void *opaque, QemuOpts *opts, Error **errp)
@@ -865,9 +863,7 @@ static DeviceState *virtio_connect_failover_devices(VirtIONet *n,
         n->primary_device_id = g_strdup(prim_dev->id);
         n->primary_device_opts = prim_dev->opts;
     } else {
-        if (err) {
-            error_propagate(errp, err);
-        }
+        error_propagate(errp, err);
     }
 
     return prim_dev;
diff --git a/migration/colo.c b/migration/colo.c
index ea7d1e9d4e..c6848c829b 100644
--- a/migration/colo.c
+++ b/migration/colo.c
@@ -798,9 +798,7 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
 
     colo_send_message(mis->to_src_file, COLO_MESSAGE_VMSTATE_LOADED,
                  &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-    }
+    error_propagate(errp, local_err);
 }
 
 static void colo_wait_handle_message(MigrationIncomingState *mis,
diff --git a/migration/migration.c b/migration/migration.c
index 2ed9923227..6a458b8fe5 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -601,9 +601,7 @@ void migration_fd_process_incoming(QEMUFile *f, Error **errp)
     }
 
     if (migration_incoming_setup(f, &local_err)) {
-        if (local_err) {
-            error_propagate(errp, local_err);
-        }
+        error_propagate(errp, local_err);
         return;
     }
     migration_incoming_process();
@@ -625,9 +623,7 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
         }
 
         if (migration_incoming_setup(f, &local_err)) {
-            if (local_err) {
-                error_propagate(errp, local_err);
-            }
+            error_propagate(errp, local_err);
             return;
         }
 
-- 
2.26.2

[PATCH 4/4] error: Use error_fatal to simplify obvious fatal errors (again)

[Markus Armbruster]

Patch created mechanically by rerunning:

    $ spatch --in-place --sp-file scripts/coccinelle/use-error_fatal.cocci \
	     --macro-file scripts/cocci-macro-file.h --use-gitgrep .

Variables now unused dropped manually.

Cc: Eric Auger <eric.auger@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 exec.c                     | 11 +++--------
 hw/s390x/s390-virtio-ccw.c |  6 +-----
 hw/virtio/vhost.c          | 10 +++-------
 3 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/exec.c b/exec.c
index 6f381f98e2..61b46a62f8 100644
--- a/exec.c
+++ b/exec.c
@@ -627,8 +627,7 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
      */
     MemoryRegion *mr = MEMORY_REGION(iommu_mr);
     TCGIOMMUNotifier *notifier;
-    Error *err = NULL;
-    int i, ret;
+    int i;
 
     for (i = 0; i < cpu->iommu_notifiers->len; i++) {
         notifier = g_array_index(cpu->iommu_notifiers, TCGIOMMUNotifier *, i);
@@ -657,12 +656,8 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
                             0,
                             HWADDR_MAX,
                             iommu_idx);
-        ret = memory_region_register_iommu_notifier(notifier->mr, &notifier->n,
-                                                    &err);
-        if (ret) {
-            error_report_err(err);
-            exit(1);
-        }
+        memory_region_register_iommu_notifier(notifier->mr, &notifier->n,
+                                              &error_fatal);
     }
 
     if (!notifier->active) {
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 8cc2f25d8a..4fc92559fb 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -170,11 +170,7 @@ static void s390_memory_init(MemoryRegion *ram)
      * Configure the maximum page size. As no memory devices were created
      * yet, this is the page size of initial memory only.
      */
-    s390_set_max_pagesize(qemu_maxrampagesize(), &local_err);
-    if (local_err) {
-        error_report_err(local_err);
-        exit(EXIT_FAILURE);
-    }
+    s390_set_max_pagesize(qemu_maxrampagesize(), &error_fatal);
     /* Initialize storage key device */
     s390_skeys_init();
     /* Initialize storage attributes device */
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 1a1384e7a6..cc8c1f89d5 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -712,9 +712,8 @@ static void vhost_iommu_region_add(MemoryListener *listener,
                                          iommu_listener);
     struct vhost_iommu *iommu;
     Int128 end;
-    int iommu_idx, ret;
+    int iommu_idx;
     IOMMUMemoryRegion *iommu_mr;
-    Error *err = NULL;
 
     if (!memory_region_is_iommu(section->mr)) {
         return;
@@ -737,11 +736,8 @@ static void vhost_iommu_region_add(MemoryListener *listener,
     iommu->iommu_offset = section->offset_within_address_space -
                           section->offset_within_region;
     iommu->hdev = dev;
-    ret = memory_region_register_iommu_notifier(section->mr, &iommu->n, &err);
-    if (ret) {
-        error_report_err(err);
-        exit(1);
-    }
+    memory_region_register_iommu_notifier(section->mr, &iommu->n,
+                                          &error_fatal);
     QLIST_INSERT_HEAD(&dev->iommu_list, iommu, iommu_next);
     /* TODO: can replay help performance here? */
 }
-- 
2.26.2

Re: [PATCH 4/4] error: Use error_fatal to simplify obvious fatal errors (again)

[David Hildenbrand]

On 22.07.20 10:40, Markus Armbruster wrote:
> Patch created mechanically by rerunning:
> 
>     $ spatch --in-place --sp-file scripts/coccinelle/use-error_fatal.cocci \
> 	     --macro-file scripts/cocci-macro-file.h --use-gitgrep .
> 
> Variables now unused dropped manually.
> 
> Cc: Eric Auger <eric.auger@redhat.com>
> Cc: David Hildenbrand <david@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  exec.c                     | 11 +++--------
>  hw/s390x/s390-virtio-ccw.c |  6 +-----
>  hw/virtio/vhost.c          | 10 +++-------
>  3 files changed, 7 insertions(+), 20 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 6f381f98e2..61b46a62f8 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -627,8 +627,7 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
>       */
>      MemoryRegion *mr = MEMORY_REGION(iommu_mr);
>      TCGIOMMUNotifier *notifier;
> -    Error *err = NULL;
> -    int i, ret;
> +    int i;
>  
>      for (i = 0; i < cpu->iommu_notifiers->len; i++) {
>          notifier = g_array_index(cpu->iommu_notifiers, TCGIOMMUNotifier *, i);
> @@ -657,12 +656,8 @@ static void tcg_register_iommu_notifier(CPUState *cpu,
>                              0,
>                              HWADDR_MAX,
>                              iommu_idx);
> -        ret = memory_region_register_iommu_notifier(notifier->mr, &notifier->n,
> -                                                    &err);
> -        if (ret) {
> -            error_report_err(err);
> -            exit(1);
> -        }
> +        memory_region_register_iommu_notifier(notifier->mr, &notifier->n,
> +                                              &error_fatal);
>      }
>  
>      if (!notifier->active) {
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 8cc2f25d8a..4fc92559fb 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -170,11 +170,7 @@ static void s390_memory_init(MemoryRegion *ram)
>       * Configure the maximum page size. As no memory devices were created
>       * yet, this is the page size of initial memory only.
>       */
> -    s390_set_max_pagesize(qemu_maxrampagesize(), &local_err);
> -    if (local_err) {
> -        error_report_err(local_err);
> -        exit(EXIT_FAILURE);
> -    }
> +    s390_set_max_pagesize(qemu_maxrampagesize(), &error_fatal);
>      /* Initialize storage key device */
>      s390_skeys_init();
>      /* Initialize storage attributes device */
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 1a1384e7a6..cc8c1f89d5 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -712,9 +712,8 @@ static void vhost_iommu_region_add(MemoryListener *listener,
>                                           iommu_listener);
>      struct vhost_iommu *iommu;
>      Int128 end;
> -    int iommu_idx, ret;
> +    int iommu_idx;
>      IOMMUMemoryRegion *iommu_mr;
> -    Error *err = NULL;
>  
>      if (!memory_region_is_iommu(section->mr)) {
>          return;
> @@ -737,11 +736,8 @@ static void vhost_iommu_region_add(MemoryListener *listener,
>      iommu->iommu_offset = section->offset_within_address_space -
>                            section->offset_within_region;
>      iommu->hdev = dev;
> -    ret = memory_region_register_iommu_notifier(section->mr, &iommu->n, &err);
> -    if (ret) {
> -        error_report_err(err);
> -        exit(1);
> -    }
> +    memory_region_register_iommu_notifier(section->mr, &iommu->n,
> +                                          &error_fatal);
>      QLIST_INSERT_HEAD(&dev->iommu_list, iommu, iommu_next);
>      /* TODO: can replay help performance here? */
>  }
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH 2/4] error: Strip trailing '\n' from error string arguments (again)

[Philippe Mathieu-Daudé]

On 7/22/20 10:40 AM, Markus Armbruster wrote:
> Tracked down with scripts/coccinelle/err-bad-newline.cocci.
> 
> Cc: Peter Xu <peterx@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/i386/intel_iommu.c   | 6 +++---
>  target/ppc/mmu-hash64.c | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 2/4] error: Strip trailing '\n' from error string arguments (again)

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2938 bytes --]

On Wed, Jul 22, 2020 at 10:40:46AM +0200, Markus Armbruster wrote:
> Tracked down with scripts/coccinelle/err-bad-newline.cocci.
> 
> Cc: Peter Xu <peterx@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

ppc part
Acked-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  hw/i386/intel_iommu.c   | 6 +++---
>  target/ppc/mmu-hash64.c | 2 +-
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index c56398e991..8cd9ed8d3b 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -2356,7 +2356,7 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
>      if ((inv_desc->lo & VTD_INV_DESC_IOTLB_RSVD_LO) ||
>          (inv_desc->hi & VTD_INV_DESC_IOTLB_RSVD_HI)) {
>          error_report_once("%s: invalid iotlb inv desc: hi=0x%"PRIx64
> -                          ", lo=0x%"PRIx64" (reserved bits unzero)\n",
> +                          ", lo=0x%"PRIx64" (reserved bits unzero)",
>                            __func__, inv_desc->hi, inv_desc->lo);
>          return false;
>      }
> @@ -2377,7 +2377,7 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
>          am = VTD_INV_DESC_IOTLB_AM(inv_desc->hi);
>          if (am > VTD_MAMV) {
>              error_report_once("%s: invalid iotlb inv desc: hi=0x%"PRIx64
> -                              ", lo=0x%"PRIx64" (am=%u > VTD_MAMV=%u)\n",
> +                              ", lo=0x%"PRIx64" (am=%u > VTD_MAMV=%u)",
>                                __func__, inv_desc->hi, inv_desc->lo,
>                                am, (unsigned)VTD_MAMV);
>              return false;
> @@ -2387,7 +2387,7 @@ static bool vtd_process_iotlb_desc(IntelIOMMUState *s, VTDInvDesc *inv_desc)
>  
>      default:
>          error_report_once("%s: invalid iotlb inv desc: hi=0x%"PRIx64
> -                          ", lo=0x%"PRIx64" (type mismatch: 0x%llx)\n",
> +                          ", lo=0x%"PRIx64" (type mismatch: 0x%llx)",
>                            __func__, inv_desc->hi, inv_desc->lo,
>                            inv_desc->lo & VTD_INV_DESC_IOTLB_G);
>          return false;
> diff --git a/target/ppc/mmu-hash64.c b/target/ppc/mmu-hash64.c
> index e5baabf0e1..c31d21e6a9 100644
> --- a/target/ppc/mmu-hash64.c
> +++ b/target/ppc/mmu-hash64.c
> @@ -859,7 +859,7 @@ static int build_vrma_slbe(PowerPCCPU *cpu, ppc_slb_t *slb)
>      }
>  
>      error_report("Bad page size encoding in LPCR[VRMASD]; LPCR=0x"
> -                 TARGET_FMT_lx"\n", lpcr);
> +                 TARGET_FMT_lx, lpcr);
>  
>      return -1;
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] error: Strip trailing '\n' from error string arguments (again)

[Peter Xu]

On Wed, Jul 22, 2020 at 10:40:46AM +0200, Markus Armbruster wrote:
> Tracked down with scripts/coccinelle/err-bad-newline.cocci.
> 
> Cc: Peter Xu <peterx@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [PATCH 1/4] coccinelle/err-bad-newline: Fix for Python 3, and add patterns

[Eric Blake]

On 7/22/20 3:40 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   scripts/coccinelle/err-bad-newline.cocci | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/4] error: Remove NULL checks on error_propagate() calls (again)

[Eric Blake]

On 7/22/20 3:40 AM, Markus Armbruster wrote:
> Patch created mechanically by rerunning:
> 
>      $ spatch --sp-file scripts/coccinelle/error_propagate_null.cocci \
>               --macro-file scripts/cocci-macro-file.h \
>               --use-gitgrep .
> 
> Cc: Jens Freimann <jfreimann@redhat.com>
> Cc: Hailiang Zhang <zhang.zhanghailiang@huawei.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

> +++ b/migration/colo.c
> @@ -798,9 +798,7 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>   
>       colo_send_message(mis->to_src_file, COLO_MESSAGE_VMSTATE_LOADED,
>                    &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> -    }
> +    error_propagate(errp, local_err);
>   }

As this is mechanical, it is fine. But there is now a further cleanup 
possible of passing errp directly to colo_send_message, and possibly 
dropping local_err altogether.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/4] error: Remove NULL checks on error_propagate() calls (again)

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 7/22/20 3:40 AM, Markus Armbruster wrote:
>> Patch created mechanically by rerunning:
>>
>>      $ spatch --sp-file scripts/coccinelle/error_propagate_null.cocci \
>>               --macro-file scripts/cocci-macro-file.h \
>>               --use-gitgrep .
>>
>> Cc: Jens Freimann <jfreimann@redhat.com>
>> Cc: Hailiang Zhang <zhang.zhanghailiang@huawei.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>
>> +++ b/migration/colo.c
>> @@ -798,9 +798,7 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>>         colo_send_message(mis->to_src_file,
>> COLO_MESSAGE_VMSTATE_LOADED,
>>                    &local_err);
>> -    if (local_err) {
>> -        error_propagate(errp, local_err);
>> -    }
>> +    error_propagate(errp, local_err);
>>   }
>
> As this is mechanical, it is fine. But there is now a further cleanup
> possible of passing errp directly to colo_send_message, and possibly
> dropping local_err altogether.

True.

The patch is small and simple enough for squashing in further manual
cleanups.  I'd like to first check whether a followup patch created with
the machinery I used for eliminating error_propagate() comes out better.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [PATCH 3/4] error: Remove NULL checks on error_propagate() calls (again)

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Eric Blake <eblake@redhat.com> writes:
>
>> On 7/22/20 3:40 AM, Markus Armbruster wrote:
>>> Patch created mechanically by rerunning:
>>>
>>>      $ spatch --sp-file scripts/coccinelle/error_propagate_null.cocci \
>>>               --macro-file scripts/cocci-macro-file.h \
>>>               --use-gitgrep .
>>>
>>> Cc: Jens Freimann <jfreimann@redhat.com>
>>> Cc: Hailiang Zhang <zhang.zhanghailiang@huawei.com>
>>> Cc: Juan Quintela <quintela@redhat.com>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>
>>> +++ b/migration/colo.c
>>> @@ -798,9 +798,7 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>>>         colo_send_message(mis->to_src_file,
>>> COLO_MESSAGE_VMSTATE_LOADED,
>>>                    &local_err);
>>> -    if (local_err) {
>>> -        error_propagate(errp, local_err);
>>> -    }
>>> +    error_propagate(errp, local_err);
>>>   }
>>
>> As this is mechanical, it is fine. But there is now a further cleanup
>> possible of passing errp directly to colo_send_message, and possibly
>> dropping local_err altogether.
>
> True.
>
> The patch is small and simple enough for squashing in further manual
> cleanups.  I'd like to first check whether a followup patch created with
> the machinery I used for eliminating error_propagate() comes out better.

Vladimir's scripts/coccinelle/errp-guard.cocci will take care of it.

Eliminating error propagation altogether would be even better, but it's
also more work: several void functions need to return bool instead.

>> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> Thanks!

Re: [PATCH 3/4] error: Remove NULL checks on error_propagate() calls (again)

[Eric Blake]

On 7/23/20 8:38 AM, Markus Armbruster wrote:

>>>> +++ b/migration/colo.c
>>>> @@ -798,9 +798,7 @@ static void colo_incoming_process_checkpoint(MigrationIncomingState *mis,
>>>>          colo_send_message(mis->to_src_file,
>>>> COLO_MESSAGE_VMSTATE_LOADED,
>>>>                     &local_err);
>>>> -    if (local_err) {
>>>> -        error_propagate(errp, local_err);
>>>> -    }
>>>> +    error_propagate(errp, local_err);
>>>>    }
>>>
>>> As this is mechanical, it is fine. But there is now a further cleanup
>>> possible of passing errp directly to colo_send_message, and possibly
>>> dropping local_err altogether.
>>
>> True.
>>
>> The patch is small and simple enough for squashing in further manual
>> cleanups.  I'd like to first check whether a followup patch created with
>> the machinery I used for eliminating error_propagate() comes out better.
> 
> Vladimir's scripts/coccinelle/errp-guard.cocci will take care of it.

Good to know.  Then I'm fine deferring those cleanups to the mechanical 
patches down the road, rather than a manual effort now.

> 
> Eliminating error propagation altogether would be even better, but it's
> also more work: several void functions need to return bool instead.

Correct, but also doesn't change the fact that this patch is ready to go 
regardless of how much further cleanup we plan on doing.

> 
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>
>> Thanks!

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 0/4] error: Mechanical fixes & cleanups

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster (4):
>   coccinelle/err-bad-newline: Fix for Python 3, and add patterns
>   error: Strip trailing '\n' from error string arguments (again)
>   error: Remove NULL checks on error_propagate() calls (again)
>   error: Use error_fatal to simplify obvious fatal errors (again)

PATCH 1+2 queued for 5.1, PATCH 3+4 queued for 5.2.