[PATCH v4 00/31] error: auto propagated local_err

[PATCH v4 00/31] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

Hi all!

Here is a proposal of auto propagation for local_err, to not call
error_propagate on every exit point, when we deal with local_err.

There are also two issues with errp:

1. error_fatal & error_append_hint/error_prepend: user can't see this
additional info, because exit() happens in error_setg earlier than info
is added. [Reported by Greg Kurz]

2. error_abort & error_propagate: when we wrap
error_abort by local_err+error_propagate, resulting coredump will
refer to error_propagate and not to the place where error happened.
(the macro itself don't fix the issue, but it allows to [3.] drop all
local_err+error_propagate pattern, which will definitely fix the issue)
[Reported by Kevin Wolf]

Still, applying new macro to all errp-functions is a huge task, which is
impossible to solve in one series.

So, here is a minimum: solve only [1.], by adding new macro to all
errp-functions which wants to call error_append_hint.

v4;
02: - check errp to be not NULL
    - drop Eric's r-b
03: add Eric's r-b
04: - rename macro to ERRP_AUTO_PROPAGATE [Kevin]
    - improve comment and commit msg, mention
      error_prepend
05: - handle error_prepend too
    - use new macro name
    - drop empty line at the end

commit message for auto-generated commits updated,
commits regenerated.

I'll use cc-cmd to cc appropriate recipients per patch, still
cover-letter together with 04-06 patches should be interesting for
all:

CC: kwolf@redhat.com
CC: mreitz@redhat.com
CC: jsnow@redhat.com
CC: fam@euphon.net
CC: sw@weilnetz.de
CC: codyprime@gmail.com
CC: marcandre.lureau@redhat.com
CC: pbonzini@redhat.com
CC: groug@kaod.org
CC: sundeep.lkml@gmail.com
CC: peter.maydell@linaro.org
CC: stefanha@redhat.com
CC: pburton@wavecomp.com
CC: arikalo@wavecomp.com
CC: berrange@redhat.com
CC: ehabkost@redhat.com
CC: david@gibson.dropbear.id.au
CC: clg@kaod.org
CC: mst@redhat.com
CC: marcel.apfelbaum@gmail.com
CC: mark.cave-ayland@ilande.co.uk
CC: yuval.shaia@oracle.com
CC: cohuck@redhat.com
CC: farman@linux.ibm.com
CC: rth@twiddle.net
CC: david@redhat.com
CC: pasic@linux.ibm.com
CC: borntraeger@de.ibm.com
CC: kraxel@redhat.com
CC: alex.williamson@redhat.com
CC: andrew@aj.id.au
CC: joel@jms.id.au
CC: eblake@redhat.com
CC: armbru@redhat.com
CC: mdroth@linux.vnet.ibm.com
CC: quintela@redhat.com
CC: dgilbert@redhat.com
CC: jasowang@redhat.com
CC: qemu-block@nongnu.org
CC: integration@gluster.org
CC: qemu-arm@nongnu.org
CC: qemu-ppc@nongnu.org
CC: qemu-s390x@nongnu.org


Vladimir Sementsov-Ogievskiy (31):
  errp: rename errp to errp_in where it is IN-argument
  hw/core/loader-fit: fix freeing errp in fit_load_fdt
  net/net: fix local variable shadowing in net_client_init
  error: auto propagated local_err
  scripts: add script to fix error_append_hint/error_prepend usage
  python: add commit-per-subsystem.py
  s390: Fix error_append_hint/error_prepend usage
  ARM TCG CPUs: Fix error_append_hint/error_prepend usage
  PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage
  arm: Fix error_append_hint/error_prepend usage
  SmartFusion2: Fix error_append_hint/error_prepend usage
  ASPEED BMCs: Fix error_append_hint/error_prepend usage
  Boston: Fix error_append_hint/error_prepend usage
  PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage
  PCI: Fix error_append_hint/error_prepend usage
  SCSI: Fix error_append_hint/error_prepend usage
  USB: Fix error_append_hint/error_prepend usage
  VFIO: Fix error_append_hint/error_prepend usage
  vhost: Fix error_append_hint/error_prepend usage
  virtio: Fix error_append_hint/error_prepend usage
  virtio-9p: Fix error_append_hint/error_prepend usage
  XIVE: Fix error_append_hint/error_prepend usage
  block: Fix error_append_hint/error_prepend usage
  chardev: Fix error_append_hint/error_prepend usage
  cmdline: Fix error_append_hint/error_prepend usage
  QOM: Fix error_append_hint/error_prepend usage
  Migration: Fix error_append_hint/error_prepend usage
  Sockets: Fix error_append_hint/error_prepend usage
  nbd: Fix error_append_hint/error_prepend usage
  PVRDMA: Fix error_append_hint/error_prepend usage
  ivshmem: Fix error_append_hint/error_prepend usage

 include/block/nbd.h                         |  1 +
 include/monitor/hmp.h                       |  2 +-
 include/qapi/error.h                        | 39 +++++++++++-
 ui/vnc.h                                    |  2 +-
 block.c                                     |  3 +
 block/backup.c                              |  1 +
 block/dirty-bitmap.c                        |  1 +
 block/file-posix.c                          |  4 ++
 block/gluster.c                             |  2 +
 block/qcow.c                                |  1 +
 block/qcow2-bitmap.c                        |  1 +
 block/qcow2.c                               |  3 +
 block/vdi.c                                 |  1 +
 block/vhdx-log.c                            |  1 +
 block/vmdk.c                                |  1 +
 block/vpc.c                                 |  1 +
 chardev/spice.c                             |  1 +
 hw/9pfs/9p-local.c                          |  1 +
 hw/9pfs/9p-proxy.c                          |  1 +
 hw/9pfs/9p.c                                |  1 +
 hw/arm/msf2-soc.c                           |  1 +
 hw/arm/virt.c                               |  2 +
 hw/block/dataplane/virtio-blk.c             |  1 +
 hw/core/loader-fit.c                        |  7 ++-
 hw/core/qdev-properties-system.c            |  1 +
 hw/intc/arm_gicv3_kvm.c                     |  1 +
 hw/intc/pnv_xive.c                          |  1 +
 hw/intc/xive.c                              |  3 +
 hw/misc/ivshmem.c                           |  1 +
 hw/misc/msf2-sysreg.c                       |  1 +
 hw/pci-bridge/pcie_root_port.c              |  1 +
 hw/ppc/mac_newworld.c                       |  1 +
 hw/ppc/pnv_lpc.c                            |  1 +
 hw/ppc/pnv_occ.c                            |  1 +
 hw/ppc/spapr.c                              |  1 +
 hw/ppc/spapr_irq.c                          |  1 +
 hw/ppc/spapr_pci.c                          |  1 +
 hw/rdma/vmw/pvrdma_main.c                   |  1 +
 hw/s390x/s390-ccw.c                         |  1 +
 hw/scsi/scsi-disk.c                         |  1 +
 hw/scsi/scsi-generic.c                      |  1 +
 hw/scsi/vhost-scsi.c                        |  1 +
 hw/usb/ccid-card-emulated.c                 |  1 +
 hw/vfio/common.c                            |  3 +
 hw/vfio/pci-quirks.c                        |  1 +
 hw/vfio/pci.c                               |  3 +
 hw/vfio/platform.c                          |  1 +
 hw/virtio/vhost-vsock.c                     |  1 +
 hw/virtio/virtio-pci.c                      |  2 +
 hw/watchdog/wdt_aspeed.c                    |  1 +
 migration/migration.c                       |  1 +
 migration/savevm.c                          |  2 +
 monitor/hmp-cmds.c                          |  8 +--
 nbd/client.c                                |  5 ++
 nbd/server.c                                |  4 ++
 net/net.c                                   | 17 +++--
 qdev-monitor.c                              |  2 +
 target/ppc/kvm.c                            |  2 +
 target/s390x/cpu_models.c                   |  2 +
 ui/vnc.c                                    | 10 +--
 util/error.c                                |  8 +--
 util/qemu-option.c                          |  2 +
 util/qemu-sockets.c                         |  2 +
 python/commit-per-subsystem.py              | 69 +++++++++++++++++++++
 scripts/coccinelle/fix-error-add-info.cocci | 28 +++++++++
 65 files changed, 247 insertions(+), 27 deletions(-)
 create mode 100755 python/commit-per-subsystem.py
 create mode 100644 scripts/coccinelle/fix-error-add-info.cocci

-- 
2.21.0

[PATCH v4 01/31] errp: rename errp to errp_in where it is IN-argument

[Vladimir Sementsov-Ogievskiy]

Error **errp is almost always OUT-argument: it's assumed to be NULL, or
pointer to NULL-initialized pointer, or pointer to error_abort or
error_fatal, for callee to report error.

But very few functions instead get Error **errp as IN-argument:
it's assumed to be set, and callee should clean it.
In such cases, rename errp to errp_in.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 include/monitor/hmp.h |  2 +-
 include/qapi/error.h  |  2 +-
 ui/vnc.h              |  2 +-
 monitor/hmp-cmds.c    |  8 ++++----
 ui/vnc.c              | 10 +++++-----
 util/error.c          |  8 ++++----
 6 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index a0e9511440..f929814f1a 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -16,7 +16,7 @@
 
 #include "qemu/readline.h"
 
-void hmp_handle_error(Monitor *mon, Error **errp);
+void hmp_handle_error(Monitor *mon, Error **errp_in);
 
 void hmp_info_name(Monitor *mon, const QDict *qdict);
 void hmp_info_version(Monitor *mon, const QDict *qdict);
diff --git a/include/qapi/error.h b/include/qapi/error.h
index 3f95141a01..9376f59c35 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -283,7 +283,7 @@ void error_free(Error *err);
 /*
  * Convenience function to assert that *@errp is set, then silently free it.
  */
-void error_free_or_abort(Error **errp);
+void error_free_or_abort(Error **errp_in);
 
 /*
  * Convenience function to warn_report() and free @err.
diff --git a/ui/vnc.h b/ui/vnc.h
index fea79c2fc9..00e0b48f2f 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -547,7 +547,7 @@ uint32_t read_u32(uint8_t *data, size_t offset);
 
 /* Protocol stage functions */
 void vnc_client_error(VncState *vs);
-size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp);
+size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in);
 
 void start_client_init(VncState *vs);
 void start_auth_vnc(VncState *vs);
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b2551c16d1..941d5d0a45 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -60,11 +60,11 @@
 #include <spice/enums.h>
 #endif
 
-void hmp_handle_error(Monitor *mon, Error **errp)
+void hmp_handle_error(Monitor *mon, Error **errp_in)
 {
-    assert(errp);
-    if (*errp) {
-        error_reportf_err(*errp, "Error: ");
+    assert(errp_in);
+    if (*errp_in) {
+        error_reportf_err(*errp_in, "Error: ");
     }
 }
 
diff --git a/ui/vnc.c b/ui/vnc.c
index 87b8045afe..9d6384d9b1 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1312,7 +1312,7 @@ void vnc_disconnect_finish(VncState *vs)
     g_free(vs);
 }
 
-size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
+size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in)
 {
     if (ret <= 0) {
         if (ret == 0) {
@@ -1320,14 +1320,14 @@ size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
             vnc_disconnect_start(vs);
         } else if (ret != QIO_CHANNEL_ERR_BLOCK) {
             trace_vnc_client_io_error(vs, vs->ioc,
-                                      errp ? error_get_pretty(*errp) :
+                                      errp_in ? error_get_pretty(*errp_in) :
                                       "Unknown");
             vnc_disconnect_start(vs);
         }
 
-        if (errp) {
-            error_free(*errp);
-            *errp = NULL;
+        if (errp_in) {
+            error_free(*errp_in);
+            *errp_in = NULL;
         }
         return 0;
     }
diff --git a/util/error.c b/util/error.c
index d4532ce318..b3ff3832d6 100644
--- a/util/error.c
+++ b/util/error.c
@@ -271,11 +271,11 @@ void error_free(Error *err)
     }
 }
 
-void error_free_or_abort(Error **errp)
+void error_free_or_abort(Error **errp_in)
 {
-    assert(errp && *errp);
-    error_free(*errp);
-    *errp = NULL;
+    assert(errp_in && *errp_in);
+    error_free(*errp_in);
+    *errp_in = NULL;
 }
 
 void error_propagate(Error **dst_errp, Error *local_err)
-- 
2.21.0

[PATCH v4 02/31] hw/core/loader-fit: fix freeing errp in fit_load_fdt

[Vladimir Sementsov-Ogievskiy]

fit_load_fdt forget to check that errp is not NULL and to zero it after
freeing. Fix it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/core/loader-fit.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index 953b16bc82..3ee9fb2f2e 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -200,7 +200,10 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
     err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
     if (err == -ENOENT) {
         load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
-        error_free(*errp);
+        if (errp) {
+            error_free(*errp);
+            *errp = NULL;
+        }
     } else if (err) {
         error_prepend(errp, "unable to read FDT load address from FIT: ");
         ret = err;
-- 
2.21.0

[PATCH v4 03/31] net/net: fix local variable shadowing in net_client_init

[Vladimir Sementsov-Ogievskiy]

Don't shadow Error *err: it's a bad thing.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 net/net.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/net/net.c b/net/net.c
index 84aa6d8d00..9e93c3f8a1 100644
--- a/net/net.c
+++ b/net/net.c
@@ -1126,16 +1126,13 @@ static int net_client_init(QemuOpts *opts, bool is_netdev, Error **errp)
 
             prefix_addr = substrings[0];
 
-            if (substrings[1]) {
-                /* User-specified prefix length.  */
-                int err;
-
-                err = qemu_strtoul(substrings[1], NULL, 10, &prefix_len);
-                if (err) {
-                    error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
-                               "ipv6-prefixlen", "a number");
-                    goto out;
-                }
+            /* Handle user-specified prefix length. */
+            if (substrings[1] &&
+                qemu_strtoul(substrings[1], NULL, 10, &prefix_len))
+            {
+                error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
+                           "ipv6-prefixlen", "a number");
+                goto out;
             }
 
             qemu_opt_set(opts, "ipv6-prefix", prefix_addr, &error_abort);
-- 
2.21.0

[PATCH v4 04/31] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
functions with errp OUT parameter.

It has three goals:

1. Fix issue with error_fatal & error_prepend/error_append_hint: user
can't see this additional information, because exit() happens in
error_setg earlier than information is added. [Reported by Greg Kurz]

2. Fix issue with error_abort & error_propagate: when we wrap
error_abort by local_err+error_propagate, resulting coredump will
refer to error_propagate and not to the place where error happened.
(the macro itself doesn't fix the issue, but it allows to [3.] drop all
local_err+error_propagate pattern, which will definitely fix the issue)
[Reported by Kevin Wolf]

3. Drop local_err+error_propagate pattern, which is used to workaround
void functions with errp parameter, when caller wants to know resulting
status. (Note: actually these functions could be merely updated to
return int error code).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

CC: kwolf@redhat.com
CC: mreitz@redhat.com
CC: jsnow@redhat.com
CC: fam@euphon.net
CC: sw@weilnetz.de
CC: codyprime@gmail.com
CC: marcandre.lureau@redhat.com
CC: pbonzini@redhat.com
CC: groug@kaod.org
CC: sundeep.lkml@gmail.com
CC: peter.maydell@linaro.org
CC: stefanha@redhat.com
CC: pburton@wavecomp.com
CC: arikalo@wavecomp.com
CC: berrange@redhat.com
CC: ehabkost@redhat.com
CC: david@gibson.dropbear.id.au
CC: clg@kaod.org
CC: mst@redhat.com
CC: marcel.apfelbaum@gmail.com
CC: mark.cave-ayland@ilande.co.uk
CC: yuval.shaia@oracle.com
CC: cohuck@redhat.com
CC: farman@linux.ibm.com
CC: rth@twiddle.net
CC: david@redhat.com
CC: pasic@linux.ibm.com
CC: borntraeger@de.ibm.com
CC: kraxel@redhat.com
CC: alex.williamson@redhat.com
CC: andrew@aj.id.au
CC: joel@jms.id.au
CC: eblake@redhat.com
CC: armbru@redhat.com
CC: mdroth@linux.vnet.ibm.com
CC: quintela@redhat.com
CC: dgilbert@redhat.com
CC: jasowang@redhat.com
CC: qemu-block@nongnu.org
CC: integration@gluster.org
CC: qemu-arm@nongnu.org
CC: qemu-ppc@nongnu.org
CC: qemu-s390x@nongnu.org

 include/qapi/error.h | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 9376f59c35..02f967ac1d 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -322,6 +322,43 @@ void error_set_internal(Error **errp,
                         ErrorClass err_class, const char *fmt, ...)
     GCC_FMT_ATTR(6, 7);
 
+typedef struct ErrorPropagator {
+    Error *local_err;
+    Error **errp;
+} ErrorPropagator;
+
+static inline void error_propagator_cleanup(ErrorPropagator *prop)
+{
+    error_propagate(prop->errp, prop->local_err);
+}
+
+G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
+
+/*
+ * ERRP_AUTO_PROPAGATE
+ *
+ * This macro is created to be the first line of a function with Error **errp
+ * OUT parameter. It's needed only in cases where we want to use error_prepend,
+ * error_append_hint or dereference *errp. It's still safe (but useless) in
+ * other cases.
+ *
+ * If errp is NULL or points to error_fatal, it is rewritten to point to a
+ * local Error object, which will be automatically propagated to the original
+ * errp on function exit (see error_propagator_cleanup).
+ *
+ * After invocation of this macro it is always safe to dereference errp
+ * (as it's not NULL anymore) and to append hints (by error_append_hint)
+ * (as, if it was error_fatal, we swapped it with a local_error to be
+ * propagated on cleanup).
+ *
+ * Note: we don't wrap the error_abort case, as we want resulting coredump
+ * to point to the place where the error happened, not to error_propagate.
+ */
+#define ERRP_AUTO_PROPAGATE() \
+g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
+errp = ((errp == NULL || *errp == error_fatal) ? \
+    &__auto_errp_prop.local_err : errp)
+
 /*
  * Special error destination to abort on error.
  * See error_setg() and error_propagate() for details.
-- 
2.21.0

[PATCH v4 05/31] scripts: add script to fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

error_append_hint and error_prepend will not work, if errp ==
&fatal_error, as program will exit before error_append_hint or
error_prepend call. Fix this by use of special macro
ERRP_AUTO_PROPAGATE.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

CC: kwolf@redhat.com
CC: mreitz@redhat.com
CC: jsnow@redhat.com
CC: fam@euphon.net
CC: sw@weilnetz.de
CC: codyprime@gmail.com
CC: marcandre.lureau@redhat.com
CC: pbonzini@redhat.com
CC: groug@kaod.org
CC: sundeep.lkml@gmail.com
CC: peter.maydell@linaro.org
CC: stefanha@redhat.com
CC: pburton@wavecomp.com
CC: arikalo@wavecomp.com
CC: berrange@redhat.com
CC: ehabkost@redhat.com
CC: david@gibson.dropbear.id.au
CC: clg@kaod.org
CC: mst@redhat.com
CC: marcel.apfelbaum@gmail.com
CC: mark.cave-ayland@ilande.co.uk
CC: yuval.shaia@oracle.com
CC: cohuck@redhat.com
CC: farman@linux.ibm.com
CC: rth@twiddle.net
CC: david@redhat.com
CC: pasic@linux.ibm.com
CC: borntraeger@de.ibm.com
CC: kraxel@redhat.com
CC: alex.williamson@redhat.com
CC: andrew@aj.id.au
CC: joel@jms.id.au
CC: eblake@redhat.com
CC: armbru@redhat.com
CC: mdroth@linux.vnet.ibm.com
CC: quintela@redhat.com
CC: dgilbert@redhat.com
CC: jasowang@redhat.com
CC: qemu-block@nongnu.org
CC: integration@gluster.org
CC: qemu-arm@nongnu.org
CC: qemu-ppc@nongnu.org
CC: qemu-s390x@nongnu.org

 scripts/coccinelle/fix-error-add-info.cocci | 28 +++++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 scripts/coccinelle/fix-error-add-info.cocci

diff --git a/scripts/coccinelle/fix-error-add-info.cocci b/scripts/coccinelle/fix-error-add-info.cocci
new file mode 100644
index 0000000000..34fa3be720
--- /dev/null
+++ b/scripts/coccinelle/fix-error-add-info.cocci
@@ -0,0 +1,28 @@
+@rule0@
+// Add invocation to errp-functions
+identifier fn;
+@@
+
+ fn(..., Error **errp, ...)
+ {
++   ERRP_AUTO_PROPAGATE();
+    <+...
+(
+    error_append_hint(errp, ...);
+|
+    error_prepend(errp, ...);
+)
+    ...+>
+ }
+
+@@
+// Drop doubled invocation
+identifier rule0.fn;
+@@
+
+ fn(...)
+{
+    ERRP_AUTO_PROPAGATE();
+-   ERRP_AUTO_PROPAGATE();
+    ...
+}
-- 
2.21.0

[PATCH v4 06/31] python: add commit-per-subsystem.py

[Vladimir Sementsov-Ogievskiy]

Add script to automatically commit tree-wide changes per-subsystem.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---

CC: kwolf@redhat.com
CC: mreitz@redhat.com
CC: jsnow@redhat.com
CC: fam@euphon.net
CC: sw@weilnetz.de
CC: codyprime@gmail.com
CC: marcandre.lureau@redhat.com
CC: pbonzini@redhat.com
CC: groug@kaod.org
CC: sundeep.lkml@gmail.com
CC: peter.maydell@linaro.org
CC: stefanha@redhat.com
CC: pburton@wavecomp.com
CC: arikalo@wavecomp.com
CC: berrange@redhat.com
CC: ehabkost@redhat.com
CC: david@gibson.dropbear.id.au
CC: clg@kaod.org
CC: mst@redhat.com
CC: marcel.apfelbaum@gmail.com
CC: mark.cave-ayland@ilande.co.uk
CC: yuval.shaia@oracle.com
CC: cohuck@redhat.com
CC: farman@linux.ibm.com
CC: rth@twiddle.net
CC: david@redhat.com
CC: pasic@linux.ibm.com
CC: borntraeger@de.ibm.com
CC: kraxel@redhat.com
CC: alex.williamson@redhat.com
CC: andrew@aj.id.au
CC: joel@jms.id.au
CC: eblake@redhat.com
CC: armbru@redhat.com
CC: mdroth@linux.vnet.ibm.com
CC: quintela@redhat.com
CC: dgilbert@redhat.com
CC: jasowang@redhat.com
CC: qemu-block@nongnu.org
CC: integration@gluster.org
CC: qemu-arm@nongnu.org
CC: qemu-ppc@nongnu.org
CC: qemu-s390x@nongnu.org


 python/commit-per-subsystem.py | 69 ++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100755 python/commit-per-subsystem.py

diff --git a/python/commit-per-subsystem.py b/python/commit-per-subsystem.py
new file mode 100755
index 0000000000..d8442d9ea3
--- /dev/null
+++ b/python/commit-per-subsystem.py
@@ -0,0 +1,69 @@
+#!/usr/bin/env python3
+#
+# Copyright (c) 2019 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import subprocess
+import sys
+
+
+def git_add(pattern):
+    subprocess.run(['git', 'add', pattern])
+
+
+def git_commit(msg):
+    subprocess.run(['git', 'commit', '-m', msg], capture_output=True)
+
+
+maintainers = sys.argv[1]
+message = sys.argv[2].strip()
+
+subsystem = None
+
+shortnames = {
+    'Block layer core': 'block',
+    'ARM cores': 'arm',
+    'Network Block Device (NBD)': 'nbd',
+    'Command line option argument parsing': 'cmdline',
+    'Character device backends': 'chardev',
+    'S390 general architecture support': 's390'
+}
+
+
+def commit():
+    if subsystem:
+        msg = subsystem
+        if msg in shortnames:
+            msg = shortnames[msg]
+        msg += ': ' + message
+        git_commit(msg)
+
+
+with open(maintainers) as f:
+    for line in f:
+        line = line.rstrip()
+        if not line:
+            continue
+        if len(line) >= 2 and line[1] == ':':
+            if line[0] == 'F' and line[3:] not in ['*', '*/']:
+                git_add(line[3:])
+        else:
+            # new subsystem start
+            commit()
+
+            subsystem = line
+
+commit()
-- 
2.21.0

[PATCH v4 07/31] s390: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/s390x/s390-ccw.c       | 1 +
 target/s390x/cpu_models.c | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 0c5a5b60bd..c0a648a7e0 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -55,6 +55,7 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
                                   char *sysfsdev,
                                   Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     unsigned int cssid, ssid, devid;
     char dev_path[PATH_MAX] = {0}, *tmp;
 
diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
index 009afc38b9..32f2e5e822 100644
--- a/target/s390x/cpu_models.c
+++ b/target/s390x/cpu_models.c
@@ -840,6 +840,7 @@ static void error_prepend_missing_feat(const char *name, void *opaque)
 static void check_compatibility(const S390CPUModel *max_model,
                                 const S390CPUModel *model, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     S390FeatBitmap missing;
 
     if (model->def->gen > max_model->def->gen) {
@@ -922,6 +923,7 @@ static inline void apply_cpu_model(const S390CPUModel *model, Error **errp)
 
 void s390_realize_cpu_model(CPUState *cs, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     S390CPUClass *xcc = S390_CPU_GET_CLASS(cs);
     S390CPU *cpu = S390_CPU(cs);
     const S390CPUModel *max_model;
-- 
2.21.0

[PATCH v4 08/31] ARM TCG CPUs: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/arm/msf2-soc.c | 1 +
 hw/arm/virt.c     | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
index 008fd9327a..b76f45657e 100644
--- a/hw/arm/msf2-soc.c
+++ b/hw/arm/msf2-soc.c
@@ -85,6 +85,7 @@ static void m2sxxx_soc_initfn(Object *obj)
 
 static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     MSF2State *s = MSF2_SOC(dev_soc);
     DeviceState *dev, *armv7m;
     SysBusDevice *busdev;
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d74538b021..ce2e57fd59 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1793,6 +1793,7 @@ static char *virt_get_gic_version(Object *obj, Error **errp)
 
 static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     VirtMachineState *vms = VIRT_MACHINE(obj);
 
     if (!strcmp(value, "3")) {
@@ -1825,6 +1826,7 @@ static char *virt_get_iommu(Object *obj, Error **errp)
 
 static void virt_set_iommu(Object *obj, const char *value, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     VirtMachineState *vms = VIRT_MACHINE(obj);
 
     if (!strcmp(value, "smmuv3")) {
-- 
2.21.0

[PATCH v4 09/31] PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/ppc/mac_newworld.c | 1 +
 hw/ppc/pnv_lpc.c      | 1 +
 hw/ppc/pnv_occ.c      | 1 +
 hw/ppc/spapr.c        | 1 +
 hw/ppc/spapr_irq.c    | 1 +
 hw/ppc/spapr_pci.c    | 1 +
 target/ppc/kvm.c      | 2 ++
 7 files changed, 8 insertions(+)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index c5bbcc7433..1a37412d31 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -609,6 +609,7 @@ static char *core99_get_via_config(Object *obj, Error **errp)
 
 static void core99_set_via_config(Object *obj, const char *value, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     Core99MachineState *cms = CORE99_MACHINE(obj);
 
     if (!strcmp(value, "cuda")) {
diff --git a/hw/ppc/pnv_lpc.c b/hw/ppc/pnv_lpc.c
index 9466d4a1be..5022afa2a8 100644
--- a/hw/ppc/pnv_lpc.c
+++ b/hw/ppc/pnv_lpc.c
@@ -681,6 +681,7 @@ static const TypeInfo pnv_lpc_power9_info = {
 
 static void pnv_lpc_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     PnvLpcController *lpc = PNV_LPC(dev);
     Object *obj;
     Error *local_err = NULL;
diff --git a/hw/ppc/pnv_occ.c b/hw/ppc/pnv_occ.c
index 8bead2c930..735e5655da 100644
--- a/hw/ppc/pnv_occ.c
+++ b/hw/ppc/pnv_occ.c
@@ -181,6 +181,7 @@ static const TypeInfo pnv_occ_power9_type_info = {
 
 static void pnv_occ_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     PnvOCC *occ = PNV_OCC(dev);
     PnvOCCClass *poc = PNV_OCC_GET_CLASS(occ);
     Object *obj;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 08a2a5a770..617afb722c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4330,6 +4330,7 @@ int spapr_get_vcpu_id(PowerPCCPU *cpu)
 
 void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     MachineState *ms = MACHINE(spapr);
     int vcpu_id;
diff --git a/hw/ppc/spapr_irq.c b/hw/ppc/spapr_irq.c
index 06fe2432ba..679f1306e4 100644
--- a/hw/ppc/spapr_irq.c
+++ b/hw/ppc/spapr_irq.c
@@ -549,6 +549,7 @@ static int spapr_irq_post_load_dual(SpaprMachineState *spapr, int version_id)
 
 static void spapr_irq_reset_dual(SpaprMachineState *spapr, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     Error *local_err = NULL;
 
     /*
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7b71ad7c74..012ecdd40a 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1817,6 +1817,7 @@ static void spapr_phb_destroy_msi(gpointer opaque)
 
 static void spapr_phb_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     /* We don't use SPAPR_MACHINE() in order to exit gracefully if the user
      * tries to add a sPAPR PHB to a non-pseries machine.
      */
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 8c5b1f25cc..6ea7502cb9 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -237,6 +237,7 @@ static int kvm_booke206_tlb_init(PowerPCCPU *cpu)
 #if defined(TARGET_PPC64)
 static void kvm_get_smmu_info(struct kvm_ppc_smmu_info *info, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     int ret;
 
     assert(kvm_state != NULL);
@@ -2073,6 +2074,7 @@ int kvmppc_set_smt_threads(int smt)
 
 void kvmppc_hint_smt_possible(Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     int i;
     GString *g;
     char *s;
-- 
2.21.0

[PATCH v4 10/31] arm: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/intc/arm_gicv3_kvm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 9c7f4ab871..e3b79d6179 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -766,6 +766,7 @@ static void vm_change_state_handler(void *opaque, int running,
 
 static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     GICv3State *s = KVM_ARM_GICV3(dev);
     KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
     bool multiple_redist_region_allowed;
-- 
2.21.0

[PATCH v4 11/31] SmartFusion2: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/misc/msf2-sysreg.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/msf2-sysreg.c b/hw/misc/msf2-sysreg.c
index ddc5a30c80..343351480d 100644
--- a/hw/misc/msf2-sysreg.c
+++ b/hw/misc/msf2-sysreg.c
@@ -127,6 +127,7 @@ static Property msf2_sysreg_properties[] = {
 
 static void msf2_sysreg_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     MSF2SysregState *s = MSF2_SYSREG(dev);
 
     if ((s->apb0div > 32 || !is_power_of_2(s->apb0div))
-- 
2.21.0

[PATCH v4 12/31] ASPEED BMCs: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/watchdog/wdt_aspeed.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index 9b93213417..a70d6dee63 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -243,6 +243,7 @@ static void aspeed_wdt_timer_expired(void *dev)
 
 static void aspeed_wdt_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
     AspeedWDTState *s = ASPEED_WDT(dev);
     Error *err = NULL;
-- 
2.21.0

[PATCH v4 13/31] Boston: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/core/loader-fit.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index 3ee9fb2f2e..84f35a1fe2 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -120,6 +120,7 @@ static int fit_load_kernel(const struct fit_loader *ldr, const void *itb,
                            int cfg, void *opaque, hwaddr *pend,
                            Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     const char *name;
     const void *data;
     const void *load_data;
@@ -178,6 +179,7 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
                         int cfg, void *opaque, const void *match_data,
                         hwaddr kernel_end, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     const char *name;
     const void *data;
     const void *load_data;
-- 
2.21.0

[PATCH v4 14/31] PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/intc/pnv_xive.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/intc/pnv_xive.c b/hw/intc/pnv_xive.c
index ed6e9d71bb..61ad7d46fe 100644
--- a/hw/intc/pnv_xive.c
+++ b/hw/intc/pnv_xive.c
@@ -1659,6 +1659,7 @@ static void pnv_xive_init(Object *obj)
 
 static void pnv_xive_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     PnvXive *xive = PNV_XIVE(dev);
     XiveSource *xsrc = &xive->ipi_source;
     XiveENDSource *end_xsrc = &xive->end_source;
-- 
2.21.0

[PATCH v4 15/31] PCI: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/pci-bridge/pcie_root_port.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 012c2cb12c..a1b4989534 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -60,6 +60,7 @@ static void rp_reset(DeviceState *qdev)
 
 static void rp_realize(PCIDevice *d, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     PCIEPort *p = PCIE_PORT(d);
     PCIESlot *s = PCIE_SLOT(d);
     PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(d);
-- 
2.21.0

[PATCH v4 16/31] SCSI: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/scsi/scsi-disk.c    | 1 +
 hw/scsi/scsi-generic.c | 1 +
 hw/scsi/vhost-scsi.c   | 1 +
 3 files changed, 3 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 915641a0f1..48165dc128 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2597,6 +2597,7 @@ static int get_device_type(SCSIDiskState *s)
 
 static void scsi_block_realize(SCSIDevice *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, dev);
     AioContext *ctx;
     int sg_version;
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index e7798ebcd0..4c7543801f 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -653,6 +653,7 @@ static void scsi_generic_reset(DeviceState *dev)
 
 static void scsi_generic_realize(SCSIDevice *s, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     int rc;
     int sg_version;
     struct sg_scsi_id scsiid;
diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index c693fc748a..c01c6dfbe2 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -165,6 +165,7 @@ static const VMStateDescription vmstate_virtio_vhost_scsi = {
 
 static void vhost_scsi_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
     VHostSCSICommon *vsc = VHOST_SCSI_COMMON(dev);
     Error *err = NULL;
-- 
2.21.0

[PATCH v4 17/31] USB: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/usb/ccid-card-emulated.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index 291e41db8a..958791e817 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -488,6 +488,7 @@ static uint32_t parse_enumeration(char *str,
 
 static void emulated_realize(CCIDCardState *base, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     EmulatedState *card = EMULATED_CCID_CARD(base);
     VCardEmulError ret;
     const EnumTable *ptable;
-- 
2.21.0

[PATCH v4 18/31] VFIO: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/vfio/common.c     | 3 +++
 hw/vfio/pci-quirks.c | 1 +
 hw/vfio/pci.c        | 3 +++
 hw/vfio/platform.c   | 1 +
 4 files changed, 8 insertions(+)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 3e03c495d8..b0f3f64f5d 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -136,6 +136,7 @@ static const char *index_to_str(VFIODevice *vbasedev, int index)
 int vfio_set_irq_signaling(VFIODevice *vbasedev, int index, int subindex,
                            int action, int fd, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     struct vfio_irq_set *irq_set;
     int argsz, ret = 0;
     const char *name;
@@ -1437,6 +1438,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
 
 VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     VFIOGroup *group;
     char path[32];
     struct vfio_group_status status = { .argsz = sizeof(status) };
@@ -1526,6 +1528,7 @@ void vfio_put_group(VFIOGroup *group)
 int vfio_get_device(VFIOGroup *group, const char *name,
                     VFIODevice *vbasedev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) };
     int ret, fd;
 
diff --git a/hw/vfio/pci-quirks.c b/hw/vfio/pci-quirks.c
index 136f3a9ad6..d8b6ef7021 100644
--- a/hw/vfio/pci-quirks.c
+++ b/hw/vfio/pci-quirks.c
@@ -2139,6 +2139,7 @@ const PropertyInfo qdev_prop_nv_gpudirect_clique = {
 
 static int vfio_add_nv_gpudirect_cap(VFIOPCIDevice *vdev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     PCIDevice *pdev = &vdev->pdev;
     int ret, pos = 0xC8;
 
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c5e6fe61cb..4b537bce2c 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1916,6 +1916,7 @@ static void vfio_check_af_flr(VFIOPCIDevice *vdev, uint8_t pos)
 
 static int vfio_add_std_cap(VFIOPCIDevice *vdev, uint8_t pos, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     PCIDevice *pdev = &vdev->pdev;
     uint8_t cap_id, next, size;
     int ret;
@@ -2469,6 +2470,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
 
 static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     VFIODevice *vbasedev = &vdev->vbasedev;
     struct vfio_region_info *reg_info;
     struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
@@ -2700,6 +2702,7 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
 
 static void vfio_realize(PCIDevice *pdev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     VFIOPCIDevice *vdev = PCI_VFIO(pdev);
     VFIODevice *vbasedev_iter;
     VFIOGroup *group;
diff --git a/hw/vfio/platform.c b/hw/vfio/platform.c
index d7598c6152..236e5f8f57 100644
--- a/hw/vfio/platform.c
+++ b/hw/vfio/platform.c
@@ -617,6 +617,7 @@ static int vfio_base_device_init(VFIODevice *vbasedev, Error **errp)
  */
 static void vfio_platform_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     VFIOPlatformDevice *vdev = VFIO_PLATFORM_DEVICE(dev);
     SysBusDevice *sbdev = SYS_BUS_DEVICE(dev);
     VFIODevice *vbasedev = &vdev->vbasedev;
-- 
2.21.0

[PATCH v4 19/31] vhost: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/virtio/vhost-vsock.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/vhost-vsock.c b/hw/virtio/vhost-vsock.c
index f5744363a8..61ade0fa65 100644
--- a/hw/virtio/vhost-vsock.c
+++ b/hw/virtio/vhost-vsock.c
@@ -300,6 +300,7 @@ static const VMStateDescription vmstate_virtio_vhost_vsock = {
 
 static void vhost_vsock_device_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VHostVSock *vsock = VHOST_VSOCK(dev);
     int vhostfd;
-- 
2.21.0

[PATCH v4 20/31] virtio: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/block/dataplane/virtio-blk.c | 1 +
 hw/virtio/virtio-pci.c          | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 119906a5fe..f8a1e70886 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -85,6 +85,7 @@ bool virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
                                   VirtIOBlockDataPlane **dataplane,
                                   Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     VirtIOBlockDataPlane *s;
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index c6b47a9c73..a36e5f6990 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1525,6 +1525,7 @@ static void virtio_pci_pre_plugged(DeviceState *d, Error **errp)
 /* This is called by virtio-bus just after the device is plugged. */
 static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     VirtIOPCIProxy *proxy = VIRTIO_PCI(d);
     VirtioBusState *bus = &proxy->bus;
     bool legacy = virtio_pci_legacy(proxy);
@@ -1684,6 +1685,7 @@ static void virtio_pci_device_unplugged(DeviceState *d)
 
 static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     VirtIOPCIProxy *proxy = VIRTIO_PCI(pci_dev);
     VirtioPCIClass *k = VIRTIO_PCI_GET_CLASS(pci_dev);
     bool pcie_port = pci_bus_is_express(pci_get_bus(pci_dev)) &&
-- 
2.21.0

[PATCH v4 21/31] virtio-9p: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/9pfs/9p-local.c | 1 +
 hw/9pfs/9p-proxy.c | 1 +
 hw/9pfs/9p.c       | 1 +
 3 files changed, 3 insertions(+)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 08e673a79c..fccbf758bd 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1471,6 +1471,7 @@ static void local_cleanup(FsContext *ctx)
 
 static void error_append_security_model_hint(Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     error_append_hint(errp, "Valid options are: security_model="
                       "[passthrough|mapped-xattr|mapped-file|none]\n");
 }
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index 57a8c1c808..9291c8efa2 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -1116,6 +1116,7 @@ static int connect_namedsocket(const char *path, Error **errp)
 
 static void error_append_socket_sockfd_hint(Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     error_append_hint(errp, "Either specify socket=/some/path where /some/path"
                       " points to a listening AF_UNIX socket or sock_fd=fd"
                       " where fd is a file descriptor to a connected AF_UNIX"
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
index cce2366219..1df2749e03 100644
--- a/hw/9pfs/9p.c
+++ b/hw/9pfs/9p.c
@@ -3552,6 +3552,7 @@ void pdu_submit(V9fsPDU *pdu, P9MsgHeader *hdr)
 int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
                                Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     int i, len;
     struct stat stat;
     FsDriverEntry *fse;
-- 
2.21.0

[PATCH v4 22/31] XIVE: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/intc/xive.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/intc/xive.c b/hw/intc/xive.c
index b7417210d8..926cb6741d 100644
--- a/hw/intc/xive.c
+++ b/hw/intc/xive.c
@@ -570,6 +570,7 @@ static void xive_tctx_reset(void *dev)
 
 static void xive_tctx_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XiveTCTX *tctx = XIVE_TCTX(dev);
     PowerPCCPU *cpu;
     CPUPPCState *env;
@@ -1050,6 +1051,7 @@ static void xive_source_reset(void *dev)
 
 static void xive_source_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XiveSource *xsrc = XIVE_SOURCE(dev);
     Object *obj;
     Error *local_err = NULL;
@@ -1798,6 +1800,7 @@ static const MemoryRegionOps xive_end_source_ops = {
 
 static void xive_end_source_realize(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     XiveENDSource *xsrc = XIVE_END_SOURCE(dev);
     Object *obj;
     Error *local_err = NULL;
-- 
2.21.0

[PATCH v4 23/31] block: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/block/nbd.h  | 1 +
 block.c              | 3 +++
 block/backup.c       | 1 +
 block/dirty-bitmap.c | 1 +
 block/file-posix.c   | 4 ++++
 block/gluster.c      | 2 ++
 block/qcow.c         | 1 +
 block/qcow2-bitmap.c | 1 +
 block/qcow2.c        | 3 +++
 block/vdi.c          | 1 +
 block/vhdx-log.c     | 1 +
 block/vmdk.c         | 1 +
 block/vpc.c          | 1 +
 13 files changed, 21 insertions(+)

diff --git a/include/block/nbd.h b/include/block/nbd.h
index 316fd705a9..330f40142a 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -360,6 +360,7 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds,
 static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
                            const char *desc, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     int ret = qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
 
     if (ret < 0) {
diff --git a/block.c b/block.c
index 5944124845..5dfcfc9b90 100644
--- a/block.c
+++ b/block.c
@@ -1565,6 +1565,7 @@ fail_opts:
 
 static QDict *parse_json_filename(const char *filename, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     QObject *options_obj;
     QDict *options;
     int ret;
@@ -2592,6 +2593,7 @@ out:
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
                            const char *bdref_key, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     char *backing_filename = NULL;
     char *bdref_key_dot;
     const char *reference = NULL;
@@ -2823,6 +2825,7 @@ static BlockDriverState *bdrv_append_temp_snapshot(BlockDriverState *bs,
                                                    QDict *snapshot_options,
                                                    Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     /* TODO: extra byte is a hack to ensure MAX_PATH space on Windows. */
     char *tmp_filename = g_malloc0(PATH_MAX + 1);
     int64_t total_size;
diff --git a/block/backup.c b/block/backup.c
index 763f0d7ff6..58488a21fb 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -583,6 +583,7 @@ static const BlockJobDriver backup_job_driver = {
 static int64_t backup_calculate_cluster_size(BlockDriverState *target,
                                              Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     int ret;
     BlockDriverInfo bdi;
 
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 134e0c9a0c..099ed74dfa 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -237,6 +237,7 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
 int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
                             Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     if ((flags & BDRV_BITMAP_BUSY) && bdrv_dirty_bitmap_busy(bitmap)) {
         error_setg(errp, "Bitmap '%s' is currently in use by another"
                    " operation and cannot be used", bitmap->name);
diff --git a/block/file-posix.c b/block/file-posix.c
index f12c06de2d..db7a5b7e1a 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -320,6 +320,7 @@ static bool raw_is_io_aligned(int fd, void *buf, size_t len)
 
 static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BDRVRawState *s = bs->opaque;
     char *buf;
     size_t max_align = MAX(MAX_BLOCKSIZE, getpagesize());
@@ -473,6 +474,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
                            int bdrv_flags, int open_flags,
                            bool device, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BDRVRawState *s = bs->opaque;
     QemuOpts *opts;
     Error *local_err = NULL;
@@ -817,6 +819,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
                                 uint64_t new_perm, uint64_t new_shared,
                                 Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BDRVRawState *s = bs->opaque;
     int ret = 0;
     Error *local_err = NULL;
@@ -2232,6 +2235,7 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs)
 static int coroutine_fn
 raw_co_create(BlockdevCreateOptions *options, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BlockdevCreateOptionsFile *file_opts;
     Error *local_err = NULL;
     int fd;
diff --git a/block/gluster.c b/block/gluster.c
index 64028b2cba..a69e89b42a 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -419,6 +419,7 @@ out:
 static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
                                            Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     struct glfs *glfs;
     int ret;
     int old_errno;
@@ -694,6 +695,7 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf,
                               const char *filename,
                               QDict *options, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     int ret;
     if (filename) {
         ret = qemu_gluster_parse_uri(gconf, filename);
diff --git a/block/qcow.c b/block/qcow.c
index 5bdf72ba33..0caf94c8f8 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -117,6 +117,7 @@ static QemuOptsList qcow_runtime_opts = {
 static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
                      Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BDRVQcowState *s = bs->opaque;
     unsigned int len, i, shift;
     int ret;
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index b2487101ed..90f2f010f2 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1618,6 +1618,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
                                       uint32_t granularity,
                                       Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BDRVQcow2State *s = bs->opaque;
     bool found;
     Qcow2BitmapList *bm_list;
diff --git a/block/qcow2.c b/block/qcow2.c
index 4d16393e61..a8f0bf3cae 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1207,6 +1207,7 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
 static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
                                       int flags, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BDRVQcow2State *s = bs->opaque;
     unsigned int len, i;
     int ret = 0;
@@ -3036,6 +3037,7 @@ static uint64_t qcow2_opt_get_refcount_bits_del(QemuOpts *opts, int version,
 static int coroutine_fn
 qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BlockdevCreateOptionsQcow2 *qcow2_opts;
     QDict *options;
 
@@ -3740,6 +3742,7 @@ fail:
 static int coroutine_fn qcow2_co_truncate(BlockDriverState *bs, int64_t offset,
                                           PreallocMode prealloc, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BDRVQcow2State *s = bs->opaque;
     uint64_t old_length;
     int64_t new_l1_size;
diff --git a/block/vdi.c b/block/vdi.c
index 806ba7f53c..5259cce24b 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -735,6 +735,7 @@ nonallocating_write:
 static int coroutine_fn vdi_co_do_create(BlockdevCreateOptions *create_options,
                                          size_t block_size, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BlockdevCreateOptionsVdi *vdi_opts;
     int ret = 0;
     uint64_t bytes = 0;
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index fdd3a7adc3..176e03327b 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -748,6 +748,7 @@ exit:
 int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
                    Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     int ret = 0;
     VHDXHeader *hdr;
     VHDXLogSequence logs = { 0 };
diff --git a/block/vmdk.c b/block/vmdk.c
index fed3b50c8a..6b10830d94 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -1078,6 +1078,7 @@ static const char *next_line(const char *s)
 static int vmdk_parse_extents(const char *desc, BlockDriverState *bs,
                               QDict *options, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     int ret;
     int matches;
     char access[11];
diff --git a/block/vpc.c b/block/vpc.c
index 5cd3890780..90d0cae862 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -971,6 +971,7 @@ static int calculate_rounded_image_size(BlockdevCreateOptionsVpc *vpc_opts,
 static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
                                       Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BlockdevCreateOptionsVpc *vpc_opts;
     BlockBackend *blk = NULL;
     BlockDriverState *bs = NULL;
-- 
2.21.0

[PATCH v4 24/31] chardev: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 chardev/spice.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/chardev/spice.c b/chardev/spice.c
index 241e2b7770..ce2145fb19 100644
--- a/chardev/spice.c
+++ b/chardev/spice.c
@@ -267,6 +267,7 @@ static void qemu_chr_open_spice_vmc(Chardev *chr,
                                     bool *be_opened,
                                     Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     ChardevSpiceChannel *spicevmc = backend->u.spicevmc.data;
     const char *type = spicevmc->type;
     const char **psubtype = spice_server_char_device_recognized_subtypes();
-- 
2.21.0

[PATCH v4 25/31] cmdline: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 util/qemu-option.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 97172b5eaa..031c01eddc 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -145,6 +145,7 @@ static const QemuOptDesc *find_desc_by_name(const QemuOptDesc *desc,
 void parse_option_size(const char *name, const char *value,
                        uint64_t *ret, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     uint64_t size;
     int err;
 
@@ -660,6 +661,7 @@ QemuOpts *qemu_opts_find(QemuOptsList *list, const char *id)
 QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
                            int fail_if_exists, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     QemuOpts *opts = NULL;
 
     if (id) {
-- 
2.21.0

[PATCH v4 26/31] QOM: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/core/qdev-properties-system.c | 1 +
 qdev-monitor.c                   | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/hw/core/qdev-properties-system.c b/hw/core/qdev-properties-system.c
index 70bfd4809b..497a568cc2 100644
--- a/hw/core/qdev-properties-system.c
+++ b/hw/core/qdev-properties-system.c
@@ -221,6 +221,7 @@ static void get_chr(Object *obj, Visitor *v, const char *name, void *opaque,
 static void set_chr(Object *obj, Visitor *v, const char *name, void *opaque,
                     Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     DeviceState *dev = DEVICE(obj);
     Error *local_err = NULL;
     Property *prop = opaque;
diff --git a/qdev-monitor.c b/qdev-monitor.c
index 148df9cacf..89c99d9753 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -328,6 +328,7 @@ static Object *qdev_get_peripheral_anon(void)
 
 static void qbus_list_bus(DeviceState *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BusState *child;
     const char *sep = " ";
 
@@ -342,6 +343,7 @@ static void qbus_list_bus(DeviceState *dev, Error **errp)
 
 static void qbus_list_dev(BusState *bus, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BusChild *kid;
     const char *sep = " ";
 
-- 
2.21.0

[PATCH v4 27/31] Migration: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 migration/migration.c | 1 +
 migration/savevm.c    | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index 5f7e4d15e9..deaa789533 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -971,6 +971,7 @@ static bool migrate_caps_check(bool *cap_list,
                                MigrationCapabilityStatusList *params,
                                Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     MigrationCapabilityStatusList *cap;
     bool old_postcopy_cap;
     MigrationIncomingState *mis = migration_incoming_get_current();
diff --git a/migration/savevm.c b/migration/savevm.c
index bb9462a54d..f9293fe192 100644
--- a/migration/savevm.c
+++ b/migration/savevm.c
@@ -2586,6 +2586,7 @@ int qemu_load_device_state(QEMUFile *f)
 
 int save_snapshot(const char *name, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BlockDriverState *bs, *bs1;
     QEMUSnapshotInfo sn1, *sn = &sn1, old_sn1, *old_sn = &old_sn1;
     int ret = -1;
@@ -2790,6 +2791,7 @@ void qmp_xen_load_devices_state(const char *filename, Error **errp)
 
 int load_snapshot(const char *name, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     BlockDriverState *bs, *bs_vm_state;
     QEMUSnapshotInfo sn;
     QEMUFile *f;
-- 
2.21.0

[PATCH v4 28/31] Sockets: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 util/qemu-sockets.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index bcc06d0e01..396e028ae0 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -861,6 +861,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
                              int num,
                              Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     struct sockaddr_un un;
     int sock, fd;
     char *pathbuf = NULL;
@@ -936,6 +937,7 @@ err:
 
 static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     struct sockaddr_un un;
     int sock, rc;
     size_t pathlen;
-- 
2.21.0

[PATCH v4 29/31] nbd: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/client.c | 5 +++++
 nbd/server.c | 4 ++++
 2 files changed, 9 insertions(+)

diff --git a/nbd/client.c b/nbd/client.c
index f6733962b4..6e510f4a14 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -68,6 +68,7 @@ static int nbd_send_option_request(QIOChannel *ioc, uint32_t opt,
                                    uint32_t len, const char *data,
                                    Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     NBDOption req;
     QEMU_BUILD_BUG_ON(sizeof(req) != 16);
 
@@ -153,6 +154,7 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt,
 static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
                                 bool strict, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     g_autofree char *msg = NULL;
 
     if (!(reply->type & (1 << 31))) {
@@ -331,6 +333,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
 static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
                               NBDExportInfo *info, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     NBDOptionReply reply;
     uint32_t len = strlen(info->name);
     uint16_t type;
@@ -870,6 +873,7 @@ static int nbd_start_negotiate(AioContext *aio_context, QIOChannel *ioc,
                                bool structured_reply, bool *zeroes,
                                Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     uint64_t magic;
 
     trace_nbd_start_negotiate(tlscreds, hostname ? hostname : "<null>");
@@ -1005,6 +1009,7 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc,
                           const char *hostname, QIOChannel **outioc,
                           NBDExportInfo *info, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     int result;
     bool zeroes;
     bool base_allocation = info->base_allocation;
diff --git a/nbd/server.c b/nbd/server.c
index d8d1e62455..e7eb154f53 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -365,6 +365,7 @@ static int nbd_opt_read_name(NBDClient *client, char *name, uint32_t *length,
 static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp,
                                        Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     size_t name_len, desc_len;
     uint32_t len;
     const char *name = exp->name ? exp->name : "";
@@ -427,6 +428,7 @@ static void nbd_check_meta_export(NBDClient *client)
 static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes,
                                             Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     char name[NBD_MAX_NAME_SIZE + 1];
     char buf[NBD_REPLY_EXPORT_NAME_SIZE] = "";
     size_t len;
@@ -1260,6 +1262,7 @@ static int nbd_negotiate_options(NBDClient *client, Error **errp)
  */
 static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     char buf[NBD_OLDSTYLE_NEGOTIATE_SIZE] = "";
     int ret;
 
@@ -1631,6 +1634,7 @@ void nbd_export_close(NBDExport *exp)
 
 void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     if (mode == NBD_SERVER_REMOVE_MODE_HARD || QTAILQ_EMPTY(&exp->clients)) {
         nbd_export_close(exp);
         return;
-- 
2.21.0

[PATCH v4 30/31] PVRDMA: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/rdma/vmw/pvrdma_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 3e36e13013..fd1a6ef099 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -592,6 +592,7 @@ static void pvrdma_shutdown_notifier(Notifier *n, void *opaque)
 
 static void pvrdma_realize(PCIDevice *pdev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     int rc = 0;
     PVRDMADev *dev = PVRDMA_DEV(pdev);
     Object *memdev_root;
-- 
2.21.0

[PATCH v4 31/31] ivshmem: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

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.

This commit (together with its neighbors) was generated by

git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
--in-place $f; done

and then

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

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

and then by hand, for not maintained changed files:

git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>

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

Reported-by: Greg Kurz <groug@kaod.org>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/misc/ivshmem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 5e3b05eae0..b6dcc27d18 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -864,6 +864,7 @@ static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
 
 static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
 {
+    ERRP_AUTO_PROPAGATE();
     IVShmemState *s = IVSHMEM_COMMON(dev);
     Error *err = NULL;
     uint8_t *pci_conf;
-- 
2.21.0

Re: [PATCH v4 02/31] hw/core/loader-fit: fix freeing errp in fit_load_fdt

[Eric Blake]

On 10/1/19 10:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> fit_load_fdt forget to check that errp is not NULL and to zero it after
> freeing. Fix it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   hw/core/loader-fit.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)

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

> 
> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
> index 953b16bc82..3ee9fb2f2e 100644
> --- a/hw/core/loader-fit.c
> +++ b/hw/core/loader-fit.c
> @@ -200,7 +200,10 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>       err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
>       if (err == -ENOENT) {
>           load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
> -        error_free(*errp);
> +        if (errp) {
> +            error_free(*errp);
> +            *errp = NULL;
> +        }
>       } else if (err) {
>           error_prepend(errp, "unable to read FDT load address from FIT: ");
>           ret = err;
> 

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

Re: [PATCH v4 04/31] error: auto propagated local_err

[Eric Blake]

On 10/1/19 10:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with errp OUT parameter.
> 
> It has three goals:
> 
> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
> 
> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
> local_err+error_propagate pattern, which will definitely fix the issue)
> [Reported by Kevin Wolf]
> 
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

I'm still hoping Markus gets some review time for this series with a 
chance to speak up (and that we haven't been causing you a lot of work 
which would all have to change if Markus has a different viewpoint).

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

> +/*
> + * ERRP_AUTO_PROPAGATE
> + *
> + * This macro is created to be the first line of a function with Error **errp
> + * OUT parameter. It's needed only in cases where we want to use error_prepend,
> + * error_append_hint or dereference *errp. It's still safe (but useless) in
> + * other cases.
> + *
> + * If errp is NULL or points to error_fatal, it is rewritten to point to a
> + * local Error object, which will be automatically propagated to the original
> + * errp on function exit (see error_propagator_cleanup).
> + *
> + * After invocation of this macro it is always safe to dereference errp
> + * (as it's not NULL anymore) and to append hints (by error_append_hint)

Maybe: 'to append information (by error_prepend or error_append_hint)'

> + * (as, if it was error_fatal, we swapped it with a local_error to be
> + * propagated on cleanup).
> + *
> + * Note: we don't wrap the error_abort case, as we want resulting coredump
> + * to point to the place where the error happened, not to error_propagate.
> + */
> +#define ERRP_AUTO_PROPAGATE() \
> +g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
> +errp = ((errp == NULL || *errp == error_fatal) ? \
> +    &__auto_errp_prop.local_err : errp)
> +
>   /*
>    * Special error destination to abort on error.
>    * See error_setg() and error_propagate() for details.
> 

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

Re: [PATCH v4 05/31] scripts: add script to fix error_append_hint/error_prepend usage

[Eric Blake]

On 10/1/19 10:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> error_append_hint and error_prepend will not work, if errp ==
> &fatal_error, as program will exit before error_append_hint or
> error_prepend call. Fix this by use of special macro
> ERRP_AUTO_PROPAGATE.

This patch doesn't actually fix any code, but adds the script to enable 
automating the fixing of the code in subsequent patches.  Tweaking the 
commit message to make that point clear might be helpful.


>   scripts/coccinelle/fix-error-add-info.cocci | 28 +++++++++++++++++++++
>   1 file changed, 28 insertions(+)
>   create mode 100644 scripts/coccinelle/fix-error-add-info.cocci
> 
> diff --git a/scripts/coccinelle/fix-error-add-info.cocci b/scripts/coccinelle/fix-error-add-info.cocci
> new file mode 100644
> index 0000000000..34fa3be720
> --- /dev/null
> +++ b/scripts/coccinelle/fix-error-add-info.cocci
> @@ -0,0 +1,28 @@
> +@rule0@
> +// Add invocation to errp-functions
> +identifier fn;
> +@@
> +
> + fn(..., Error **errp, ...)
> + {
> ++   ERRP_AUTO_PROPAGATE();
> +    <+...
> +(
> +    error_append_hint(errp, ...);
> +|
> +    error_prepend(errp, ...);
> +)

So, for now, you aren't addressing any *errp usage, or any potential 
cleanups of error_propagate.  But that's okay; your cover letter did 
call out that you were only addressing 1 part out of 3 potential uses 
just to get some motion, based on the size of the effort.

> +    ...+>
> + }
> +
> +@@
> +// Drop doubled invocation
> +identifier rule0.fn;
> +@@
> +
> + fn(...)
> +{
> +    ERRP_AUTO_PROPAGATE();
> +-   ERRP_AUTO_PROPAGATE();
> +    ...
> +}
> 

This looks idempotent once a file is patched, and safe to rerun as many 
times in the future as needed.  I'm still hoping we can find a way to 
make scripts/checkpatch.pl also do a sanity check, but as it's harder to 
parse C in perl than in Coccinelle, I can live with just the .cocci 
script in-tree as long as we remember to manually run it periodically.

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 v4 05/31] scripts: add script to fix error_append_hint/error_prepend usage

[Eric Blake]

On 10/1/19 10:52 AM, Vladimir Sementsov-Ogievskiy wrote:
> error_append_hint and error_prepend will not work, if errp ==
> &fatal_error, as program will exit before error_append_hint or
> error_prepend call. Fix this by use of special macro
> ERRP_AUTO_PROPAGATE.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 

> +++ b/scripts/coccinelle/fix-error-add-info.cocci
> @@ -0,0 +1,28 @@
> +@rule0@
> +// Add invocation to errp-functions
> +identifier fn;
> +@@
> +
> + fn(..., Error **errp, ...)
> + {

When running this script, I get lots of warnings:

init_defs_builtins: /usr/lib64/coccinelle/standard.h
warning: line 6: should errp be a metavariable?
warning: line 11: should errp be a metavariable?
warning: line 13: should errp be a metavariable?
HANDLING: include/block/nbd.h

This means we are trying to patch only an actual variable named 'errp', 
and not a meta-variable representing any variable of type Error ** 
regardless of whether it is named errp or something else.  I think 
that's okay, in part because ERRP_AUTO_PROPAGATE() expects the exact 
name 'errp' to be in scope, so we cannot apply it to other variable 
names).  But I still have to wonder if there is a way to silence the 
warning, or if we have any outliers (other than the intentional Error 
**errp_in in patch 1) using a different name for a parameter of type 
Error **, which should also be tweaked.

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

Re: [PATCH v4 05/31] scripts: add script to fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

01.10.2019 19:22, Eric Blake wrote:
> On 10/1/19 10:52 AM, Vladimir Sementsov-Ogievskiy wrote:
>> error_append_hint and error_prepend will not work, if errp ==
>> &fatal_error, as program will exit before error_append_hint or
>> error_prepend call. Fix this by use of special macro
>> ERRP_AUTO_PROPAGATE.
> 
> This patch doesn't actually fix any code, but adds the script to enable automating the fixing of the code in subsequent patches.  Tweaking the commit message to make that point clear might be helpful.

Hmm, so, maybe, switch "Fix this by use ..." to

To fix code with help of this script do
spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci FILE_TO_FIX


> 
> 
>>   scripts/coccinelle/fix-error-add-info.cocci | 28 +++++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>   create mode 100644 scripts/coccinelle/fix-error-add-info.cocci
>>
>> diff --git a/scripts/coccinelle/fix-error-add-info.cocci b/scripts/coccinelle/fix-error-add-info.cocci
>> new file mode 100644
>> index 0000000000..34fa3be720
>> --- /dev/null
>> +++ b/scripts/coccinelle/fix-error-add-info.cocci
>> @@ -0,0 +1,28 @@
>> +@rule0@
>> +// Add invocation to errp-functions
>> +identifier fn;
>> +@@
>> +
>> + fn(..., Error **errp, ...)
>> + {
>> ++   ERRP_AUTO_PROPAGATE();
>> +    <+...
>> +(
>> +    error_append_hint(errp, ...);
>> +|
>> +    error_prepend(errp, ...);
>> +)
> 
> So, for now, you aren't addressing any *errp usage, or any potential cleanups of error_propagate.  But that's okay; your cover letter did call out that you were only addressing 1 part out of 3 potential uses just to get some motion, based on the size of the effort.
> 
>> +    ...+>
>> + }
>> +
>> +@@
>> +// Drop doubled invocation
>> +identifier rule0.fn;
>> +@@
>> +
>> + fn(...)
>> +{
>> +    ERRP_AUTO_PROPAGATE();
>> +-   ERRP_AUTO_PROPAGATE();
>> +    ...
>> +}
>>
> 
> This looks idempotent once a file is patched, and safe to rerun as many times in the future as needed.  I'm still hoping we can find a way to make scripts/checkpatch.pl also do a sanity check, but as it's harder to parse C in perl than in Coccinelle, I can live with just the .cocci script in-tree as long as we remember to manually run it periodically.

scripts/checkpatch.pl is so unfriendly.. And to run coccinelle scripts we need
working directory, not just patch files.

I imagine the following:

move scripts/checkpatch.pl to scripts/checkpatch dir

Create scripts/checkpatch.py (or python/checkpatch.py, I don't
know what is the idea of python/ dir), which will work only on
commits or on patch files with specified base (master by default),
it will create temporary worktree, checkout corresponding commit
and then run scripts from scripts/checkpatch/, at least it would be
checkpatch.pl and coccinelle.sh (which will run some coccinelle
scripts from coccinelle dir)..

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


-- 
Best regards,
Vladimir

Re: [PATCH v4 05/31] scripts: add script to fix error_append_hint/error_prepend usage

[Eric Blake]

On 10/1/19 11:50 AM, Eric Blake wrote:
> On 10/1/19 10:52 AM, Vladimir Sementsov-Ogievskiy wrote:
>> error_append_hint and error_prepend will not work, if errp ==
>> &fatal_error, as program will exit before error_append_hint or
>> error_prepend call. Fix this by use of special macro
>> ERRP_AUTO_PROPAGATE.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
> 
>> +++ b/scripts/coccinelle/fix-error-add-info.cocci
>> @@ -0,0 +1,28 @@
>> +@rule0@
>> +// Add invocation to errp-functions
>> +identifier fn;

Adding:

symbol errp;

to this section appears to be the correct solution for...

>> +@@
>> +
>> + fn(..., Error **errp, ...)
>> + {
> 
> When running this script, I get lots of warnings:
> 
> init_defs_builtins: /usr/lib64/coccinelle/standard.h
> warning: line 6: should errp be a metavariable?
> warning: line 11: should errp be a metavariable?
> warning: line 13: should errp be a metavariable?
> HANDLING: include/block/nbd.h
> 

silencing these warnings.  But it's still not enough to fix the issue 
I'm pointing out in patch 23 being incomplete.


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

Re: [PATCH v4 23/31] block: Fix error_append_hint/error_prepend usage

[Eric Blake]

On 10/1/19 10:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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.
> 
> This commit (together with its neighbors) was generated by
> 
> git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
> spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
> --in-place $f; done
> 
> and then
> 
> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
> 
> (auto-msg was a file with this commit message)
> 
> and then by hand, for not maintained changed files:
> 
> git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>
> 
> Still, for backporting it may be more comfortable to use only the first
> command and then do one huge commit.
> 
> Reported-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   include/block/nbd.h  | 1 +
>   block.c              | 3 +++
>   block/backup.c       | 1 +
>   block/dirty-bitmap.c | 1 +
>   block/file-posix.c   | 4 ++++
>   block/gluster.c      | 2 ++
>   block/qcow.c         | 1 +
>   block/qcow2-bitmap.c | 1 +
>   block/qcow2.c        | 3 +++
>   block/vdi.c          | 1 +
>   block/vhdx-log.c     | 1 +
>   block/vmdk.c         | 1 +
>   block/vpc.c          | 1 +
>   13 files changed, 21 insertions(+)

The addition of error_prepend() checking makes this patch larger than in v3.

But similar to v3, I was able to come up with a matching grep query:

$ git grep -np 'error_\(append_hint\|prepend\)(errp' \
    block.c block/ include/block/ \
   | grep '\.[ch]=' | wc -l
22

and see that grep found one more instance than coccinelle.  Looking 
closer, I see where my diff found a spot you missed:

block/qcow2-bitmap.c=1448=void 
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
block/qcow2-bitmap.c=1671=fail:

[Ouch - there's a REASON that emacs prefers indenting labels at column 
'2, 5, 9, 13, ...' rather than '1, 5, 9, 13' - that's because anything 
at column 1 messes up determination of 'grep -p' for which function owns 
the code, showing the label at 1671 instead of the function 
qcow2_can_store_new_dirty_bitmap at 1618 - but that's a side point]


> +++ b/block/qcow2-bitmap.c
> @@ -1618,6 +1618,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>                                         uint32_t granularity,
>                                         Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       BDRVQcow2State *s = bs->opaque;
>       bool found;
>       Qcow2BitmapList *bm_list;
> diff --git a/block/qcow2.c b/block/qcow2.c

Your patch is missing a patch to qcow2_store_persistent_dirty_bitmaps(), 
which calls error_prepend(errp, ...).  However, when I manually ran the 
same spatch command line, I also got the same failure to include a fix 
in that function.

I'm not sure what's wrong with the .cocci script to miss that instance; 
I've tried fiddling around with the .cocci file to see if I can spot any 
change to make (for example, using ... instead of <+...), but so far, 
with no success at getting the second function patched.

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

Re: [PATCH v4 05/31] scripts: add script to fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

01.10.2019 19:50, Eric Blake wrote:
> On 10/1/19 10:52 AM, Vladimir Sementsov-Ogievskiy wrote:
>> error_append_hint and error_prepend will not work, if errp ==
>> &fatal_error, as program will exit before error_append_hint or
>> error_prepend call. Fix this by use of special macro
>> ERRP_AUTO_PROPAGATE.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
> 
>> +++ b/scripts/coccinelle/fix-error-add-info.cocci
>> @@ -0,0 +1,28 @@
>> +@rule0@
>> +// Add invocation to errp-functions
>> +identifier fn;
>> +@@
>> +
>> + fn(..., Error **errp, ...)
>> + {
> 
> When running this script, I get lots of warnings:
> 
> init_defs_builtins: /usr/lib64/coccinelle/standard.h
> warning: line 6: should errp be a metavariable?
> warning: line 11: should errp be a metavariable?
> warning: line 13: should errp be a metavariable?
> HANDLING: include/block/nbd.h
> 
> This means we are trying to patch only an actual variable named 'errp', and not a meta-variable representing any variable of type Error ** regardless of whether it is named errp or something else.  I think that's okay, in part because ERRP_AUTO_PROPAGATE() expects the exact name 'errp' to be in scope, so we cannot apply it to other variable names).  But I still have to wonder if there is a way to silence the warning, or if we have any outliers (other than the intentional Error **errp_in in patch 1) using a different name for a parameter of type Error **, which should also be tweaked.
> 


Hmm, good question:

git grep 'Error \*\*' | grep -v '\<Error \*\*\(errp\|)\)'
block/nbd.c:                                   int ret, Error **local_err)
block/quorum.c:    /* XXX - would be nice if we could pass in the Error **
block/snapshot.c:                             Error **err)
docs/devel/qapi-code-gen.txt:complex argument type.  It takes an additional Error ** argument in
docs/devel/writing-qmp-commands.txt:3. It takes an "Error **" argument. This is required. Later we will see how to
hw/core/qdev.c:static bool check_only_migratable(Object *obj, Error **err)
hw/core/qdev.c:        Error **local_errp = NULL;
hw/core/qdev.c:static bool device_get_hotplugged(Object *obj, Error **err)
hw/i386/amd_iommu.c:static void amdvi_realize(DeviceState *dev, Error **err)
hw/s390x/event-facility.c:static bool sclp_event_get_allow_all_mask_sizes(Object *obj, Error **e)
hw/s390x/s390-stattrib.c:static inline bool s390_stattrib_get_migration_enabled(Object *obj, Error **e)
hw/sd/sdhci.c:static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
hw/tpm/tpm_emulator.c:                                     Error **err)
hw/usb/dev-network.c:static void usb_net_realize(USBDevice *dev, Error **errrp)
include/block/snapshot.h:                             Error **err);
include/io/channel.h:                           Error **erp);
include/qapi/error.h:void error_propagate(Error **dst_errp, Error *local_err);
include/qapi/error.h:void error_propagate_prepend(Error **dst_errp, Error *local_err,
include/qom/object.h:                                    const uint64_t *v, Error **Errp);
include/qom/object.h:                                          const uint64_t *v, Error **Errp);
job.c:/* A wrapper around job_cancel() taking an Error ** parameter so it may be
monitor/qmp-cmds.c:void qmp_system_powerdown(Error **erp)
qga/commands-posix.c:GuestUserList *qmp_guest_get_users(Error **err)
qga/commands-win32.c:GuestUserList *qmp_guest_get_users(Error **err)
qga/commands.c:GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **err)
qga/commands.c:                       Error **err)
qga/commands.c:GuestHostName *qmp_guest_get_host_name(Error **err)
scripts/coccinelle/remove_local_err.cocci: F(..., Error **ERRP)
tests/tpm-util.c:                              SocketAddress **addr, GError **error)
tests/tpm-util.h:                              SocketAddress **addr, GError **error);
util/error.c:void error_propagate(Error **dst_errp, Error *local_err)
util/error.c:void error_propagate_prepend(Error **dst_errp, Error *err,


My favorite is errrp :)

But I tend to not do more effort, until I don't even know, will these series be
applied or not. All these cases are to be checked and fixed by hand, it may be done later.

Check possibly missed by previous grep things:

git grep 'Error \*\*' | grep 'Error \*\*.*Error \*\*'
include/qemu/job.h:int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp);
job.c:int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp)

- seems, nothing missed.

-- 
Best regards,
Vladimir

Re: [PATCH v4 29/31] nbd: Fix error_append_hint/error_prepend usage

[Eric Blake]

On 10/1/19 10:53 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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.
> 

> ---
>   nbd/client.c | 5 +++++
>   nbd/server.c | 4 ++++
>   2 files changed, 9 insertions(+)
> 

> +++ b/nbd/server.c
> @@ -365,6 +365,7 @@ static int nbd_opt_read_name(NBDClient *client, char *name, uint32_t *length,
>   static int nbd_negotiate_send_rep_list(NBDClient *client, NBDExport *exp,
>                                          Error **errp)
>   {
> +    ERRP_AUTO_PROPAGATE();
>       size_t name_len, desc_len;

This missed nbd_negotiate_send_rep_verr().  But when I reran the script 
manually on the same two files, it picked up for me.  Not sure why that 
is different.

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

Re: [PATCH v4 23/31] block: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

01.10.2019 20:09, Eric Blake wrote:
> On 10/1/19 10:53 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 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.
>>
>> This commit (together with its neighbors) was generated by
>>
>> git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
>> spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
>> --in-place $f; done
>>
>> and then
>>
>> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
>>
>> (auto-msg was a file with this commit message)
>>
>> and then by hand, for not maintained changed files:
>>
>> git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>
>>
>> Still, for backporting it may be more comfortable to use only the first
>> command and then do one huge commit.
>>
>> Reported-by: Greg Kurz <groug@kaod.org>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   include/block/nbd.h  | 1 +
>>   block.c              | 3 +++
>>   block/backup.c       | 1 +
>>   block/dirty-bitmap.c | 1 +
>>   block/file-posix.c   | 4 ++++
>>   block/gluster.c      | 2 ++
>>   block/qcow.c         | 1 +
>>   block/qcow2-bitmap.c | 1 +
>>   block/qcow2.c        | 3 +++
>>   block/vdi.c          | 1 +
>>   block/vhdx-log.c     | 1 +
>>   block/vmdk.c         | 1 +
>>   block/vpc.c          | 1 +
>>   13 files changed, 21 insertions(+)
> 
> The addition of error_prepend() checking makes this patch larger than in v3.
> 
> But similar to v3, I was able to come up with a matching grep query:
> 
> $ git grep -np 'error_\(append_hint\|prepend\)(errp' \
>     block.c block/ include/block/ \
>    | grep '\.[ch]=' | wc -l
> 22
> 
> and see that grep found one more instance than coccinelle.  Looking closer, I see where my diff found a spot you missed:
> 
> block/qcow2-bitmap.c=1448=void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> block/qcow2-bitmap.c=1671=fail:
> 
> [Ouch - there's a REASON that emacs prefers indenting labels at column '2, 5, 9, 13, ...' rather than '1, 5, 9, 13' - that's because anything at column 1 messes up determination of 'grep -p' for which function owns the code, showing the label at 1671 instead of the function qcow2_can_store_new_dirty_bitmap at 1618 - but that's a side point]
> 
> 
>> +++ b/block/qcow2-bitmap.c
>> @@ -1618,6 +1618,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>                                         uint32_t granularity,
>>                                         Error **errp)
>>   {
>> +    ERRP_AUTO_PROPAGATE();
>>       BDRVQcow2State *s = bs->opaque;
>>       bool found;
>>       Qcow2BitmapList *bm_list;
>> diff --git a/block/qcow2.c b/block/qcow2.c
> 
> Your patch is missing a patch to qcow2_store_persistent_dirty_bitmaps(), which calls error_prepend(errp, ...).  However, when I manually ran the same spatch command line, I also got the same failure to include a fix in that function.
> 
> I'm not sure what's wrong with the .cocci script to miss that instance; I've tried fiddling around with the .cocci file to see if I can spot any change to make (for example, using ... instead of <+...), but so far, with no success at getting the second function patched.
> 


Hmm, interesting. Actually I think that coccinelle is something that just works bad from the beginning of these series. And here:

coccinelle works with

void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
{
     error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ",
                   name);
}


but failes with:

void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
{
     QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables;

     error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ",
                   name);
}

So, it can't parse "QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables" thing..
It even can't parse it without comma:

void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
{
     QSIMPLEQ_HEAD(Qcow2BitmapTable) drop_tables;

     error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ",
                   name);
}

----

aha, it works if I add definition of QSIMPLEQ_HEAD. So, we may miss includes?

adding --recursive-includes parameter to spatch leads to error:

[.. a lot of failures]
failed on sys/shm.h
failed on sys/uio.h
failed on qapi/qapi-types-error.h
failed on qapi/qapi-types-crypto.h
failed on sys/endian.h
failed on machine/bswap.h
failed on byteswap.h
failed on pthread.h
failed on semaphore.h
failed on qapi/qapi-builtin-types.h
failed on qapi/qapi-types-block-core.h
failed on qapi/qapi-types-job.h
failed on qcow2.h
Impossible: How can diff be null and have not Correct in compare_c? Tag1 ("diff token: QEMU_PACKED VS QEMU_PACKED\nFile \"block/qcow2-bitmap.c\", line 59, column 15, charpos = 2334\n  around = 'QEMU_PACKED',\n  whole content = typedef struct QEMU_PACKED Qcow2BitmapDirEntry {\nFile \"/tmp/cocci-output-10311-cc4e45-qcow2-bitmap.c\", line 59, column 15, charpos = 2334\n  around = 'QEMU_PACKED',\n  whole content = typedef struct QEMU_PACKED Qcow2BitmapDirEntry {\n")

Aha, we need -I option. Something like

spatch --verbose-parsing --verbose-includes -I include -I '.' -I block --recursive-includes --sp-file scripts/coccinelle/fix-error-add-info.cocci block/qcow2-bitmap.c 2>&1


And it just can't parse our includes, queue.h for example.. So many parsing errors.

...

including include/qemu/queue.h
ERROR-RECOV: found sync end of #define, line 90
parsing pass2: try again
ERROR-RECOV: found sync end of #define, line 90
parse error
  = File "include/qemu/queue.h", line 90, column 15, charpos = 4359
   around = '}',
   whole content =         { NULL }
badcount: 3
bad: }
bad:
bad: #define QLIST_HEAD_INITIALIZER(head)                                    \
BAD:!!!!!         { NULL }
ERROR-RECOV: found sync end of #define, line 150
parsing pass2: try again
ERROR-RECOV: found sync end of #define, line 150
parse error
  = File "include/qemu/queue.h", line 150, column 47, charpos = 7628
   around = '',
   whole content =                 (var) = ((var)->field.le_next))
badcount: 5
bad: } while (/*CONSTCOND*/0)
bad:
bad: #define QLIST_FOREACH(var, head, field)                                 \
bad:         for ((var) = ((head)->lh_first);                                \
bad:                 (var);                                                  \
BAD:!!!!!                 (var) = ((var)->field.le_next))
ERROR-RECOV: found sync end of #define, line 155
parsing pass2: try again

...


So, it seems like coccinelle just don't work. At least it don't allow to define initializer macro.

Any ideas? The series is still meaningful. Not all bugs are fixed, but at least some bugs are fixed.

I'm afraid I can't put more effort on this topic, it already ate a lot of time.

As an alternative I can suggest Greg to rebase his series on my patch 04 and forget about error_append
and so on.

Hmm or may be try some simple regex instead of coccinelle?


Something as simple as substitute
(^[^{}]+\([^{}]*Error \*\*errp[^{}]*\)\s*^\{)(([^}]|(?<!^)})*error_(prepend|append_hint)\(errp)

by

\1\n    ERRP_AUTO_PROPAGATE();\2

seems work

-- 
Best regards,
Vladimir

Re: [PATCH v4 23/31] block: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

01.10.2019 21:55, Vladimir Sementsov-Ogievskiy wrote:
> 01.10.2019 20:09, Eric Blake wrote:
>> On 10/1/19 10:53 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> 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.
>>>
>>> This commit (together with its neighbors) was generated by
>>>
>>> git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
>>> spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
>>> --in-place $f; done
>>>
>>> and then
>>>
>>> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
>>>
>>> (auto-msg was a file with this commit message)
>>>
>>> and then by hand, for not maintained changed files:
>>>
>>> git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>
>>>
>>> Still, for backporting it may be more comfortable to use only the first
>>> command and then do one huge commit.
>>>
>>> Reported-by: Greg Kurz <groug@kaod.org>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   include/block/nbd.h  | 1 +
>>>   block.c              | 3 +++
>>>   block/backup.c       | 1 +
>>>   block/dirty-bitmap.c | 1 +
>>>   block/file-posix.c   | 4 ++++
>>>   block/gluster.c      | 2 ++
>>>   block/qcow.c         | 1 +
>>>   block/qcow2-bitmap.c | 1 +
>>>   block/qcow2.c        | 3 +++
>>>   block/vdi.c          | 1 +
>>>   block/vhdx-log.c     | 1 +
>>>   block/vmdk.c         | 1 +
>>>   block/vpc.c          | 1 +
>>>   13 files changed, 21 insertions(+)
>>
>> The addition of error_prepend() checking makes this patch larger than in v3.
>>
>> But similar to v3, I was able to come up with a matching grep query:
>>
>> $ git grep -np 'error_\(append_hint\|prepend\)(errp' \
>>     block.c block/ include/block/ \
>>    | grep '\.[ch]=' | wc -l
>> 22
>>
>> and see that grep found one more instance than coccinelle.  Looking closer, I see where my diff found a spot you missed:
>>
>> block/qcow2-bitmap.c=1448=void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
>> block/qcow2-bitmap.c=1671=fail:
>>
>> [Ouch - there's a REASON that emacs prefers indenting labels at column '2, 5, 9, 13, ...' rather than '1, 5, 9, 13' - that's because anything at column 1 messes up determination of 'grep -p' for which function owns the code, showing the label at 1671 instead of the function qcow2_can_store_new_dirty_bitmap at 1618 - but that's a side point]
>>
>>
>>> +++ b/block/qcow2-bitmap.c
>>> @@ -1618,6 +1618,7 @@ bool qcow2_can_store_new_dirty_bitmap(BlockDriverState *bs,
>>>                                         uint32_t granularity,
>>>                                         Error **errp)
>>>   {
>>> +    ERRP_AUTO_PROPAGATE();
>>>       BDRVQcow2State *s = bs->opaque;
>>>       bool found;
>>>       Qcow2BitmapList *bm_list;
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>
>> Your patch is missing a patch to qcow2_store_persistent_dirty_bitmaps(), which calls error_prepend(errp, ...).  However, when I manually ran the same spatch command line, I also got the same failure to include a fix in that function.
>>
>> I'm not sure what's wrong with the .cocci script to miss that instance; I've tried fiddling around with the .cocci file to see if I can spot any change to make (for example, using ... instead of <+...), but so far, with no success at getting the second function patched.
>>
> 
> 
> Hmm, interesting. Actually I think that coccinelle is something that just works bad from the beginning of these series. And here:
> 
> coccinelle works with
> 
> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> {
>      error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ",
>                    name);
> }
> 
> 
> but failes with:
> 
> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> {
>      QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables;
> 
>      error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ",
>                    name);
> }
> 
> So, it can't parse "QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables" thing..
> It even can't parse it without comma:
> 
> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> {
>      QSIMPLEQ_HEAD(Qcow2BitmapTable) drop_tables;
> 
>      error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ",
>                    name);
> }
> 
> ----
> 
> aha, it works if I add definition of QSIMPLEQ_HEAD. So, we may miss includes?
> 
> adding --recursive-includes parameter to spatch leads to error:
> 
> [.. a lot of failures]
> failed on sys/shm.h
> failed on sys/uio.h
> failed on qapi/qapi-types-error.h
> failed on qapi/qapi-types-crypto.h
> failed on sys/endian.h
> failed on machine/bswap.h
> failed on byteswap.h
> failed on pthread.h
> failed on semaphore.h
> failed on qapi/qapi-builtin-types.h
> failed on qapi/qapi-types-block-core.h
> failed on qapi/qapi-types-job.h
> failed on qcow2.h
> Impossible: How can diff be null and have not Correct in compare_c? Tag1 ("diff token: QEMU_PACKED VS QEMU_PACKED\nFile \"block/qcow2-bitmap.c\", line 59, column 15, charpos = 2334\n  around = 'QEMU_PACKED',\n  whole content = typedef struct QEMU_PACKED Qcow2BitmapDirEntry {\nFile \"/tmp/cocci-output-10311-cc4e45-qcow2-bitmap.c\", line 59, column 15, charpos = 2334\n  around = 'QEMU_PACKED',\n  whole content = typedef struct QEMU_PACKED Qcow2BitmapDirEntry {\n")
> 
> Aha, we need -I option. Something like
> 
> spatch --verbose-parsing --verbose-includes -I include -I '.' -I block --recursive-includes --sp-file scripts/coccinelle/fix-error-add-info.cocci block/qcow2-bitmap.c 2>&1
> 
> 
> And it just can't parse our includes, queue.h for example.. So many parsing errors.
> 
> ...
> 
> including include/qemu/queue.h
> ERROR-RECOV: found sync end of #define, line 90
> parsing pass2: try again
> ERROR-RECOV: found sync end of #define, line 90
> parse error
>   = File "include/qemu/queue.h", line 90, column 15, charpos = 4359
>    around = '}',
>    whole content =         { NULL }
> badcount: 3
> bad: }
> bad:
> bad: #define QLIST_HEAD_INITIALIZER(head)                                    \
> BAD:!!!!!         { NULL }
> ERROR-RECOV: found sync end of #define, line 150
> parsing pass2: try again
> ERROR-RECOV: found sync end of #define, line 150
> parse error
>   = File "include/qemu/queue.h", line 150, column 47, charpos = 7628
>    around = '',
>    whole content =                 (var) = ((var)->field.le_next))
> badcount: 5
> bad: } while (/*CONSTCOND*/0)
> bad:
> bad: #define QLIST_FOREACH(var, head, field)                                 \
> bad:         for ((var) = ((head)->lh_first);                                \
> bad:                 (var);                                                  \
> BAD:!!!!!                 (var) = ((var)->field.le_next))
> ERROR-RECOV: found sync end of #define, line 155
> parsing pass2: try again
> 
> ...
> 
> 
> So, it seems like coccinelle just don't work. At least it don't allow to define initializer macro.
> 
> Any ideas? The series is still meaningful. Not all bugs are fixed, but at least some bugs are fixed.
> 
> I'm afraid I can't put more effort on this topic, it already ate a lot of time.
> 
> As an alternative I can suggest Greg to rebase his series on my patch 04 and forget about error_append
> and so on.
> 
> Hmm or may be try some simple regex instead of coccinelle?
> 
> 
> Something as simple as substitute
> (^[^{}]+\([^{}]*Error \*\*errp[^{}]*\)\s*^\{)(([^}]|(?<!^)})*error_(prepend|append_hint)\(errp)
> 
> by
> 
> \1\n    ERRP_AUTO_PROPAGATE();\2
> 
> seems work
> 

Hahahaha.

I tried, it works. And it generate exactly same patches as in these series, except only two functions in nbd
and block which you've found. It's amazing. I'll drop coccinelle script and resend. My current script looks as
follows. Hmm I'd better rewrite regexp in a bit more readable manner.

#!/usr/bin/env python
import re
import sys

r = re.compile(r"(^[^{}]+\([^{}]*Error \*\*errp[^{}]*\)\s*^\{)(([^}]|(?<!^)})*error_(prepend|append_hint)\(errp)", re.M)

with open(sys.argv[1]) as f:
     text = f.read()

text = r.sub(r'\1\n    ERRP_AUTO_PROPAGATE();\2', text)

with open(sys.argv[1], 'w') as f:
     f.write(text)

-- 
Best regards,
Vladimir

Re: [PATCH v4 23/31] block: Fix error_append_hint/error_prepend usage

[Eric Blake]

On 10/1/19 1:55 PM, Vladimir Sementsov-Ogievskiy wrote:

>> Your patch is missing a patch to qcow2_store_persistent_dirty_bitmaps(), which calls error_prepend(errp, ...).  However, when I manually ran the same spatch command line, I also got the same failure to include a fix in that function.
>>
>> I'm not sure what's wrong with the .cocci script to miss that instance; I've tried fiddling around with the .cocci file to see if I can spot any change to make (for example, using ... instead of <+...), but so far, with no success at getting the second function patched.
>>
> 
> 
> Hmm, interesting. Actually I think that coccinelle is something that just works bad from the beginning of these series. And here:
> 

> but failes with:
> 
> void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp)
> {
>       QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables;
> 
>       error_prepend(errp, "Bitmap '%s' doesn't satisfy the constraints: ",
>                     name);
> }
> 
> So, it can't parse "QSIMPLEQ_HEAD(, Qcow2BitmapTable) drop_tables" thing..

Generally, when running spatch, you want to include --macro-file 
scripts/cocci-macro-file.h to help coccinelle get past the worst of the 
preprocessor macros it can't otherwise handle.  But that is rather sad 
that it ignores the entire function body as soon as it encounters a 
parse problem, and also sad that scripts/cocci-macro-file.h isn't yet 
complete enough to help coccinelle past the QSIMPLEQ_HEAD() uses in our 
sources.  (I wonder how many other false negatives we have where we 
missed a code cleanup because Coccinelle silently gave up on parsing a 
function or file)


> adding --recursive-includes parameter to spatch leads to error:
> 
> [.. a lot of failures]
> failed on sys/shm.h
> failed on sys/uio.h
> failed on qapi/qapi-types-error.h
> failed on qapi/qapi-types-crypto.h
> failed on sys/endian.h
> failed on machine/bswap.h
> failed on byteswap.h
> failed on pthread.h
> failed on semaphore.h
> failed on qapi/qapi-builtin-types.h
> failed on qapi/qapi-types-block-core.h
> failed on qapi/qapi-types-job.h
> failed on qcow2.h
> Impossible: How can diff be null and have not Correct in compare_c? Tag1 ("diff token: QEMU_PACKED VS QEMU_PACKED\nFile \"block/qcow2-bitmap.c\", line 59, column 15, charpos = 2334\n  around = 'QEMU_PACKED',\n  whole content = typedef struct QEMU_PACKED Qcow2BitmapDirEntry {\nFile \"/tmp/cocci-output-10311-cc4e45-qcow2-bitmap.c\", line 59, column 15, charpos = 2334\n  around = 'QEMU_PACKED',\n  whole content = typedef struct QEMU_PACKED Qcow2BitmapDirEntry {\n")

Eww - that sounds like a Coccinelle bug that we should report to their 
upstream.

> 
> Aha, we need -I option. Something like
> 
> spatch --verbose-parsing --verbose-includes -I include -I '.' -I block --recursive-includes --sp-file scripts/coccinelle/fix-error-add-info.cocci block/qcow2-bitmap.c 2>&1
> 
> 
> And it just can't parse our includes, queue.h for example.. So many parsing errors.

We may not need to parse all our headers, if the --macro-file 
scripts/cocci-macro-file.h is sufficient.

In fact, now that I found that (by reading through git log history of 
previous Coccinelle scripts we've run), and adding the proper 
--macro-file command line argument, I didn't have to add a 
--recursive-includes, but Coccinelle was finally able to fix that last 
spot in block/qcow2-bitmap.c.


> So, it seems like coccinelle just don't work. At least it don't allow to define initializer macro.
> 
> Any ideas? The series is still meaningful. Not all bugs are fixed, but at least some bugs are fixed.
> 

Using --macro-file sscripts/cocci-macro-file.h should get it a lot 
further.  The sad part is I don't have a quantitative way to tell how 
many functions/files are being silently skipped when Coccinelle runs up 
against something it doesn't know how to parse.

> I'm afraid I can't put more effort on this topic, it already ate a lot of time.
> 
> As an alternative I can suggest Greg to rebase his series on my patch 04 and forget about error_append
> and so on.
> 
> Hmm or may be try some simple regex instead of coccinelle?
> 
> 
> Something as simple as substitute
> (^[^{}]+\([^{}]*Error \*\*errp[^{}]*\)\s*^\{)(([^}]|(?<!^)})*error_(prepend|append_hint)\(errp)
> 
> by
> 
> \1\n    ERRP_AUTO_PROPAGATE();\2
> 
> seems work

A little more blunt (and as written, not idempotent), but as long as we 
document whatever pattern we use (whether coccinelle or regex) and the 
patch is repeatable, that's less of a concern.

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

Re: [PATCH v4 00/31] error: auto propagated local_err

[no-reply]

Patchew URL: https://patchew.org/QEMU/20191001155319.8066-1-vsementsov@virtuozzo.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20191001155319.8066-1-vsementsov@virtuozzo.com
Subject: [PATCH v4 00/31] error: auto propagated local_err

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
f0134cb ivshmem: Fix error_append_hint/error_prepend usage
927e8d8 PVRDMA: Fix error_append_hint/error_prepend usage
c901c43 nbd: Fix error_append_hint/error_prepend usage
a990496 Sockets: Fix error_append_hint/error_prepend usage
7e9b7be Migration: Fix error_append_hint/error_prepend usage
c37742f QOM: Fix error_append_hint/error_prepend usage
e2d8729 cmdline: Fix error_append_hint/error_prepend usage
111a147 chardev: Fix error_append_hint/error_prepend usage
7f7095d block: Fix error_append_hint/error_prepend usage
42992e2 XIVE: Fix error_append_hint/error_prepend usage
aef0d72 virtio-9p: Fix error_append_hint/error_prepend usage
fadcc18 virtio: Fix error_append_hint/error_prepend usage
0b46fe6 vhost: Fix error_append_hint/error_prepend usage
2b5a95f VFIO: Fix error_append_hint/error_prepend usage
17cff57 USB: Fix error_append_hint/error_prepend usage
b8c7bd2 SCSI: Fix error_append_hint/error_prepend usage
2833abc PCI: Fix error_append_hint/error_prepend usage
61a303e PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage
c287bb4 Boston: Fix error_append_hint/error_prepend usage
163ff63 ASPEED BMCs: Fix error_append_hint/error_prepend usage
1eb85cb SmartFusion2: Fix error_append_hint/error_prepend usage
a29bb2d arm: Fix error_append_hint/error_prepend usage
af781f2 PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage
00ceb2b ARM TCG CPUs: Fix error_append_hint/error_prepend usage
a8ac50c s390: Fix error_append_hint/error_prepend usage
82e8c8b python: add commit-per-subsystem.py
a97cab0 scripts: add script to fix error_append_hint/error_prepend usage
987fd13 error: auto propagated local_err
b209564 net/net: fix local variable shadowing in net_client_init
896b2dd hw/core/loader-fit: fix freeing errp in fit_load_fdt
cd953ee errp: rename errp to errp_in where it is IN-argument

=== OUTPUT BEGIN ===
1/31 Checking commit cd953ee1bb8c (errp: rename errp to errp_in where it is IN-argument)
2/31 Checking commit 896b2dde85c4 (hw/core/loader-fit: fix freeing errp in fit_load_fdt)
3/31 Checking commit b209564a3bf7 (net/net: fix local variable shadowing in net_client_init)
4/31 Checking commit 987fd13e64fc (error: auto propagated local_err)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#71: FILE: include/qapi/error.h:357:
+#define ERRP_AUTO_PROPAGATE() \
+g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
+errp = ((errp == NULL || *errp == error_fatal) ? \
+    &__auto_errp_prop.local_err : errp)

total: 1 errors, 0 warnings, 43 lines checked

Patch 4/31 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

5/31 Checking commit a97cab09ecd5 (scripts: add script to fix error_append_hint/error_prepend usage)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#16: 
new file mode 100644

total: 0 errors, 1 warnings, 28 lines checked

Patch 5/31 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
6/31 Checking commit 82e8c8bfa5e9 (python: add commit-per-subsystem.py)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#13: 
new file mode 100755

total: 0 errors, 1 warnings, 69 lines checked

Patch 6/31 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/31 Checking commit a8ac50cc35ed (s390: Fix error_append_hint/error_prepend usage)
8/31 Checking commit 00ceb2bad505 (ARM TCG CPUs: Fix error_append_hint/error_prepend usage)
9/31 Checking commit af781f20f6a2 (PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage)
10/31 Checking commit a29bb2dcb2b9 (arm: Fix error_append_hint/error_prepend usage)
11/31 Checking commit 1eb85cb68ba5 (SmartFusion2: Fix error_append_hint/error_prepend usage)
12/31 Checking commit 163ff63668f9 (ASPEED BMCs: Fix error_append_hint/error_prepend usage)
13/31 Checking commit c287bb4ab3b2 (Boston: Fix error_append_hint/error_prepend usage)
14/31 Checking commit 61a303eb291b (PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage)
15/31 Checking commit 2833abc764d5 (PCI: Fix error_append_hint/error_prepend usage)
16/31 Checking commit b8c7bd233ee3 (SCSI: Fix error_append_hint/error_prepend usage)
17/31 Checking commit 17cff57051b2 (USB: Fix error_append_hint/error_prepend usage)
18/31 Checking commit 2b5a95f33325 (VFIO: Fix error_append_hint/error_prepend usage)
19/31 Checking commit 0b46fe629163 (vhost: Fix error_append_hint/error_prepend usage)
20/31 Checking commit fadcc18dfb3d (virtio: Fix error_append_hint/error_prepend usage)
21/31 Checking commit aef0d72aef21 (virtio-9p: Fix error_append_hint/error_prepend usage)
22/31 Checking commit 42992e22ff24 (XIVE: Fix error_append_hint/error_prepend usage)
23/31 Checking commit 7f7095d05e79 (block: Fix error_append_hint/error_prepend usage)
24/31 Checking commit 111a1472baf3 (chardev: Fix error_append_hint/error_prepend usage)
25/31 Checking commit e2d872961ff8 (cmdline: Fix error_append_hint/error_prepend usage)
26/31 Checking commit c37742f15571 (QOM: Fix error_append_hint/error_prepend usage)
27/31 Checking commit 7e9b7bef90c8 (Migration: Fix error_append_hint/error_prepend usage)
28/31 Checking commit a9904966d137 (Sockets: Fix error_append_hint/error_prepend usage)
29/31 Checking commit c901c432d406 (nbd: Fix error_append_hint/error_prepend usage)
30/31 Checking commit 927e8d8a6586 (PVRDMA: Fix error_append_hint/error_prepend usage)
31/31 Checking commit f0134cb1157d (ivshmem: Fix error_append_hint/error_prepend usage)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20191001155319.8066-1-vsementsov@virtuozzo.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v4 21/31] virtio-9p: Fix error_append_hint/error_prepend usage

[Greg Kurz]

On Tue,  1 Oct 2019 18:53:09 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

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

Well... this patch doesn't really fix anything because...

> This commit (together with its neighbors) was generated by
> 
> git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
> spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
> --in-place $f; done
> 
> and then
> 
> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
> 
> (auto-msg was a file with this commit message)
> 
> and then by hand, for not maintained changed files:
> 
> git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>
> 
> Still, for backporting it may be more comfortable to use only the first
> command and then do one huge commit.
> 
> Reported-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  hw/9pfs/9p-local.c | 1 +
>  hw/9pfs/9p-proxy.c | 1 +
>  hw/9pfs/9p.c       | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 08e673a79c..fccbf758bd 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1471,6 +1471,7 @@ static void local_cleanup(FsContext *ctx)
>  
>  static void error_append_security_model_hint(Error **errp)
>  {
> +    ERRP_AUTO_PROPAGATE();
>      error_append_hint(errp, "Valid options are: security_model="
>                        "[passthrough|mapped-xattr|mapped-file|none]\n");
>  }

This function doesn't need auto propagation in the first place. It is
simply a wrapper around error_append_hint(). ERRP_AUTO_PROPAGATE()
should go to the caller local_parse_opts().

Also some extra care is needed there to handle part [3.] of the
cleanup. I understand this is out of the scope of that series,
but I'd rather see all of this fixed in the same patch.

> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> index 57a8c1c808..9291c8efa2 100644
> --- a/hw/9pfs/9p-proxy.c
> +++ b/hw/9pfs/9p-proxy.c
> @@ -1116,6 +1116,7 @@ static int connect_namedsocket(const char *path, Error **errp)
>  
>  static void error_append_socket_sockfd_hint(Error **errp)
>  {
> +    ERRP_AUTO_PROPAGATE();
>      error_append_hint(errp, "Either specify socket=/some/path where /some/path"
>                        " points to a listening AF_UNIX socket or sock_fd=fd"
>                        " where fd is a file descriptor to a connected AF_UNIX"

Same here. ERRP_AUTO_PROPAGATE() should go to proxy_parse_opts().

> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
> index cce2366219..1df2749e03 100644
> --- a/hw/9pfs/9p.c
> +++ b/hw/9pfs/9p.c
> @@ -3552,6 +3552,7 @@ void pdu_submit(V9fsPDU *pdu, P9MsgHeader *hdr)
>  int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
>                                 Error **errp)
>  {
> +    ERRP_AUTO_PROPAGATE();

This is correct since this function calls error_prepend() but I think
errp is never &error_fatal or &error_abort on the realize path. Anyway,
better safe than sorry.

>      int i, len;
>      struct stat stat;
>      FsDriverEntry *fse;

Please drop this patch from your series and I'll do the change once
ERRP_AUTO_PROPAGATE() gets merged.

Great thanks for your time in finding a clever way to deal with error
propagation. :)

Cheers,

--
Greg

Re: [PATCH v4 04/31] error: auto propagated local_err

[Roman Kagan]

On Tue, Oct 01, 2019 at 06:52:52PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with errp OUT parameter.
> 
> It has three goals:
> 
> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
> 
> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
> local_err+error_propagate pattern, which will definitely fix the issue)
> [Reported by Kevin Wolf]
> 
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> CC: kwolf@redhat.com
> CC: mreitz@redhat.com
> CC: jsnow@redhat.com
> CC: fam@euphon.net
> CC: sw@weilnetz.de
> CC: codyprime@gmail.com
> CC: marcandre.lureau@redhat.com
> CC: pbonzini@redhat.com
> CC: groug@kaod.org
> CC: sundeep.lkml@gmail.com
> CC: peter.maydell@linaro.org
> CC: stefanha@redhat.com
> CC: pburton@wavecomp.com
> CC: arikalo@wavecomp.com
> CC: berrange@redhat.com
> CC: ehabkost@redhat.com
> CC: david@gibson.dropbear.id.au
> CC: clg@kaod.org
> CC: mst@redhat.com
> CC: marcel.apfelbaum@gmail.com
> CC: mark.cave-ayland@ilande.co.uk
> CC: yuval.shaia@oracle.com
> CC: cohuck@redhat.com
> CC: farman@linux.ibm.com
> CC: rth@twiddle.net
> CC: david@redhat.com
> CC: pasic@linux.ibm.com
> CC: borntraeger@de.ibm.com
> CC: kraxel@redhat.com
> CC: alex.williamson@redhat.com
> CC: andrew@aj.id.au
> CC: joel@jms.id.au
> CC: eblake@redhat.com
> CC: armbru@redhat.com
> CC: mdroth@linux.vnet.ibm.com
> CC: quintela@redhat.com
> CC: dgilbert@redhat.com
> CC: jasowang@redhat.com
> CC: qemu-block@nongnu.org
> CC: integration@gluster.org
> CC: qemu-arm@nongnu.org
> CC: qemu-ppc@nongnu.org
> CC: qemu-s390x@nongnu.org
> 
>  include/qapi/error.h | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 9376f59c35..02f967ac1d 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -322,6 +322,43 @@ void error_set_internal(Error **errp,
>                          ErrorClass err_class, const char *fmt, ...)
>      GCC_FMT_ATTR(6, 7);
>  
> +typedef struct ErrorPropagator {
> +    Error *local_err;
> +    Error **errp;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +    error_propagate(prop->errp, prop->local_err);
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
> +/*
> + * ERRP_AUTO_PROPAGATE
> + *
> + * This macro is created to be the first line of a function with Error **errp
> + * OUT parameter. It's needed only in cases where we want to use error_prepend,
> + * error_append_hint or dereference *errp. It's still safe (but useless) in
> + * other cases.
> + *
> + * If errp is NULL or points to error_fatal, it is rewritten to point to a
> + * local Error object, which will be automatically propagated to the original
> + * errp on function exit (see error_propagator_cleanup).
> + *
> + * After invocation of this macro it is always safe to dereference errp
> + * (as it's not NULL anymore) and to append hints (by error_append_hint)
> + * (as, if it was error_fatal, we swapped it with a local_error to be
> + * propagated on cleanup).
> + *
> + * Note: we don't wrap the error_abort case, as we want resulting coredump
> + * to point to the place where the error happened, not to error_propagate.
> + */
> +#define ERRP_AUTO_PROPAGATE() \
> +g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
> +errp = ((errp == NULL || *errp == error_fatal) ? \
> +    &__auto_errp_prop.local_err : errp)
> +

I guess it has been discussed but I couldn't find it, so: what's the
reason for casting in stone the name of the function parameter, which
isn't quite so obvious when you see this macro used in the code?  IMO
if the macro took errp as a parameter that would be easier to follow.

Thanks,
Roman.

Re: [PATCH v4 00/31] error: auto propagated local_err

[Markus Armbruster]

Reviewing this series is finally getting close to the head of my work
queue.  I apologize for the delay.

Re: [PATCH v4 21/31] virtio-9p: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

02.10.2019 12:19, Greg Kurz wrote:
> On Tue,  1 Oct 2019 18:53:09 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> 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.
>>
> 
> Well... this patch doesn't really fix anything because...

I'm sure it fixes. But it also breaks some things you mention below..

> 
>> This commit (together with its neighbors) was generated by
>>
>> git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
>> spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
>> --in-place $f; done
>>
>> and then
>>
>> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
>>
>> (auto-msg was a file with this commit message)
>>
>> and then by hand, for not maintained changed files:
>>
>> git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>
>>
>> Still, for backporting it may be more comfortable to use only the first
>> command and then do one huge commit.
>>
>> Reported-by: Greg Kurz <groug@kaod.org>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   hw/9pfs/9p-local.c | 1 +
>>   hw/9pfs/9p-proxy.c | 1 +
>>   hw/9pfs/9p.c       | 1 +
>>   3 files changed, 3 insertions(+)
>>
>> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
>> index 08e673a79c..fccbf758bd 100644
>> --- a/hw/9pfs/9p-local.c
>> +++ b/hw/9pfs/9p-local.c
>> @@ -1471,6 +1471,7 @@ static void local_cleanup(FsContext *ctx)
>>   
>>   static void error_append_security_model_hint(Error **errp)
>>   {
>> +    ERRP_AUTO_PROPAGATE();
>>       error_append_hint(errp, "Valid options are: security_model="
>>                         "[passthrough|mapped-xattr|mapped-file|none]\n");
>>   }
> 
> This function doesn't need auto propagation in the first place. It is
> simply a wrapper around error_append_hint(). ERRP_AUTO_PROPAGATE()
> should go to the caller local_parse_opts().

Hmm, that's bad. So, actually it's one more errp IN parameter.

> 
> Also some extra care is needed there to handle part [3.] of the
> cleanup. I understand this is out of the scope of that series,
> but I'd rather see all of this fixed in the same patch.
> 
>> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
>> index 57a8c1c808..9291c8efa2 100644
>> --- a/hw/9pfs/9p-proxy.c
>> +++ b/hw/9pfs/9p-proxy.c
>> @@ -1116,6 +1116,7 @@ static int connect_namedsocket(const char *path, Error **errp)
>>   
>>   static void error_append_socket_sockfd_hint(Error **errp)
>>   {
>> +    ERRP_AUTO_PROPAGATE();
>>       error_append_hint(errp, "Either specify socket=/some/path where /some/path"
>>                         " points to a listening AF_UNIX socket or sock_fd=fd"
>>                         " where fd is a file descriptor to a connected AF_UNIX"
> 
> Same here. ERRP_AUTO_PROPAGATE() should go to proxy_parse_opts().

and this one.

> 
>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>> index cce2366219..1df2749e03 100644
>> --- a/hw/9pfs/9p.c
>> +++ b/hw/9pfs/9p.c
>> @@ -3552,6 +3552,7 @@ void pdu_submit(V9fsPDU *pdu, P9MsgHeader *hdr)
>>   int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
>>                                  Error **errp)
>>   {
>> +    ERRP_AUTO_PROPAGATE();
> 
> This is correct since this function calls error_prepend() but I think
> errp is never &error_fatal or &error_abort on the realize path. Anyway,
> better safe than sorry.
> 
>>       int i, len;
>>       struct stat stat;
>>       FsDriverEntry *fse;
> 
> Please drop this patch from your series and I'll do the change once
> ERRP_AUTO_PROPAGATE() gets merged.
> 
> Great thanks for your time in finding a clever way to deal with error
> propagation. :)
> 

Hmm, actual question is: how many other errp IN parameters I've broken in
these series?

I can't simply drop one patch from atomatically genereated series: how people
will backport it than, or understand how to backport other crossing things?

Actually, for the cases you mean, ERRP_AUTO_PROPAGATE is no-op, as it uses original
errp if it's not NULL and don't point to error_fatal.

So, nothing is broken here, may be we can proceed as is? But I agree, it looks strange.

Eric, what do you think?

I tried to change cocci script to

   @rule0@
   // Add invocation to errp-functions
~ identifier fn, fn2;
   @@

    fn(..., Error **errp, ...)
    {
   +   ERRP_AUTO_PROPAGATE();
       <+...
+     fn2(..., errp, ...)
+     ...
   (
       error_append_hint(errp, ...);
   |
       error_prepend(errp, ...);
   )
       ...+>
    }


but it stubs on hw/vfio/pci.c... and it skips a lot of valid cases, so it's wrong.



-- 
Best regards,
Vladimir

Re: [PATCH v4 21/31] virtio-9p: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

02.10.2019 15:58, Vladimir Sementsov-Ogievskiy wrote:
> 02.10.2019 12:19, Greg Kurz wrote:
>> On Tue,  1 Oct 2019 18:53:09 +0300
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>
>>> 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.
>>>
>>
>> Well... this patch doesn't really fix anything because...
> 
> I'm sure it fixes. But it also breaks some things you mention below..
> 
>>
>>> This commit (together with its neighbors) was generated by
>>>
>>> git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
>>> spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
>>> --in-place $f; done
>>>
>>> and then
>>>
>>> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
>>>
>>> (auto-msg was a file with this commit message)
>>>
>>> and then by hand, for not maintained changed files:
>>>
>>> git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>
>>>
>>> Still, for backporting it may be more comfortable to use only the first
>>> command and then do one huge commit.
>>>
>>> Reported-by: Greg Kurz <groug@kaod.org>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   hw/9pfs/9p-local.c | 1 +
>>>   hw/9pfs/9p-proxy.c | 1 +
>>>   hw/9pfs/9p.c       | 1 +
>>>   3 files changed, 3 insertions(+)
>>>
>>> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
>>> index 08e673a79c..fccbf758bd 100644
>>> --- a/hw/9pfs/9p-local.c
>>> +++ b/hw/9pfs/9p-local.c
>>> @@ -1471,6 +1471,7 @@ static void local_cleanup(FsContext *ctx)
>>>   static void error_append_security_model_hint(Error **errp)
>>>   {
>>> +    ERRP_AUTO_PROPAGATE();
>>>       error_append_hint(errp, "Valid options are: security_model="
>>>                         "[passthrough|mapped-xattr|mapped-file|none]\n");
>>>   }
>>
>> This function doesn't need auto propagation in the first place. It is
>> simply a wrapper around error_append_hint(). ERRP_AUTO_PROPAGATE()
>> should go to the caller local_parse_opts().
> 
> Hmm, that's bad. So, actually it's one more errp IN parameter.
> 
>>
>> Also some extra care is needed there to handle part [3.] of the
>> cleanup. I understand this is out of the scope of that series,
>> but I'd rather see all of this fixed in the same patch.
>>
>>> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
>>> index 57a8c1c808..9291c8efa2 100644
>>> --- a/hw/9pfs/9p-proxy.c
>>> +++ b/hw/9pfs/9p-proxy.c
>>> @@ -1116,6 +1116,7 @@ static int connect_namedsocket(const char *path, Error **errp)
>>>   static void error_append_socket_sockfd_hint(Error **errp)
>>>   {
>>> +    ERRP_AUTO_PROPAGATE();
>>>       error_append_hint(errp, "Either specify socket=/some/path where /some/path"
>>>                         " points to a listening AF_UNIX socket or sock_fd=fd"
>>>                         " where fd is a file descriptor to a connected AF_UNIX"
>>
>> Same here. ERRP_AUTO_PROPAGATE() should go to proxy_parse_opts().
> 
> and this one.
> 
>>
>>> diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c
>>> index cce2366219..1df2749e03 100644
>>> --- a/hw/9pfs/9p.c
>>> +++ b/hw/9pfs/9p.c
>>> @@ -3552,6 +3552,7 @@ void pdu_submit(V9fsPDU *pdu, P9MsgHeader *hdr)
>>>   int v9fs_device_realize_common(V9fsState *s, const V9fsTransport *t,
>>>                                  Error **errp)
>>>   {
>>> +    ERRP_AUTO_PROPAGATE();
>>
>> This is correct since this function calls error_prepend() but I think
>> errp is never &error_fatal or &error_abort on the realize path. Anyway,
>> better safe than sorry.
>>
>>>       int i, len;
>>>       struct stat stat;
>>>       FsDriverEntry *fse;
>>
>> Please drop this patch from your series and I'll do the change once
>> ERRP_AUTO_PROPAGATE() gets merged.
>>
>> Great thanks for your time in finding a clever way to deal with error
>> propagation. :)
>>
> 
> Hmm, actual question is: how many other errp IN parameters I've broken in
> these series?
> 
> I can't simply drop one patch from atomatically genereated series: how people
> will backport it than, or understand how to backport other crossing things?
> 
> Actually, for the cases you mean, ERRP_AUTO_PROPAGATE is no-op, as it uses original
> errp if it's not NULL and don't point to error_fatal.
> 
> So, nothing is broken here, may be we can proceed as is? But I agree, it looks strange.
> 
> Eric, what do you think?
> 
> I tried to change cocci script to
> 
>    @rule0@
>    // Add invocation to errp-functions
> ~ identifier fn, fn2;
>    @@
> 
>     fn(..., Error **errp, ...)
>     {
>    +   ERRP_AUTO_PROPAGATE();
>        <+...
> +     fn2(..., errp, ...)
> +     ...
>    (
>        error_append_hint(errp, ...);
>    |
>        error_prepend(errp, ...);
>    )
>        ...+>
>     }
> 
> 
> but it stubs on hw/vfio/pci.c... and it skips a lot of valid cases, so it's wrong.
> 
> 
> 

Eric, you are the most interested maintainer, may be, I'll make a v5 with only nbd updated?
Then Greg will update his subsystem, then may be I or someone else will update block.. I'm
afraid, nobody will review the whole series, and Greg shows, that there maybe some unpredicted
effects.

-- 
Best regards,
Vladimir

Re: [PATCH v4 04/31] error: auto propagated local_err

[Eric Blake]

On 10/2/19 5:15 AM, Roman Kagan wrote:
> On Tue, Oct 01, 2019 at 06:52:52PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
>> functions with errp OUT parameter.
>>
>> It has three goals:
>>
>> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
>> can't see this additional information, because exit() happens in
>> error_setg earlier than information is added. [Reported by Greg Kurz]
>>

>> +/*
>> + * ERRP_AUTO_PROPAGATE
>> + *

>> +#define ERRP_AUTO_PROPAGATE() \
>> +g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
>> +errp = ((errp == NULL || *errp == error_fatal) ? \
>> +    &__auto_errp_prop.local_err : errp)
>> +
> 
> I guess it has been discussed but I couldn't find it, so: what's the
> reason for casting in stone the name of the function parameter, which
> isn't quite so obvious when you see this macro used in the code?  IMO
> if the macro took errp as a parameter that would be easier to follow.

It was discussed.  Forcing a specific name of the 'Error **errp' has 
tradeoffs:

Pro: possible to write a sed script to check for missing spots in the 
macros (in fact, my sed script found spots missed by Coccinelle when not 
using the correct --macro-header option)
Pro: enforces uniform style
Con: misses instances that have typos or otherwise used the wrong name

Allowing the macro to take the name of the parameter:
Pro/Con: More flexible (allows use in more place, but loses consistency)
Pro: Coccinelle can still handle the conversion
Con: using sed to check if Coccinelle missed anything is harder

But opinions on how to paint the bikeshed are still welcome; it's easy 
enough to respin the series with the macro taking an argument if that 
is agreed to indeed be more legible.

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

Re: [PATCH v4 06/31] python: add commit-per-subsystem.py

[Cornelia Huck]

On Tue,  1 Oct 2019 18:52:54 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> Add script to automatically commit tree-wide changes per-subsystem.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> CC: kwolf@redhat.com
> CC: mreitz@redhat.com
> CC: jsnow@redhat.com
> CC: fam@euphon.net
> CC: sw@weilnetz.de
> CC: codyprime@gmail.com
> CC: marcandre.lureau@redhat.com
> CC: pbonzini@redhat.com
> CC: groug@kaod.org
> CC: sundeep.lkml@gmail.com
> CC: peter.maydell@linaro.org
> CC: stefanha@redhat.com
> CC: pburton@wavecomp.com
> CC: arikalo@wavecomp.com
> CC: berrange@redhat.com
> CC: ehabkost@redhat.com
> CC: david@gibson.dropbear.id.au
> CC: clg@kaod.org
> CC: mst@redhat.com
> CC: marcel.apfelbaum@gmail.com
> CC: mark.cave-ayland@ilande.co.uk
> CC: yuval.shaia@oracle.com
> CC: cohuck@redhat.com
> CC: farman@linux.ibm.com
> CC: rth@twiddle.net
> CC: david@redhat.com
> CC: pasic@linux.ibm.com
> CC: borntraeger@de.ibm.com
> CC: kraxel@redhat.com
> CC: alex.williamson@redhat.com
> CC: andrew@aj.id.au
> CC: joel@jms.id.au
> CC: eblake@redhat.com
> CC: armbru@redhat.com
> CC: mdroth@linux.vnet.ibm.com
> CC: quintela@redhat.com
> CC: dgilbert@redhat.com
> CC: jasowang@redhat.com
> CC: qemu-block@nongnu.org
> CC: integration@gluster.org
> CC: qemu-arm@nongnu.org
> CC: qemu-ppc@nongnu.org
> CC: qemu-s390x@nongnu.org
> 
> 
>  python/commit-per-subsystem.py | 69 ++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100755 python/commit-per-subsystem.py
> 
> diff --git a/python/commit-per-subsystem.py b/python/commit-per-subsystem.py
> new file mode 100755
> index 0000000000..d8442d9ea3
> --- /dev/null
> +++ b/python/commit-per-subsystem.py
> @@ -0,0 +1,69 @@
> +#!/usr/bin/env python3
> +#
> +# Copyright (c) 2019 Virtuozzo International GmbH
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import subprocess
> +import sys
> +
> +
> +def git_add(pattern):
> +    subprocess.run(['git', 'add', pattern])
> +
> +
> +def git_commit(msg):
> +    subprocess.run(['git', 'commit', '-m', msg], capture_output=True)
> +
> +
> +maintainers = sys.argv[1]
> +message = sys.argv[2].strip()
> +
> +subsystem = None
> +
> +shortnames = {
> +    'Block layer core': 'block',
> +    'ARM cores': 'arm',
> +    'Network Block Device (NBD)': 'nbd',
> +    'Command line option argument parsing': 'cmdline',
> +    'Character device backends': 'chardev',
> +    'S390 general architecture support': 's390'
> +}
> +
> +
> +def commit():
> +    if subsystem:
> +        msg = subsystem
> +        if msg in shortnames:
> +            msg = shortnames[msg]
> +        msg += ': ' + message
> +        git_commit(msg)
> +
> +
> +with open(maintainers) as f:
> +    for line in f:
> +        line = line.rstrip()
> +        if not line:
> +            continue
> +        if len(line) >= 2 and line[1] == ':':
> +            if line[0] == 'F' and line[3:] not in ['*', '*/']:
> +                git_add(line[3:])
> +        else:
> +            # new subsystem start
> +            commit()
> +
> +            subsystem = line
> +
> +commit()

Hm... I'm not sure about the purpose of this script (and my python is
rather weak)... is this supposed to collect all changes covered by a
subsystem F: pattern into one patch? If so, what happens to files
covered by multiple sections?

Re: [PATCH v4 07/31] s390: Fix error_append_hint/error_prepend usage

[Cornelia Huck]

On Tue,  1 Oct 2019 18:52:55 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 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.
> 
> This commit (together with its neighbors) was generated by
> 
> git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
> spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
> --in-place $f; done
> 
> and then
> 
> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
> 
> (auto-msg was a file with this commit message)
> 
> and then by hand, for not maintained changed files:
> 
> git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>
> 
> Still, for backporting it may be more comfortable to use only the first
> command and then do one huge commit.
> 
> Reported-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  hw/s390x/s390-ccw.c       | 1 +
>  target/s390x/cpu_models.c | 2 ++
>  2 files changed, 3 insertions(+)

Acked-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v4 06/31] python: add commit-per-subsystem.py

[Vladimir Sementsov-Ogievskiy]

07.10.2019 18:55, Cornelia Huck wrote:
> On Tue,  1 Oct 2019 18:52:54 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> Add script to automatically commit tree-wide changes per-subsystem.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>
>> CC: kwolf@redhat.com
>> CC: mreitz@redhat.com
>> CC: jsnow@redhat.com
>> CC: fam@euphon.net
>> CC: sw@weilnetz.de
>> CC: codyprime@gmail.com
>> CC: marcandre.lureau@redhat.com
>> CC: pbonzini@redhat.com
>> CC: groug@kaod.org
>> CC: sundeep.lkml@gmail.com
>> CC: peter.maydell@linaro.org
>> CC: stefanha@redhat.com
>> CC: pburton@wavecomp.com
>> CC: arikalo@wavecomp.com
>> CC: berrange@redhat.com
>> CC: ehabkost@redhat.com
>> CC: david@gibson.dropbear.id.au
>> CC: clg@kaod.org
>> CC: mst@redhat.com
>> CC: marcel.apfelbaum@gmail.com
>> CC: mark.cave-ayland@ilande.co.uk
>> CC: yuval.shaia@oracle.com
>> CC: cohuck@redhat.com
>> CC: farman@linux.ibm.com
>> CC: rth@twiddle.net
>> CC: david@redhat.com
>> CC: pasic@linux.ibm.com
>> CC: borntraeger@de.ibm.com
>> CC: kraxel@redhat.com
>> CC: alex.williamson@redhat.com
>> CC: andrew@aj.id.au
>> CC: joel@jms.id.au
>> CC: eblake@redhat.com
>> CC: armbru@redhat.com
>> CC: mdroth@linux.vnet.ibm.com
>> CC: quintela@redhat.com
>> CC: dgilbert@redhat.com
>> CC: jasowang@redhat.com
>> CC: qemu-block@nongnu.org
>> CC: integration@gluster.org
>> CC: qemu-arm@nongnu.org
>> CC: qemu-ppc@nongnu.org
>> CC: qemu-s390x@nongnu.org
>>
>>
>>   python/commit-per-subsystem.py | 69 ++++++++++++++++++++++++++++++++++
>>   1 file changed, 69 insertions(+)
>>   create mode 100755 python/commit-per-subsystem.py
>>
>> diff --git a/python/commit-per-subsystem.py b/python/commit-per-subsystem.py
>> new file mode 100755
>> index 0000000000..d8442d9ea3
>> --- /dev/null
>> +++ b/python/commit-per-subsystem.py
>> @@ -0,0 +1,69 @@
>> +#!/usr/bin/env python3
>> +#
>> +# Copyright (c) 2019 Virtuozzo International GmbH
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +
>> +import subprocess
>> +import sys
>> +
>> +
>> +def git_add(pattern):
>> +    subprocess.run(['git', 'add', pattern])
>> +
>> +
>> +def git_commit(msg):
>> +    subprocess.run(['git', 'commit', '-m', msg], capture_output=True)
>> +
>> +
>> +maintainers = sys.argv[1]
>> +message = sys.argv[2].strip()
>> +
>> +subsystem = None
>> +
>> +shortnames = {
>> +    'Block layer core': 'block',
>> +    'ARM cores': 'arm',
>> +    'Network Block Device (NBD)': 'nbd',
>> +    'Command line option argument parsing': 'cmdline',
>> +    'Character device backends': 'chardev',
>> +    'S390 general architecture support': 's390'
>> +}
>> +
>> +
>> +def commit():
>> +    if subsystem:
>> +        msg = subsystem
>> +        if msg in shortnames:
>> +            msg = shortnames[msg]
>> +        msg += ': ' + message
>> +        git_commit(msg)
>> +
>> +
>> +with open(maintainers) as f:
>> +    for line in f:
>> +        line = line.rstrip()
>> +        if not line:
>> +            continue
>> +        if len(line) >= 2 and line[1] == ':':
>> +            if line[0] == 'F' and line[3:] not in ['*', '*/']:
>> +                git_add(line[3:])
>> +        else:
>> +            # new subsystem start
>> +            commit()
>> +
>> +            subsystem = line
>> +
>> +commit()
> 
> Hm... I'm not sure about the purpose of this script (and my python is
> rather weak)... is this supposed to collect all changes covered by a
> subsystem F: pattern into one patch?

Yes

> If so, what happens to files
> covered by multiple sections?
> 

Hmm, they just go to the first of these sections, mentioned in MAINTAINERS.
Is it bad I don't know, but I tried to automate it somehow. Anyway, I myself
can't have better idea about how to organize patches to the subsystems which
I don't know.

-- 
Best regards,
Vladimir

Re: [PATCH v4 06/31] python: add commit-per-subsystem.py

[Cornelia Huck]

On Mon, 7 Oct 2019 16:10:02 +0000
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 07.10.2019 18:55, Cornelia Huck wrote:
> > On Tue,  1 Oct 2019 18:52:54 +0300
> > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> >> +def git_add(pattern):
> >> +    subprocess.run(['git', 'add', pattern])
> >> +
> >> +
> >> +def git_commit(msg):
> >> +    subprocess.run(['git', 'commit', '-m', msg], capture_output=True)
> >> +
> >> +
> >> +maintainers = sys.argv[1]
> >> +message = sys.argv[2].strip()
> >> +
> >> +subsystem = None
> >> +
> >> +shortnames = {
> >> +    'Block layer core': 'block',
> >> +    'ARM cores': 'arm',
> >> +    'Network Block Device (NBD)': 'nbd',
> >> +    'Command line option argument parsing': 'cmdline',
> >> +    'Character device backends': 'chardev',
> >> +    'S390 general architecture support': 's390'
> >> +}
> >> +
> >> +
> >> +def commit():
> >> +    if subsystem:
> >> +        msg = subsystem
> >> +        if msg in shortnames:
> >> +            msg = shortnames[msg]
> >> +        msg += ': ' + message
> >> +        git_commit(msg)
> >> +
> >> +
> >> +with open(maintainers) as f:
> >> +    for line in f:
> >> +        line = line.rstrip()
> >> +        if not line:
> >> +            continue
> >> +        if len(line) >= 2 and line[1] == ':':
> >> +            if line[0] == 'F' and line[3:] not in ['*', '*/']:
> >> +                git_add(line[3:])
> >> +        else:
> >> +            # new subsystem start
> >> +            commit()
> >> +
> >> +            subsystem = line
> >> +
> >> +commit()  
> > 
> > Hm... I'm not sure about the purpose of this script (and my python is
> > rather weak)... is this supposed to collect all changes covered by a
> > subsystem F: pattern into one patch?  
> 
> Yes
> 
> > If so, what happens to files
> > covered by multiple sections?
> >   
> 
> Hmm, they just go to the first of these sections, mentioned in MAINTAINERS.
> Is it bad I don't know, but I tried to automate it somehow. Anyway, I myself
> can't have better idea about how to organize patches to the subsystems which
> I don't know.
> 

Yeah, that is a problem I don't have a solution for, either.

But the script should probably get at least a comment about its
intended purpose and limitations? We don't really want people to start
using it blindly.

Re: [PATCH v4 06/31] python: add commit-per-subsystem.py

[Daniel P. Berrangé]

On Mon, Oct 07, 2019 at 06:16:51PM +0200, Cornelia Huck wrote:
> On Mon, 7 Oct 2019 16:10:02 +0000
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
> > 07.10.2019 18:55, Cornelia Huck wrote:
> > > On Tue,  1 Oct 2019 18:52:54 +0300
> > > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
> > >> +def git_add(pattern):
> > >> +    subprocess.run(['git', 'add', pattern])
> > >> +
> > >> +
> > >> +def git_commit(msg):
> > >> +    subprocess.run(['git', 'commit', '-m', msg], capture_output=True)
> > >> +
> > >> +
> > >> +maintainers = sys.argv[1]
> > >> +message = sys.argv[2].strip()
> > >> +
> > >> +subsystem = None
> > >> +
> > >> +shortnames = {
> > >> +    'Block layer core': 'block',
> > >> +    'ARM cores': 'arm',
> > >> +    'Network Block Device (NBD)': 'nbd',
> > >> +    'Command line option argument parsing': 'cmdline',
> > >> +    'Character device backends': 'chardev',
> > >> +    'S390 general architecture support': 's390'
> > >> +}
> > >> +
> > >> +
> > >> +def commit():
> > >> +    if subsystem:
> > >> +        msg = subsystem
> > >> +        if msg in shortnames:
> > >> +            msg = shortnames[msg]
> > >> +        msg += ': ' + message
> > >> +        git_commit(msg)
> > >> +
> > >> +
> > >> +with open(maintainers) as f:
> > >> +    for line in f:
> > >> +        line = line.rstrip()
> > >> +        if not line:
> > >> +            continue
> > >> +        if len(line) >= 2 and line[1] == ':':
> > >> +            if line[0] == 'F' and line[3:] not in ['*', '*/']:
> > >> +                git_add(line[3:])
> > >> +        else:
> > >> +            # new subsystem start
> > >> +            commit()
> > >> +
> > >> +            subsystem = line
> > >> +
> > >> +commit()  
> > > 
> > > Hm... I'm not sure about the purpose of this script (and my python is
> > > rather weak)... is this supposed to collect all changes covered by a
> > > subsystem F: pattern into one patch?  
> > 
> > Yes
> > 
> > > If so, what happens to files
> > > covered by multiple sections?
> > >   
> > 
> > Hmm, they just go to the first of these sections, mentioned in MAINTAINERS.
> > Is it bad I don't know, but I tried to automate it somehow. Anyway, I myself
> > can't have better idea about how to organize patches to the subsystems which
> > I don't know.
> > 
> 
> Yeah, that is a problem I don't have a solution for, either.
> 
> But the script should probably get at least a comment about its
> intended purpose and limitations? We don't really want people to start
> using it blindly.

Is this really a common enough problem to even justify having the
script to start with ?

It looks like its only really usable in the case where the changes
to each subsystem are totally self-contained, otherwise you'll get
git bisect failures. The user still has to go back and edit each
commit here to fill in a useful commit messages. It doesn't seem
to save much effort over 'git add -u sub/dir/ && git commit -s'
which is what I'd typically do for grouping changes that are
spread across the tree.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH v4 06/31] python: add commit-per-subsystem.py

[Vladimir Sementsov-Ogievskiy]

07.10.2019 19:21, Daniel P. Berrangé wrote:
> On Mon, Oct 07, 2019 at 06:16:51PM +0200, Cornelia Huck wrote:
>> On Mon, 7 Oct 2019 16:10:02 +0000
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>
>>> 07.10.2019 18:55, Cornelia Huck wrote:
>>>> On Tue,  1 Oct 2019 18:52:54 +0300
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>
>>>>> +def git_add(pattern):
>>>>> +    subprocess.run(['git', 'add', pattern])
>>>>> +
>>>>> +
>>>>> +def git_commit(msg):
>>>>> +    subprocess.run(['git', 'commit', '-m', msg], capture_output=True)
>>>>> +
>>>>> +
>>>>> +maintainers = sys.argv[1]
>>>>> +message = sys.argv[2].strip()
>>>>> +
>>>>> +subsystem = None
>>>>> +
>>>>> +shortnames = {
>>>>> +    'Block layer core': 'block',
>>>>> +    'ARM cores': 'arm',
>>>>> +    'Network Block Device (NBD)': 'nbd',
>>>>> +    'Command line option argument parsing': 'cmdline',
>>>>> +    'Character device backends': 'chardev',
>>>>> +    'S390 general architecture support': 's390'
>>>>> +}
>>>>> +
>>>>> +
>>>>> +def commit():
>>>>> +    if subsystem:
>>>>> +        msg = subsystem
>>>>> +        if msg in shortnames:
>>>>> +            msg = shortnames[msg]
>>>>> +        msg += ': ' + message
>>>>> +        git_commit(msg)
>>>>> +
>>>>> +
>>>>> +with open(maintainers) as f:
>>>>> +    for line in f:
>>>>> +        line = line.rstrip()
>>>>> +        if not line:
>>>>> +            continue
>>>>> +        if len(line) >= 2 and line[1] == ':':
>>>>> +            if line[0] == 'F' and line[3:] not in ['*', '*/']:
>>>>> +                git_add(line[3:])
>>>>> +        else:
>>>>> +            # new subsystem start
>>>>> +            commit()
>>>>> +
>>>>> +            subsystem = line
>>>>> +
>>>>> +commit()
>>>>
>>>> Hm... I'm not sure about the purpose of this script (and my python is
>>>> rather weak)... is this supposed to collect all changes covered by a
>>>> subsystem F: pattern into one patch?
>>>
>>> Yes
>>>
>>>> If so, what happens to files
>>>> covered by multiple sections?
>>>>    
>>>
>>> Hmm, they just go to the first of these sections, mentioned in MAINTAINERS.
>>> Is it bad I don't know, but I tried to automate it somehow. Anyway, I myself
>>> can't have better idea about how to organize patches to the subsystems which
>>> I don't know.
>>>
>>
>> Yeah, that is a problem I don't have a solution for, either.
>>
>> But the script should probably get at least a comment about its
>> intended purpose and limitations? We don't really want people to start
>> using it blindly.
> 
> Is this really a common enough problem to even justify having the
> script to start with ?
> 
> It looks like its only really usable in the case where the changes
> to each subsystem are totally self-contained, otherwise you'll get
> git bisect failures. The user still has to go back and edit each
> commit here to fill in a useful commit messages. It doesn't seem
> to save much effort over 'git add -u sub/dir/ && git commit -s'
> which is what I'd typically do for grouping changes that are
> spread across the tree.
> 


Commit message is the same except "subsystem: ", I don't think it shoud
be edited.
Still, we can just not commit this script to Qemu if we don't like it. It just
helped me to create patches, and we can't take them without a review anyway.


-- 
Best regards,
Vladimir

Re: [PATCH v4 00/31] error: auto propagated local_err

[Markus Armbruster]

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

> Hi all!
>
> Here is a proposal of auto propagation for local_err, to not call
> error_propagate on every exit point, when we deal with local_err.
>
> There are also two issues with errp:
>
> 1. error_fatal & error_append_hint/error_prepend: user can't see this
> additional info, because exit() happens in error_setg earlier than info
> is added. [Reported by Greg Kurz]

How is this series related to Greg's "[PATCH 00/17] Fix usage of
error_append_hint()"?  Do we need both?

> 2. error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself don't fix the issue, but it allows to [3.] drop all
> local_err+error_propagate pattern, which will definitely fix the issue)
> [Reported by Kevin Wolf]
>
> Still, applying new macro to all errp-functions is a huge task, which is
> impossible to solve in one series.
>
> So, here is a minimum: solve only [1.], by adding new macro to all
> errp-functions which wants to call error_append_hint.
>
> v4;
> 02: - check errp to be not NULL
>     - drop Eric's r-b
> 03: add Eric's r-b
> 04: - rename macro to ERRP_AUTO_PROPAGATE [Kevin]
>     - improve comment and commit msg, mention
>       error_prepend
> 05: - handle error_prepend too
>     - use new macro name
>     - drop empty line at the end
>
> commit message for auto-generated commits updated,
> commits regenerated.
>
> I'll use cc-cmd to cc appropriate recipients per patch, still
> cover-letter together with 04-06 patches should be interesting for
> all:
[...]

Big series; let me guess its structure.

> Vladimir Sementsov-Ogievskiy (31):
>   errp: rename errp to errp_in where it is IN-argument
>   hw/core/loader-fit: fix freeing errp in fit_load_fdt
>   net/net: fix local variable shadowing in net_client_init

Preparations.

>   error: auto propagated local_err

The new idea.

>   scripts: add script to fix error_append_hint/error_prepend usage
>   python: add commit-per-subsystem.py

Scripts to help you apply it.

>   s390: Fix error_append_hint/error_prepend usage
>   ARM TCG CPUs: Fix error_append_hint/error_prepend usage
>   PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage
>   arm: Fix error_append_hint/error_prepend usage
>   SmartFusion2: Fix error_append_hint/error_prepend usage
>   ASPEED BMCs: Fix error_append_hint/error_prepend usage
>   Boston: Fix error_append_hint/error_prepend usage
>   PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage
>   PCI: Fix error_append_hint/error_prepend usage
>   SCSI: Fix error_append_hint/error_prepend usage
>   USB: Fix error_append_hint/error_prepend usage
>   VFIO: Fix error_append_hint/error_prepend usage
>   vhost: Fix error_append_hint/error_prepend usage
>   virtio: Fix error_append_hint/error_prepend usage
>   virtio-9p: Fix error_append_hint/error_prepend usage
>   XIVE: Fix error_append_hint/error_prepend usage
>   block: Fix error_append_hint/error_prepend usage
>   chardev: Fix error_append_hint/error_prepend usage
>   cmdline: Fix error_append_hint/error_prepend usage
>   QOM: Fix error_append_hint/error_prepend usage
>   Migration: Fix error_append_hint/error_prepend usage
>   Sockets: Fix error_append_hint/error_prepend usage
>   nbd: Fix error_append_hint/error_prepend usage
>   PVRDMA: Fix error_append_hint/error_prepend usage
>   ivshmem: Fix error_append_hint/error_prepend usage

Applying it.

Correct?

[...]

Re: [PATCH v4 00/31] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

08.10.2019 10:30, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Hi all!
>>
>> Here is a proposal of auto propagation for local_err, to not call
>> error_propagate on every exit point, when we deal with local_err.
>>
>> There are also two issues with errp:
>>
>> 1. error_fatal & error_append_hint/error_prepend: user can't see this
>> additional info, because exit() happens in error_setg earlier than info
>> is added. [Reported by Greg Kurz]
> 
> How is this series related to Greg's "[PATCH 00/17] Fix usage of
> error_append_hint()"?  Do we need both?

These series is a substitution for Greg's. Still, there are problems with
automation, which Greg pointed in 21/31, and I don't know what to do next.

May be, just continue to review patches and fix them by hand. May be try to
improve automation...

> 
>> 2. error_abort & error_propagate: when we wrap
>> error_abort by local_err+error_propagate, resulting coredump will
>> refer to error_propagate and not to the place where error happened.
>> (the macro itself don't fix the issue, but it allows to [3.] drop all
>> local_err+error_propagate pattern, which will definitely fix the issue)
>> [Reported by Kevin Wolf]
>>
>> Still, applying new macro to all errp-functions is a huge task, which is
>> impossible to solve in one series.
>>
>> So, here is a minimum: solve only [1.], by adding new macro to all
>> errp-functions which wants to call error_append_hint.
>>
>> v4;
>> 02: - check errp to be not NULL
>>      - drop Eric's r-b
>> 03: add Eric's r-b
>> 04: - rename macro to ERRP_AUTO_PROPAGATE [Kevin]
>>      - improve comment and commit msg, mention
>>        error_prepend
>> 05: - handle error_prepend too
>>      - use new macro name
>>      - drop empty line at the end
>>
>> commit message for auto-generated commits updated,
>> commits regenerated.
>>
>> I'll use cc-cmd to cc appropriate recipients per patch, still
>> cover-letter together with 04-06 patches should be interesting for
>> all:
> [...]
> 
> Big series; let me guess its structure.
> 
>> Vladimir Sementsov-Ogievskiy (31):
>>    errp: rename errp to errp_in where it is IN-argument
>>    hw/core/loader-fit: fix freeing errp in fit_load_fdt
>>    net/net: fix local variable shadowing in net_client_init
> 
> Preparations.
> 
>>    error: auto propagated local_err
> 
> The new idea.
> 
>>    scripts: add script to fix error_append_hint/error_prepend usage
>>    python: add commit-per-subsystem.py
> 
> Scripts to help you apply it.
> 
>>    s390: Fix error_append_hint/error_prepend usage
>>    ARM TCG CPUs: Fix error_append_hint/error_prepend usage
>>    PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage
>>    arm: Fix error_append_hint/error_prepend usage
>>    SmartFusion2: Fix error_append_hint/error_prepend usage
>>    ASPEED BMCs: Fix error_append_hint/error_prepend usage
>>    Boston: Fix error_append_hint/error_prepend usage
>>    PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage
>>    PCI: Fix error_append_hint/error_prepend usage
>>    SCSI: Fix error_append_hint/error_prepend usage
>>    USB: Fix error_append_hint/error_prepend usage
>>    VFIO: Fix error_append_hint/error_prepend usage
>>    vhost: Fix error_append_hint/error_prepend usage
>>    virtio: Fix error_append_hint/error_prepend usage
>>    virtio-9p: Fix error_append_hint/error_prepend usage
>>    XIVE: Fix error_append_hint/error_prepend usage
>>    block: Fix error_append_hint/error_prepend usage
>>    chardev: Fix error_append_hint/error_prepend usage
>>    cmdline: Fix error_append_hint/error_prepend usage
>>    QOM: Fix error_append_hint/error_prepend usage
>>    Migration: Fix error_append_hint/error_prepend usage
>>    Sockets: Fix error_append_hint/error_prepend usage
>>    nbd: Fix error_append_hint/error_prepend usage
>>    PVRDMA: Fix error_append_hint/error_prepend usage
>>    ivshmem: Fix error_append_hint/error_prepend usage
> 
> Applying it.
> 
> Correct?
> 

Yes


-- 
Best regards,
Vladimir

Re: [PATCH v4 01/31] errp: rename errp to errp_in where it is IN-argument

[Markus Armbruster]

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

> Error **errp is almost always OUT-argument: it's assumed to be NULL, or
> pointer to NULL-initialized pointer, or pointer to error_abort or
> error_fatal, for callee to report error.

Yes.

> But very few functions instead get Error **errp as IN-argument:
> it's assumed to be set, and callee should clean it.

What do you mean by "callee should clean"?  Let's see below.

> In such cases, rename errp to errp_in.

I acknowledge that errp arguments that don't have the usual meaning can
be confusing.

Naming can help, comments can help, but perhaps we can tweak the code to
avoid the problem instead.  Let's see:

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  include/monitor/hmp.h |  2 +-
>  include/qapi/error.h  |  2 +-
>  ui/vnc.h              |  2 +-
>  monitor/hmp-cmds.c    |  8 ++++----
>  ui/vnc.c              | 10 +++++-----
>  util/error.c          |  8 ++++----
>  6 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
> index a0e9511440..f929814f1a 100644
> --- a/include/monitor/hmp.h
> +++ b/include/monitor/hmp.h
> @@ -16,7 +16,7 @@
>  
>  #include "qemu/readline.h"
>  
> -void hmp_handle_error(Monitor *mon, Error **errp);
> +void hmp_handle_error(Monitor *mon, Error **errp_in);
>  
>  void hmp_info_name(Monitor *mon, const QDict *qdict);
>  void hmp_info_version(Monitor *mon, const QDict *qdict);
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 3f95141a01..9376f59c35 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -283,7 +283,7 @@ void error_free(Error *err);
>  /*
>   * Convenience function to assert that *@errp is set, then silently free it.
>   */
> -void error_free_or_abort(Error **errp);
> +void error_free_or_abort(Error **errp_in);
>  
>  /*
>   * Convenience function to warn_report() and free @err.
> diff --git a/ui/vnc.h b/ui/vnc.h
> index fea79c2fc9..00e0b48f2f 100644
> --- a/ui/vnc.h
> +++ b/ui/vnc.h
> @@ -547,7 +547,7 @@ uint32_t read_u32(uint8_t *data, size_t offset);
>  
>  /* Protocol stage functions */
>  void vnc_client_error(VncState *vs);
> -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp);
> +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in);
>  
>  void start_client_init(VncState *vs);
>  void start_auth_vnc(VncState *vs);
> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
> index b2551c16d1..941d5d0a45 100644
> --- a/monitor/hmp-cmds.c
> +++ b/monitor/hmp-cmds.c
> @@ -60,11 +60,11 @@
>  #include <spice/enums.h>
>  #endif
>  
> -void hmp_handle_error(Monitor *mon, Error **errp)
> +void hmp_handle_error(Monitor *mon, Error **errp_in)
>  {
> -    assert(errp);
> -    if (*errp) {
> -        error_reportf_err(*errp, "Error: ");
> +    assert(errp_in);
> +    if (*errp_in) {
> +        error_reportf_err(*errp_in, "Error: ");
>      }
>  }

This functions frees the error.  It leaves nothing for the caller to
clean up.

All callers pass &ERR, where ERR is a local variable.  Perhaps a more
robust way to signal "@errp is not the usual out-argument" would be
peeling off an indirection: pass ERR, drop the assertion.

>  
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 87b8045afe..9d6384d9b1 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -1312,7 +1312,7 @@ void vnc_disconnect_finish(VncState *vs)
>      g_free(vs);
>  }
>  
> -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
> +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in)
>  {
>      if (ret <= 0) {
>          if (ret == 0) {
> @@ -1320,14 +1320,14 @@ size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
>              vnc_disconnect_start(vs);
>          } else if (ret != QIO_CHANNEL_ERR_BLOCK) {
>              trace_vnc_client_io_error(vs, vs->ioc,
> -                                      errp ? error_get_pretty(*errp) :
> +                                      errp_in ? error_get_pretty(*errp_in) :
>                                        "Unknown");
>              vnc_disconnect_start(vs);
>          }
>  
> -        if (errp) {
> -            error_free(*errp);
> -            *errp = NULL;
> +        if (errp_in) {
> +            error_free(*errp_in);
> +            *errp_in = NULL;
>          }
>          return 0;
>      }

This function isn't trivial, and lacks a contract, so let's figure out
what it does and how it's used.

@ret can be:

* Zero

  Trace EOF, call vnc_disconnect_start(), free the error, return zero.

  Aside: freeing the error without looking at it feels odd.  Can this
  happen?

* Negative other than QIO_CHANNEL_ERR_BLOCK

  Trace the error if any, else "Unknown" error, call
  vnc_disconnect_start(), free the error if any, return zero.

  Note that we can't have errp && !*errp here, or else tracing crashes
  in error_get_pretty().

* QIO_CHANNEL_ERR_BLOCK

  Free the error, return zero

* Positive

  Do nothing, return @ret

Callers pass one of the following:

* ret = -1 and errp = NULL

  This uses case "Negative other than QIO_CHANNEL_ERR_BLOCK".  Since
  error is null, it traces an "Unknown" error.

* ret and &err, where ret = FUN(..., &err), and FUN is
  qio_channel_read() or qio_channel_write().

  qio_channel_read(), _write() are documented to return non-negative on
  success, QIO_CHANNEL_ERR_BLOCK on "would block", and -1 on other
  error.  By convention, they set an error exactly when they fail,
  i.e. when they return a negative value.

  When qio_channel_read() / _write() succeed, we use case "Positive" or
  "Zero".  We don't free the error, which is fine, as none was returned.
  Aside: I *guess* the channel is non-blocking, and "zero" can happen
  only when read hits EOF.

  When qio_channel_read() / _write() fail, we use one of the error
  cases.

Looks like vnc_client_io_error() takes an error code @ret and an
optional error object in @errp with additional details.  If @ret is
non-negative, @errp must be null or point to null.  If @ret is negative,
@errp must be null or point to non-null.

vnc_client_io_error() frees the error.  It leaves nothing for the caller
to clean up.

I think we can again peel off an indirection.  The two kinds of calls
become:

* ret = -1 and err = NULL

  No textual change, but the NULL gets converted to Error * instead of
  Error **.

* ret and err

  Pass the (possibly null) error object instead of a pointer to the
  local variable.

> diff --git a/util/error.c b/util/error.c
> index d4532ce318..b3ff3832d6 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -271,11 +271,11 @@ void error_free(Error *err)
>      }
>  }
>  
> -void error_free_or_abort(Error **errp)
> +void error_free_or_abort(Error **errp_in)
>  {
> -    assert(errp && *errp);
> -    error_free(*errp);
> -    *errp = NULL;
> +    assert(errp_in && *errp_in);
> +    error_free(*errp_in);
> +    *errp_in = NULL;
>  }
>  
>  void error_propagate(Error **dst_errp, Error *local_err)

This functions frees the error.  It leaves nothing for the caller to
clean up.

All callers pass &ERR, where ERR is a local variable.  We can peel off
an indirection.


I figure your commit message's "But very few functions instead get Error
**errp as IN-argument: it's assumed to be set, and callee should clean
it" is to be read as "a few functions take Error **errp as IN-argument,
and free it".

You found three instances of confusing Error **errp.  How?  I'm asking
because I wonder whether there are more.

We can avoid the confusing Error **errp in all three cases by peeling
off an indirection.  What do you think?

Re: [PATCH v4 01/31] errp: rename errp to errp_in where it is IN-argument

[Vladimir Sementsov-Ogievskiy]

08.10.2019 12:08, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Error **errp is almost always OUT-argument: it's assumed to be NULL, or
>> pointer to NULL-initialized pointer, or pointer to error_abort or
>> error_fatal, for callee to report error.
> 
> Yes.
> 
>> But very few functions instead get Error **errp as IN-argument:
>> it's assumed to be set, and callee should clean it.
> 
> What do you mean by "callee should clean"?  Let's see below.
> 
>> In such cases, rename errp to errp_in.
> 
> I acknowledge that errp arguments that don't have the usual meaning can
> be confusing.
> 
> Naming can help, comments can help, but perhaps we can tweak the code to
> avoid the problem instead.  Let's see:
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   include/monitor/hmp.h |  2 +-
>>   include/qapi/error.h  |  2 +-
>>   ui/vnc.h              |  2 +-
>>   monitor/hmp-cmds.c    |  8 ++++----
>>   ui/vnc.c              | 10 +++++-----
>>   util/error.c          |  8 ++++----
>>   6 files changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
>> index a0e9511440..f929814f1a 100644
>> --- a/include/monitor/hmp.h
>> +++ b/include/monitor/hmp.h
>> @@ -16,7 +16,7 @@
>>   
>>   #include "qemu/readline.h"
>>   
>> -void hmp_handle_error(Monitor *mon, Error **errp);
>> +void hmp_handle_error(Monitor *mon, Error **errp_in);
>>   
>>   void hmp_info_name(Monitor *mon, const QDict *qdict);
>>   void hmp_info_version(Monitor *mon, const QDict *qdict);
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 3f95141a01..9376f59c35 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -283,7 +283,7 @@ void error_free(Error *err);
>>   /*
>>    * Convenience function to assert that *@errp is set, then silently free it.
>>    */
>> -void error_free_or_abort(Error **errp);
>> +void error_free_or_abort(Error **errp_in);
>>   
>>   /*
>>    * Convenience function to warn_report() and free @err.
>> diff --git a/ui/vnc.h b/ui/vnc.h
>> index fea79c2fc9..00e0b48f2f 100644
>> --- a/ui/vnc.h
>> +++ b/ui/vnc.h
>> @@ -547,7 +547,7 @@ uint32_t read_u32(uint8_t *data, size_t offset);
>>   
>>   /* Protocol stage functions */
>>   void vnc_client_error(VncState *vs);
>> -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp);
>> +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in);
>>   
>>   void start_client_init(VncState *vs);
>>   void start_auth_vnc(VncState *vs);
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index b2551c16d1..941d5d0a45 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -60,11 +60,11 @@
>>   #include <spice/enums.h>
>>   #endif
>>   
>> -void hmp_handle_error(Monitor *mon, Error **errp)
>> +void hmp_handle_error(Monitor *mon, Error **errp_in)
>>   {
>> -    assert(errp);
>> -    if (*errp) {
>> -        error_reportf_err(*errp, "Error: ");
>> +    assert(errp_in);
>> +    if (*errp_in) {
>> +        error_reportf_err(*errp_in, "Error: ");
>>       }
>>   }
> 
> This functions frees the error.  It leaves nothing for the caller to
> clean up.
> 
> All callers pass &ERR, where ERR is a local variable.  Perhaps a more
> robust way to signal "@errp is not the usual out-argument" would be
> peeling off an indirection: pass ERR, drop the assertion.
> 
>>   
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index 87b8045afe..9d6384d9b1 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -1312,7 +1312,7 @@ void vnc_disconnect_finish(VncState *vs)
>>       g_free(vs);
>>   }
>>   
>> -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
>> +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in)
>>   {
>>       if (ret <= 0) {
>>           if (ret == 0) {
>> @@ -1320,14 +1320,14 @@ size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
>>               vnc_disconnect_start(vs);
>>           } else if (ret != QIO_CHANNEL_ERR_BLOCK) {
>>               trace_vnc_client_io_error(vs, vs->ioc,
>> -                                      errp ? error_get_pretty(*errp) :
>> +                                      errp_in ? error_get_pretty(*errp_in) :
>>                                         "Unknown");
>>               vnc_disconnect_start(vs);
>>           }
>>   
>> -        if (errp) {
>> -            error_free(*errp);
>> -            *errp = NULL;
>> +        if (errp_in) {
>> +            error_free(*errp_in);
>> +            *errp_in = NULL;
>>           }
>>           return 0;
>>       }
> 
> This function isn't trivial, and lacks a contract, so let's figure out
> what it does and how it's used.
> 
> @ret can be:
> 
> * Zero
> 
>    Trace EOF, call vnc_disconnect_start(), free the error, return zero.
> 
>    Aside: freeing the error without looking at it feels odd.  Can this
>    happen?
> 
> * Negative other than QIO_CHANNEL_ERR_BLOCK
> 
>    Trace the error if any, else "Unknown" error, call
>    vnc_disconnect_start(), free the error if any, return zero.
> 
>    Note that we can't have errp && !*errp here, or else tracing crashes
>    in error_get_pretty().
> 
> * QIO_CHANNEL_ERR_BLOCK
> 
>    Free the error, return zero
> 
> * Positive
> 
>    Do nothing, return @ret
> 
> Callers pass one of the following:
> 
> * ret = -1 and errp = NULL
> 
>    This uses case "Negative other than QIO_CHANNEL_ERR_BLOCK".  Since
>    error is null, it traces an "Unknown" error.
> 
> * ret and &err, where ret = FUN(..., &err), and FUN is
>    qio_channel_read() or qio_channel_write().
> 
>    qio_channel_read(), _write() are documented to return non-negative on
>    success, QIO_CHANNEL_ERR_BLOCK on "would block", and -1 on other
>    error.  By convention, they set an error exactly when they fail,
>    i.e. when they return a negative value.
> 
>    When qio_channel_read() / _write() succeed, we use case "Positive" or
>    "Zero".  We don't free the error, which is fine, as none was returned.
>    Aside: I *guess* the channel is non-blocking, and "zero" can happen
>    only when read hits EOF.
> 
>    When qio_channel_read() / _write() fail, we use one of the error
>    cases.
> 
> Looks like vnc_client_io_error() takes an error code @ret and an
> optional error object in @errp with additional details.  If @ret is
> non-negative, @errp must be null or point to null.  If @ret is negative,
> @errp must be null or point to non-null.
> 
> vnc_client_io_error() frees the error.  It leaves nothing for the caller
> to clean up.
> 
> I think we can again peel off an indirection.  The two kinds of calls
> become:
> 
> * ret = -1 and err = NULL
> 
>    No textual change, but the NULL gets converted to Error * instead of
>    Error **.
> 
> * ret and err
> 
>    Pass the (possibly null) error object instead of a pointer to the
>    local variable.
> 
>> diff --git a/util/error.c b/util/error.c
>> index d4532ce318..b3ff3832d6 100644
>> --- a/util/error.c
>> +++ b/util/error.c
>> @@ -271,11 +271,11 @@ void error_free(Error *err)
>>       }
>>   }
>>   
>> -void error_free_or_abort(Error **errp)
>> +void error_free_or_abort(Error **errp_in)
>>   {
>> -    assert(errp && *errp);
>> -    error_free(*errp);
>> -    *errp = NULL;
>> +    assert(errp_in && *errp_in);
>> +    error_free(*errp_in);
>> +    *errp_in = NULL;
>>   }
>>   
>>   void error_propagate(Error **dst_errp, Error *local_err)
> 
> This functions frees the error.  It leaves nothing for the caller to
> clean up.
> 
> All callers pass &ERR, where ERR is a local variable.  We can peel off
> an indirection.
> 
> 
> I figure your commit message's "But very few functions instead get Error
> **errp as IN-argument: it's assumed to be set, and callee should clean
> it" is to be read as "a few functions take Error **errp as IN-argument,
> and free it".
> 
> You found three instances of confusing Error **errp.  How?  I'm asking
> because I wonder whether there are more.

I don' remember exactly, but assume that I found them by compilation errors
after automatic conversion.

> 
> We can avoid the confusing Error **errp in all three cases by peeling
> off an indirection.  What do you think?
> 

I like the idea, thanks! I think, I'll try to make a patch.

But you are right, unfortunately there more cases, at least, pointed by
Greg

error_append_socket_sockfd_hint and
error_append_security_model_hint

Which don't free error.. But if they append hint, they should always be called
on wrapped errp, accordingly to the problem about fatal_error, so they may
be converted to Error *err too.. Hmm, I should think about the script to find
such functions.

-- 
Best regards,
Vladimir

Re: [PATCH v4 00/31] error: auto propagated local_err

[Greg Kurz]

On Tue, 8 Oct 2019 08:41:08 +0000
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 08.10.2019 10:30, Markus Armbruster wrote:
> > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> > 
> >> Hi all!
> >>
> >> Here is a proposal of auto propagation for local_err, to not call
> >> error_propagate on every exit point, when we deal with local_err.
> >>
> >> There are also two issues with errp:
> >>
> >> 1. error_fatal & error_append_hint/error_prepend: user can't see this
> >> additional info, because exit() happens in error_setg earlier than info
> >> is added. [Reported by Greg Kurz]
> > 
> > How is this series related to Greg's "[PATCH 00/17] Fix usage of
> > error_append_hint()"?  Do we need both?
> 
> These series is a substitution for Greg's. Still, there are problems with
> automation, which Greg pointed in 21/31, and I don't know what to do next.
> 
> May be, just continue to review patches and fix them by hand. May be try to
> improve automation...
> 

The feeling I have after working on my series is that the lines that deal
with errors are mixed up with the functional code in a variety of ways.
That makes it very difficult if not impossible to come with code patterns
suitable for a 100% automated solution IMHO.

My questioning is more around the semantics of error_fatal actually. What
does passing &error_fatal gives us over passing &local_err and calling
error_report_err()+exit(), apart from breaking error_append_hint() and
error_prepend() ?

> > 
> >> 2. error_abort & error_propagate: when we wrap
> >> error_abort by local_err+error_propagate, resulting coredump will
> >> refer to error_propagate and not to the place where error happened.
> >> (the macro itself don't fix the issue, but it allows to [3.] drop all
> >> local_err+error_propagate pattern, which will definitely fix the issue)
> >> [Reported by Kevin Wolf]
> >>
> >> Still, applying new macro to all errp-functions is a huge task, which is
> >> impossible to solve in one series.
> >>
> >> So, here is a minimum: solve only [1.], by adding new macro to all
> >> errp-functions which wants to call error_append_hint.
> >>
> >> v4;
> >> 02: - check errp to be not NULL
> >>      - drop Eric's r-b
> >> 03: add Eric's r-b
> >> 04: - rename macro to ERRP_AUTO_PROPAGATE [Kevin]
> >>      - improve comment and commit msg, mention
> >>        error_prepend
> >> 05: - handle error_prepend too
> >>      - use new macro name
> >>      - drop empty line at the end
> >>
> >> commit message for auto-generated commits updated,
> >> commits regenerated.
> >>
> >> I'll use cc-cmd to cc appropriate recipients per patch, still
> >> cover-letter together with 04-06 patches should be interesting for
> >> all:
> > [...]
> > 
> > Big series; let me guess its structure.
> > 
> >> Vladimir Sementsov-Ogievskiy (31):
> >>    errp: rename errp to errp_in where it is IN-argument
> >>    hw/core/loader-fit: fix freeing errp in fit_load_fdt
> >>    net/net: fix local variable shadowing in net_client_init
> > 
> > Preparations.
> > 
> >>    error: auto propagated local_err
> > 
> > The new idea.
> > 
> >>    scripts: add script to fix error_append_hint/error_prepend usage
> >>    python: add commit-per-subsystem.py
> > 
> > Scripts to help you apply it.
> > 
> >>    s390: Fix error_append_hint/error_prepend usage
> >>    ARM TCG CPUs: Fix error_append_hint/error_prepend usage
> >>    PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage
> >>    arm: Fix error_append_hint/error_prepend usage
> >>    SmartFusion2: Fix error_append_hint/error_prepend usage
> >>    ASPEED BMCs: Fix error_append_hint/error_prepend usage
> >>    Boston: Fix error_append_hint/error_prepend usage
> >>    PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage
> >>    PCI: Fix error_append_hint/error_prepend usage
> >>    SCSI: Fix error_append_hint/error_prepend usage
> >>    USB: Fix error_append_hint/error_prepend usage
> >>    VFIO: Fix error_append_hint/error_prepend usage
> >>    vhost: Fix error_append_hint/error_prepend usage
> >>    virtio: Fix error_append_hint/error_prepend usage
> >>    virtio-9p: Fix error_append_hint/error_prepend usage
> >>    XIVE: Fix error_append_hint/error_prepend usage
> >>    block: Fix error_append_hint/error_prepend usage
> >>    chardev: Fix error_append_hint/error_prepend usage
> >>    cmdline: Fix error_append_hint/error_prepend usage
> >>    QOM: Fix error_append_hint/error_prepend usage
> >>    Migration: Fix error_append_hint/error_prepend usage
> >>    Sockets: Fix error_append_hint/error_prepend usage
> >>    nbd: Fix error_append_hint/error_prepend usage
> >>    PVRDMA: Fix error_append_hint/error_prepend usage
> >>    ivshmem: Fix error_append_hint/error_prepend usage
> > 
> > Applying it.
> > 
> > Correct?
> > 
> 
> Yes
> 
> 

Re: [PATCH v4 00/31] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

08.10.2019 12:39, Greg Kurz wrote:
> On Tue, 8 Oct 2019 08:41:08 +0000
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> 08.10.2019 10:30, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> Hi all!
>>>>
>>>> Here is a proposal of auto propagation for local_err, to not call
>>>> error_propagate on every exit point, when we deal with local_err.
>>>>
>>>> There are also two issues with errp:
>>>>
>>>> 1. error_fatal & error_append_hint/error_prepend: user can't see this
>>>> additional info, because exit() happens in error_setg earlier than info
>>>> is added. [Reported by Greg Kurz]
>>>
>>> How is this series related to Greg's "[PATCH 00/17] Fix usage of
>>> error_append_hint()"?  Do we need both?
>>
>> These series is a substitution for Greg's. Still, there are problems with
>> automation, which Greg pointed in 21/31, and I don't know what to do next.
>>
>> May be, just continue to review patches and fix them by hand. May be try to
>> improve automation...
>>
> 
> The feeling I have after working on my series is that the lines that deal
> with errors are mixed up with the functional code in a variety of ways.
> That makes it very difficult if not impossible to come with code patterns
> suitable for a 100% automated solution IMHO.
> 
> My questioning is more around the semantics of error_fatal actually. What
> does passing &error_fatal gives us over passing &local_err and calling
> error_report_err()+exit(), apart from breaking error_append_hint() and
> error_prepend() ?

As I understand, the only benefit is one line instead of four:

func(..., &error_fatal);

instead of

func(..., &local_err);
if (local_err) {
     exit(1);
}

But, keeping in mind all difficulties about these series... We can consider
conversion error_fatal -> local_err too. It seems simple to do with a coccinelle
script, I can send another automatic series to look at it.


Hmm, some ideas around this:

1. We can generate _fatal versions of functions by python script (we'll call it from Makefile, we have a lot of generated code anyway).

and convert
func(..., &local_err); to

func_fatal(...);

2. Use macro like

#define FATAL(func, ...) do { Error *__fatal_err = NULL; func(__VA_ARGS__ __VA_OPT(,), &__fatal_err); if (__fatal_err) { error_report(__fatal_err); exit(1); } } while (0)

and convert
func(..., &local_err); to

FATAL(func, ...);

> 
>>>
>>>> 2. error_abort & error_propagate: when we wrap
>>>> error_abort by local_err+error_propagate, resulting coredump will
>>>> refer to error_propagate and not to the place where error happened.
>>>> (the macro itself don't fix the issue, but it allows to [3.] drop all
>>>> local_err+error_propagate pattern, which will definitely fix the issue)
>>>> [Reported by Kevin Wolf]
>>>>
>>>> Still, applying new macro to all errp-functions is a huge task, which is
>>>> impossible to solve in one series.
>>>>
>>>> So, here is a minimum: solve only [1.], by adding new macro to all
>>>> errp-functions which wants to call error_append_hint.
>>>>
>>>> v4;
>>>> 02: - check errp to be not NULL
>>>>       - drop Eric's r-b
>>>> 03: add Eric's r-b
>>>> 04: - rename macro to ERRP_AUTO_PROPAGATE [Kevin]
>>>>       - improve comment and commit msg, mention
>>>>         error_prepend
>>>> 05: - handle error_prepend too
>>>>       - use new macro name
>>>>       - drop empty line at the end
>>>>
>>>> commit message for auto-generated commits updated,
>>>> commits regenerated.
>>>>
>>>> I'll use cc-cmd to cc appropriate recipients per patch, still
>>>> cover-letter together with 04-06 patches should be interesting for
>>>> all:
>>> [...]
>>>
>>> Big series; let me guess its structure.
>>>
>>>> Vladimir Sementsov-Ogievskiy (31):
>>>>     errp: rename errp to errp_in where it is IN-argument
>>>>     hw/core/loader-fit: fix freeing errp in fit_load_fdt
>>>>     net/net: fix local variable shadowing in net_client_init
>>>
>>> Preparations.
>>>
>>>>     error: auto propagated local_err
>>>
>>> The new idea.
>>>
>>>>     scripts: add script to fix error_append_hint/error_prepend usage
>>>>     python: add commit-per-subsystem.py
>>>
>>> Scripts to help you apply it.
>>>
>>>>     s390: Fix error_append_hint/error_prepend usage
>>>>     ARM TCG CPUs: Fix error_append_hint/error_prepend usage
>>>>     PowerPC TCG CPUs: Fix error_append_hint/error_prepend usage
>>>>     arm: Fix error_append_hint/error_prepend usage
>>>>     SmartFusion2: Fix error_append_hint/error_prepend usage
>>>>     ASPEED BMCs: Fix error_append_hint/error_prepend usage
>>>>     Boston: Fix error_append_hint/error_prepend usage
>>>>     PowerNV (Non-Virtualized): Fix error_append_hint/error_prepend usage
>>>>     PCI: Fix error_append_hint/error_prepend usage
>>>>     SCSI: Fix error_append_hint/error_prepend usage
>>>>     USB: Fix error_append_hint/error_prepend usage
>>>>     VFIO: Fix error_append_hint/error_prepend usage
>>>>     vhost: Fix error_append_hint/error_prepend usage
>>>>     virtio: Fix error_append_hint/error_prepend usage
>>>>     virtio-9p: Fix error_append_hint/error_prepend usage
>>>>     XIVE: Fix error_append_hint/error_prepend usage
>>>>     block: Fix error_append_hint/error_prepend usage
>>>>     chardev: Fix error_append_hint/error_prepend usage
>>>>     cmdline: Fix error_append_hint/error_prepend usage
>>>>     QOM: Fix error_append_hint/error_prepend usage
>>>>     Migration: Fix error_append_hint/error_prepend usage
>>>>     Sockets: Fix error_append_hint/error_prepend usage
>>>>     nbd: Fix error_append_hint/error_prepend usage
>>>>     PVRDMA: Fix error_append_hint/error_prepend usage
>>>>     ivshmem: Fix error_append_hint/error_prepend usage
>>>
>>> Applying it.
>>>
>>> Correct?
>>>
>>
>> Yes
>>
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 00/31] error: auto propagated local_err

[Markus Armbruster]

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

> 08.10.2019 12:39, Greg Kurz wrote:
>> On Tue, 8 Oct 2019 08:41:08 +0000
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> 
>>> 08.10.2019 10:30, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>
>>>>> Hi all!
>>>>>
>>>>> Here is a proposal of auto propagation for local_err, to not call
>>>>> error_propagate on every exit point, when we deal with local_err.
>>>>>
>>>>> There are also two issues with errp:
>>>>>
>>>>> 1. error_fatal & error_append_hint/error_prepend: user can't see this
>>>>> additional info, because exit() happens in error_setg earlier than info
>>>>> is added. [Reported by Greg Kurz]
>>>>
>>>> How is this series related to Greg's "[PATCH 00/17] Fix usage of
>>>> error_append_hint()"?  Do we need both?
>>>
>>> These series is a substitution for Greg's. Still, there are problems with
>>> automation, which Greg pointed in 21/31, and I don't know what to do next.
>>>
>>> May be, just continue to review patches and fix them by hand. May be try to
>>> improve automation...
>>>
>> 
>> The feeling I have after working on my series is that the lines that deal
>> with errors are mixed up with the functional code in a variety of ways.
>> That makes it very difficult if not impossible to come with code patterns
>> suitable for a 100% automated solution IMHO.
>> 
>> My questioning is more around the semantics of error_fatal actually. What
>> does passing &error_fatal gives us over passing &local_err and calling
>> error_report_err()+exit(), apart from breaking error_append_hint() and
>> error_prepend() ?
>
> As I understand, the only benefit is one line instead of four:

Brevity matters when it comes to error handling.

> func(..., &error_fatal);
>
> instead of
>
> func(..., &local_err);
> if (local_err) {
       error_report_err(local_err);
>      exit(1);
> }
[...]

Re: [PATCH v4 01/31] errp: rename errp to errp_in where it is IN-argument

[Markus Armbruster]

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

> 08.10.2019 12:08, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 
>>> Error **errp is almost always OUT-argument: it's assumed to be NULL, or
>>> pointer to NULL-initialized pointer, or pointer to error_abort or
>>> error_fatal, for callee to report error.
>> 
>> Yes.
>> 
>>> But very few functions instead get Error **errp as IN-argument:
>>> it's assumed to be set, and callee should clean it.
>> 
>> What do you mean by "callee should clean"?  Let's see below.
>> 
>>> In such cases, rename errp to errp_in.
>> 
>> I acknowledge that errp arguments that don't have the usual meaning can
>> be confusing.
>> 
>> Naming can help, comments can help, but perhaps we can tweak the code to
>> avoid the problem instead.  Let's see:
>> 
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
[...]
>> We can avoid the confusing Error **errp in all three cases by peeling
>> off an indirection.  What do you think?
>> 
>
> I like the idea, thanks! I think, I'll try to make a patch.
>
> But you are right, unfortunately there more cases, at least, pointed by
> Greg
>
> error_append_socket_sockfd_hint and
> error_append_security_model_hint
>
> Which don't free error..

Neither do error_append_hint() and error_prepend().

For anything named error_append_FOO_hint() or error_prepend_FOO(),
confusion seems unlikely.

>                          But if they append hint, they should always be called
> on wrapped errp, accordingly to the problem about fatal_error, so they may
> be converted to Error *err too.. Hmm, I should think about the script to find
> such functions.

I figure I better read more of your series before I comment on this
thought.

Re: [PATCH v4 02/31] hw/core/loader-fit: fix freeing errp in fit_load_fdt

[Markus Armbruster]

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

> fit_load_fdt forget to check that errp is not NULL and to zero it after
> freeing. Fix it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  hw/core/loader-fit.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
> index 953b16bc82..3ee9fb2f2e 100644
> --- a/hw/core/loader-fit.c
> +++ b/hw/core/loader-fit.c
> @@ -200,7 +200,10 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>      err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
>      if (err == -ENOENT) {
>          load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
> -        error_free(*errp);
> +        if (errp) {
> +            error_free(*errp);
> +            *errp = NULL;
> +        }
>      } else if (err) {
>          error_prepend(errp, "unable to read FDT load address from FIT: ");
>          ret = err;

Hmm.  Should we have error_clear(), similar to g_clear_error()?

https://developer.gnome.org/glib/stable/glib-Error-Reporting.html#g-clear-error

Re: [PATCH v4 03/31] net/net: fix local variable shadowing in net_client_init

[Markus Armbruster]

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

> Don't shadow Error *err: it's a bad thing.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Looks like a cleanup, not a bug fix.  If that's the case, the commit
message should say so:

    net/net: Clean up variable shadowing in net_client_init()

    Variable int err in inner scope shadows Error *err in outer scope.

Patch looks good to me.

Re: [PATCH v4 04/31] error: auto propagated local_err

[Markus Armbruster]

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

> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]
>
> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
> local_err+error_propagate pattern, which will definitely fix the issue)
> [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).

Starting with stating your goals is an excellent idea.  But I'd love to
next read a high-level description of how your patch achieves or enables
achieving these goals.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
[...]
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 9376f59c35..02f967ac1d 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -322,6 +322,43 @@ void error_set_internal(Error **errp,
>                          ErrorClass err_class, const char *fmt, ...)
>      GCC_FMT_ATTR(6, 7);
>  
> +typedef struct ErrorPropagator {
> +    Error *local_err;
> +    Error **errp;
> +} ErrorPropagator;
> +
> +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> +{
> +    error_propagate(prop->errp, prop->local_err);
> +}
> +
> +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> +
> +/*
> + * ERRP_AUTO_PROPAGATE
> + *
> + * This macro is created to be the first line of a function with Error **errp
> + * OUT parameter. It's needed only in cases where we want to use error_prepend,
> + * error_append_hint or dereference *errp. It's still safe (but useless) in
> + * other cases.
> + *
> + * If errp is NULL or points to error_fatal, it is rewritten to point to a
> + * local Error object, which will be automatically propagated to the original
> + * errp on function exit (see error_propagator_cleanup).
> + *
> + * After invocation of this macro it is always safe to dereference errp
> + * (as it's not NULL anymore) and to append hints (by error_append_hint)
> + * (as, if it was error_fatal, we swapped it with a local_error to be
> + * propagated on cleanup).

Well, appending hints was always safe, it just didn't work with
&error_fatal.  Don't worry about that now, I'll probably want to polish
this contract comment a bit anyway, but later.

> + *
> + * Note: we don't wrap the error_abort case, as we want resulting coredump
> + * to point to the place where the error happened, not to error_propagate.
> + */
> +#define ERRP_AUTO_PROPAGATE() \
> +g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \

Took me a second to realize: the macro works, because the initializer
implicitly initializes .local_error = NULL.

__auto_errp_prop is an identifier reserved for any use.  I think we
could use _auto_errp_prop, which is only reserved for use as identifiers
with file scope in both the ordinary and tag name spaces.  See ISO/IEC
9899:1999 7.1.3 Reserved identifiers.

> +errp = ((errp == NULL || *errp == error_fatal) ? \
> +    &__auto_errp_prop.local_err : errp)
> +

Please indent multi-line macros like elsewhere in this file:

#define ERRP_AUTO_PROPAGATE()					\
    g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp};	\
    errp = ((errp == NULL || *errp == error_fatal)		\
            ? &__auto_errp_prop.local_err : errp)

>  /*
>   * Special error destination to abort on error.
>   * See error_setg() and error_propagate() for details.

To be honest, the cover letter left me a bit skeptical, but now I think
you might be up to something.  Let's see how the patches putting the
macro to use come out.

Re: [PATCH v4 04/31] error: auto propagated local_err

[Greg Kurz]

On Tue, 08 Oct 2019 18:03:13 +0200
Markus Armbruster <armbru@redhat.com> wrote:

> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
> > Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> > functions with errp OUT parameter.
> >
> > It has three goals:
> >
> > 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
> > can't see this additional information, because exit() happens in
> > error_setg earlier than information is added. [Reported by Greg Kurz]
> >
> > 2. Fix issue with error_abort & error_propagate: when we wrap
> > error_abort by local_err+error_propagate, resulting coredump will
> > refer to error_propagate and not to the place where error happened.
> > (the macro itself doesn't fix the issue, but it allows to [3.] drop all
> > local_err+error_propagate pattern, which will definitely fix the issue)
> > [Reported by Kevin Wolf]
> >
> > 3. Drop local_err+error_propagate pattern, which is used to workaround
> > void functions with errp parameter, when caller wants to know resulting
> > status. (Note: actually these functions could be merely updated to
> > return int error code).
> 
> Starting with stating your goals is an excellent idea.  But I'd love to
> next read a high-level description of how your patch achieves or enables
> achieving these goals.
> 
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> [...]
> > diff --git a/include/qapi/error.h b/include/qapi/error.h
> > index 9376f59c35..02f967ac1d 100644
> > --- a/include/qapi/error.h
> > +++ b/include/qapi/error.h
> > @@ -322,6 +322,43 @@ void error_set_internal(Error **errp,
> >                          ErrorClass err_class, const char *fmt, ...)
> >      GCC_FMT_ATTR(6, 7);
> >  
> > +typedef struct ErrorPropagator {
> > +    Error *local_err;
> > +    Error **errp;
> > +} ErrorPropagator;
> > +
> > +static inline void error_propagator_cleanup(ErrorPropagator *prop)
> > +{
> > +    error_propagate(prop->errp, prop->local_err);
> > +}
> > +
> > +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
> > +
> > +/*
> > + * ERRP_AUTO_PROPAGATE
> > + *
> > + * This macro is created to be the first line of a function with Error **errp
> > + * OUT parameter. It's needed only in cases where we want to use error_prepend,
> > + * error_append_hint or dereference *errp. It's still safe (but useless) in
> > + * other cases.
> > + *
> > + * If errp is NULL or points to error_fatal, it is rewritten to point to a
> > + * local Error object, which will be automatically propagated to the original
> > + * errp on function exit (see error_propagator_cleanup).
> > + *
> > + * After invocation of this macro it is always safe to dereference errp
> > + * (as it's not NULL anymore) and to append hints (by error_append_hint)
> > + * (as, if it was error_fatal, we swapped it with a local_error to be
> > + * propagated on cleanup).
> 
> Well, appending hints was always safe, it just didn't work with
> &error_fatal.  Don't worry about that now, I'll probably want to polish
> this contract comment a bit anyway, but later.
> 

FWIW I've already posted this:

Author: Greg Kurz <groug@kaod.org>
Date:   Mon Oct 7 15:45:46 2019 +0200

    error: Update error_append_hint()'s documenation
    
    error_setg() and error_propagate(), as well as their variants, cause
    QEMU to terminate when called with &error_fatal or &error_abort. This
    prevents to add hints since error_append_hint() isn't even called in
    this case.
    
    It means that error_append_hint() should only be used with a local
    error object, and then propagate this local error to the caller.
    
    Document this in <qapi/error.h> .
    
    Signed-off-by: Greg Kurz <groug@kaod.org>

Message-id: <156871563702.196432.5964411202152101367.stgit@bahia.lan>
https://patchwork.ozlabs.org/patch/1163278/

> > + *
> > + * Note: we don't wrap the error_abort case, as we want resulting coredump
> > + * to point to the place where the error happened, not to error_propagate.
> > + */
> > +#define ERRP_AUTO_PROPAGATE() \
> > +g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
> 
> Took me a second to realize: the macro works, because the initializer
> implicitly initializes .local_error = NULL.
> 
> __auto_errp_prop is an identifier reserved for any use.  I think we
> could use _auto_errp_prop, which is only reserved for use as identifiers
> with file scope in both the ordinary and tag name spaces.  See ISO/IEC
> 9899:1999 7.1.3 Reserved identifiers.
> 
> > +errp = ((errp == NULL || *errp == error_fatal) ? \
> > +    &__auto_errp_prop.local_err : errp)
> > +
> 
> Please indent multi-line macros like elsewhere in this file:
> 
> #define ERRP_AUTO_PROPAGATE()					\
>     g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp};	\
>     errp = ((errp == NULL || *errp == error_fatal)		\
>             ? &__auto_errp_prop.local_err : errp)
> 
> >  /*
> >   * Special error destination to abort on error.
> >   * See error_setg() and error_propagate() for details.
> 
> To be honest, the cover letter left me a bit skeptical, but now I think
> you might be up to something.  Let's see how the patches putting the
> macro to use come out.

Re: [PATCH v4 04/31] error: auto propagated local_err

[Markus Armbruster]

Greg Kurz <groug@kaod.org> writes:

> On Tue, 08 Oct 2019 18:03:13 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 
>> > Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
>> > functions with errp OUT parameter.
>> >
>> > It has three goals:
>> >
>> > 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
>> > can't see this additional information, because exit() happens in
>> > error_setg earlier than information is added. [Reported by Greg Kurz]
>> >
>> > 2. Fix issue with error_abort & error_propagate: when we wrap
>> > error_abort by local_err+error_propagate, resulting coredump will
>> > refer to error_propagate and not to the place where error happened.
>> > (the macro itself doesn't fix the issue, but it allows to [3.] drop all
>> > local_err+error_propagate pattern, which will definitely fix the issue)
>> > [Reported by Kevin Wolf]
>> >
>> > 3. Drop local_err+error_propagate pattern, which is used to workaround
>> > void functions with errp parameter, when caller wants to know resulting
>> > status. (Note: actually these functions could be merely updated to
>> > return int error code).
>> 
>> Starting with stating your goals is an excellent idea.  But I'd love to
>> next read a high-level description of how your patch achieves or enables
>> achieving these goals.
>> 
>> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> > ---
>> [...]
>> > diff --git a/include/qapi/error.h b/include/qapi/error.h
>> > index 9376f59c35..02f967ac1d 100644
>> > --- a/include/qapi/error.h
>> > +++ b/include/qapi/error.h
>> > @@ -322,6 +322,43 @@ void error_set_internal(Error **errp,
>> >                          ErrorClass err_class, const char *fmt, ...)
>> >      GCC_FMT_ATTR(6, 7);
>> >  
>> > +typedef struct ErrorPropagator {
>> > +    Error *local_err;
>> > +    Error **errp;
>> > +} ErrorPropagator;
>> > +
>> > +static inline void error_propagator_cleanup(ErrorPropagator *prop)
>> > +{
>> > +    error_propagate(prop->errp, prop->local_err);
>> > +}
>> > +
>> > +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(ErrorPropagator, error_propagator_cleanup);
>> > +
>> > +/*
>> > + * ERRP_AUTO_PROPAGATE
>> > + *
>> > + * This macro is created to be the first line of a function with Error **errp
>> > + * OUT parameter. It's needed only in cases where we want to use error_prepend,
>> > + * error_append_hint or dereference *errp. It's still safe (but useless) in
>> > + * other cases.
>> > + *
>> > + * If errp is NULL or points to error_fatal, it is rewritten to point to a
>> > + * local Error object, which will be automatically propagated to the original
>> > + * errp on function exit (see error_propagator_cleanup).
>> > + *
>> > + * After invocation of this macro it is always safe to dereference errp
>> > + * (as it's not NULL anymore) and to append hints (by error_append_hint)
>> > + * (as, if it was error_fatal, we swapped it with a local_error to be
>> > + * propagated on cleanup).
>> 
>> Well, appending hints was always safe, it just didn't work with
>> &error_fatal.  Don't worry about that now, I'll probably want to polish
>> this contract comment a bit anyway, but later.
>> 
>
> FWIW I've already posted this:
>
> Author: Greg Kurz <groug@kaod.org>
> Date:   Mon Oct 7 15:45:46 2019 +0200
>
>     error: Update error_append_hint()'s documenation
>     
>     error_setg() and error_propagate(), as well as their variants, cause
>     QEMU to terminate when called with &error_fatal or &error_abort. This
>     prevents to add hints since error_append_hint() isn't even called in
>     this case.
>     
>     It means that error_append_hint() should only be used with a local
>     error object, and then propagate this local error to the caller.
>     
>     Document this in <qapi/error.h> .
>     
>     Signed-off-by: Greg Kurz <groug@kaod.org>
>
> Message-id: <156871563702.196432.5964411202152101367.stgit@bahia.lan>
> https://patchwork.ozlabs.org/patch/1163278/

I marked your series containing this patch for review.  I decided to
review this one first.  I clearly need look at yours as well before I
can advise on how to proceed.

[...]

Re: [PATCH v4 23/31] block: Fix error_append_hint/error_prepend usage

[Markus Armbruster]

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

> 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.
>
> This commit (together with its neighbors) was generated by
>
> git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
> spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
> --in-place $f; done
>
> and then
>
> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
>
> (auto-msg was a file with this commit message)
>
> and then by hand, for not maintained changed files:
>
> git commit -m "<SUB-SYSTEM>: $(< auto-msg)" <FILES>
>
> Still, for backporting it may be more comfortable to use only the first
> command and then do one huge commit.
>
> Reported-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/block/nbd.h  | 1 +
>  block.c              | 3 +++
>  block/backup.c       | 1 +
>  block/dirty-bitmap.c | 1 +
>  block/file-posix.c   | 4 ++++
>  block/gluster.c      | 2 ++
>  block/qcow.c         | 1 +
>  block/qcow2-bitmap.c | 1 +
>  block/qcow2.c        | 3 +++
>  block/vdi.c          | 1 +
>  block/vhdx-log.c     | 1 +
>  block/vmdk.c         | 1 +
>  block/vpc.c          | 1 +
>  13 files changed, 21 insertions(+)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 316fd705a9..330f40142a 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -360,6 +360,7 @@ void nbd_server_start(SocketAddress *addr, const char *tls_creds,
>  static inline int nbd_read(QIOChannel *ioc, void *buffer, size_t size,
>                             const char *desc, Error **errp)
>  {
> +    ERRP_AUTO_PROPAGATE();
>      int ret = qio_channel_read_all(ioc, buffer, size, errp) < 0 ? -EIO : 0;
>  
>      if (ret < 0) {

This is an example of commit-per-subsystem.py producing a questionable
split.  MAINTAINERS files include/block/nbd.h under both "Block layer
core" and "Network Block Device (NBD)".  The script picks "Block layer
core" because it comes first.

Re: [PATCH v4 07/31] s390: Fix error_append_hint/error_prepend usage

[Markus Armbruster]

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

> 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.
>
> This commit (together with its neighbors) was generated by
>
> git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
> spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
> --in-place $f; done

Make that:

    $ spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff `git grep -l 'error_\(append_hint\|prepend\)(errp' \*.[ch]`

Adding --macro-file is essential, as Eric noted.  Without it, we miss
qcow2_store_persistent_dirty_bitmaps() in PATCH 23 and
nbd_negotiate_send_rep_verr() in PATCH 29.  There should be a way to
make spatch warn when it gives up parsing, but I can't find it right
now.

Avoiding the loop is just for speed and simplicity.

--no-show-diff goes well with --in-place.

The even simpler

    $ spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff --use-gitgrep

misses include/block/nbd.h's nbd_read() in PATCH 23 somehow.

I recommend to add the spatch invocation to the coccinelle script's
commit message, too [PATCH 05].

[...]

Re: [PATCH v4 04/31] error: auto propagated local_err

[Markus Armbruster]

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

> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
> functions with errp OUT parameter.
>
> It has three goals:
>
> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
> can't see this additional information, because exit() happens in
> error_setg earlier than information is added. [Reported by Greg Kurz]

Done in PATCH 07-31.

> 2. Fix issue with error_abort & error_propagate: when we wrap
> error_abort by local_err+error_propagate, resulting coredump will
> refer to error_propagate and not to the place where error happened.
> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
> local_err+error_propagate pattern, which will definitely fix the issue)
> [Reported by Kevin Wolf]
>
> 3. Drop local_err+error_propagate pattern, which is used to workaround
> void functions with errp parameter, when caller wants to know resulting
> status. (Note: actually these functions could be merely updated to
> return int error code).

Not done.  Can you prototype this part?  A few manually done examples
would give us an idea how the complete solution would look like.  A
(semi-)automated complete conversion of a subsystem would additionally
give us an idea how to actually do the conversion.

We can discuss applying ERRP_AUTO_PROPAGATE() as a bug fix for 1., and
leave 2. and 3. for later.  Feels like a half-done job to me.  We've got
too many of those in the tree already.  Dunno.

Re: [PATCH v4 04/31] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

09.10.2019 11:04, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
>> functions with errp OUT parameter.
>>
>> It has three goals:
>>
>> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
>> can't see this additional information, because exit() happens in
>> error_setg earlier than information is added. [Reported by Greg Kurz]
> 
> Done in PATCH 07-31.
> 
>> 2. Fix issue with error_abort & error_propagate: when we wrap
>> error_abort by local_err+error_propagate, resulting coredump will
>> refer to error_propagate and not to the place where error happened.
>> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
>> local_err+error_propagate pattern, which will definitely fix the issue)
>> [Reported by Kevin Wolf]
>>
>> 3. Drop local_err+error_propagate pattern, which is used to workaround
>> void functions with errp parameter, when caller wants to know resulting
>> status. (Note: actually these functions could be merely updated to
>> return int error code).
> 
> Not done.  Can you prototype this part?  A few manually done examples
> would give us an idea how the complete solution would look like.  A
> (semi-)automated complete conversion of a subsystem would additionally
> give us an idea how to actually do the conversion.
> 
> We can discuss applying ERRP_AUTO_PROPAGATE() as a bug fix for 1., and
> leave 2. and 3. for later.  Feels like a half-done job to me.  We've got
> too many of those in the tree already.  Dunno.
> 

If update everything it's about 90-140 patches.

The whole thing was done in "[RFC v2 0/9] error: auto propagated local_err"
<20190923161231.22028-1-vsementsov@virtuozzo.com>
https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg05157.html

- it was so huge, that I decided to postpone 2 and 3...



-- 
Best regards,
Vladimir

Re: [PATCH v4 00/31] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

08.10.2019 13:09, Vladimir Sementsov-Ogievskiy wrote:
> 08.10.2019 12:39, Greg Kurz wrote:
>> On Tue, 8 Oct 2019 08:41:08 +0000
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>
>>> 08.10.2019 10:30, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>
>>>>> Hi all!
>>>>>
>>>>> Here is a proposal of auto propagation for local_err, to not call
>>>>> error_propagate on every exit point, when we deal with local_err.
>>>>>
>>>>> There are also two issues with errp:
>>>>>
>>>>> 1. error_fatal & error_append_hint/error_prepend: user can't see this
>>>>> additional info, because exit() happens in error_setg earlier than info
>>>>> is added. [Reported by Greg Kurz]
>>>>
>>>> How is this series related to Greg's "[PATCH 00/17] Fix usage of
>>>> error_append_hint()"?  Do we need both?
>>>
>>> These series is a substitution for Greg's. Still, there are problems with
>>> automation, which Greg pointed in 21/31, and I don't know what to do next.
>>>
>>> May be, just continue to review patches and fix them by hand. May be try to
>>> improve automation...
>>>
>>
>> The feeling I have after working on my series is that the lines that deal
>> with errors are mixed up with the functional code in a variety of ways.
>> That makes it very difficult if not impossible to come with code patterns
>> suitable for a 100% automated solution IMHO.
>>
>> My questioning is more around the semantics of error_fatal actually. What
>> does passing &error_fatal gives us over passing &local_err and calling
>> error_report_err()+exit(), apart from breaking error_append_hint() and
>> error_prepend() ?
> 
> As I understand, the only benefit is one line instead of four:
> 
> func(..., &error_fatal);
> 
> instead of
> 
> func(..., &local_err);
> if (local_err) {
>      exit(1);
> }
> 
> But, keeping in mind all difficulties about these series... We can consider
> conversion error_fatal -> local_err too. It seems simple to do with a coccinelle
> script, I can send another automatic series to look at it.
> 
> 
> Hmm, some ideas around this:
> 
> 1. We can generate _fatal versions of functions by python script (we'll call it from Makefile, we have a lot of generated code anyway).
> 
> and convert
> func(..., &local_err); to
> 
> func_fatal(...);
> 
> 2. Use macro like
> 
> #define FATAL(func, ...) do { Error *__fatal_err = NULL; func(__VA_ARGS__ __VA_OPT(,), &__fatal_err); if (__fatal_err) { error_report(__fatal_err); exit(1); } } while (0)
> 
> and convert
> func(..., &local_err); to
> 
> FATAL(func, ...);

Now, I think, that what these series do is better, as
func(..., &fatal_err) and one macro invocation at function start
is better than too many generated functions.

-- 
Best regards,
Vladimir

Re: [PATCH v4 01/31] errp: rename errp to errp_in where it is IN-argument

[Vladimir Sementsov-Ogievskiy]

08.10.2019 12:08, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> Error **errp is almost always OUT-argument: it's assumed to be NULL, or
>> pointer to NULL-initialized pointer, or pointer to error_abort or
>> error_fatal, for callee to report error.
> 
> Yes.
> 
>> But very few functions instead get Error **errp as IN-argument:
>> it's assumed to be set, and callee should clean it.
> 
> What do you mean by "callee should clean"?  Let's see below.
> 
>> In such cases, rename errp to errp_in.
> 
> I acknowledge that errp arguments that don't have the usual meaning can
> be confusing.
> 
> Naming can help, comments can help, but perhaps we can tweak the code to
> avoid the problem instead.  Let's see:
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   include/monitor/hmp.h |  2 +-
>>   include/qapi/error.h  |  2 +-
>>   ui/vnc.h              |  2 +-
>>   monitor/hmp-cmds.c    |  8 ++++----
>>   ui/vnc.c              | 10 +++++-----
>>   util/error.c          |  8 ++++----
>>   6 files changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
>> index a0e9511440..f929814f1a 100644
>> --- a/include/monitor/hmp.h
>> +++ b/include/monitor/hmp.h
>> @@ -16,7 +16,7 @@
>>   
>>   #include "qemu/readline.h"
>>   
>> -void hmp_handle_error(Monitor *mon, Error **errp);
>> +void hmp_handle_error(Monitor *mon, Error **errp_in);
>>   
>>   void hmp_info_name(Monitor *mon, const QDict *qdict);
>>   void hmp_info_version(Monitor *mon, const QDict *qdict);
>> diff --git a/include/qapi/error.h b/include/qapi/error.h
>> index 3f95141a01..9376f59c35 100644
>> --- a/include/qapi/error.h
>> +++ b/include/qapi/error.h
>> @@ -283,7 +283,7 @@ void error_free(Error *err);
>>   /*
>>    * Convenience function to assert that *@errp is set, then silently free it.
>>    */
>> -void error_free_or_abort(Error **errp);
>> +void error_free_or_abort(Error **errp_in);
>>   
>>   /*
>>    * Convenience function to warn_report() and free @err.
>> diff --git a/ui/vnc.h b/ui/vnc.h
>> index fea79c2fc9..00e0b48f2f 100644
>> --- a/ui/vnc.h
>> +++ b/ui/vnc.h
>> @@ -547,7 +547,7 @@ uint32_t read_u32(uint8_t *data, size_t offset);
>>   
>>   /* Protocol stage functions */
>>   void vnc_client_error(VncState *vs);
>> -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp);
>> +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in);
>>   
>>   void start_client_init(VncState *vs);
>>   void start_auth_vnc(VncState *vs);
>> diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
>> index b2551c16d1..941d5d0a45 100644
>> --- a/monitor/hmp-cmds.c
>> +++ b/monitor/hmp-cmds.c
>> @@ -60,11 +60,11 @@
>>   #include <spice/enums.h>
>>   #endif
>>   
>> -void hmp_handle_error(Monitor *mon, Error **errp)
>> +void hmp_handle_error(Monitor *mon, Error **errp_in)
>>   {
>> -    assert(errp);
>> -    if (*errp) {
>> -        error_reportf_err(*errp, "Error: ");
>> +    assert(errp_in);
>> +    if (*errp_in) {
>> +        error_reportf_err(*errp_in, "Error: ");
>>       }
>>   }
> 
> This functions frees the error.  It leaves nothing for the caller to
> clean up.
> 
> All callers pass &ERR, where ERR is a local variable.  Perhaps a more
> robust way to signal "@errp is not the usual out-argument" would be
> peeling off an indirection: pass ERR, drop the assertion.
> 
>>   
>> diff --git a/ui/vnc.c b/ui/vnc.c
>> index 87b8045afe..9d6384d9b1 100644
>> --- a/ui/vnc.c
>> +++ b/ui/vnc.c
>> @@ -1312,7 +1312,7 @@ void vnc_disconnect_finish(VncState *vs)
>>       g_free(vs);
>>   }
>>   
>> -size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
>> +size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp_in)
>>   {
>>       if (ret <= 0) {
>>           if (ret == 0) {
>> @@ -1320,14 +1320,14 @@ size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
>>               vnc_disconnect_start(vs);
>>           } else if (ret != QIO_CHANNEL_ERR_BLOCK) {
>>               trace_vnc_client_io_error(vs, vs->ioc,
>> -                                      errp ? error_get_pretty(*errp) :
>> +                                      errp_in ? error_get_pretty(*errp_in) :
>>                                         "Unknown");
>>               vnc_disconnect_start(vs);
>>           }
>>   
>> -        if (errp) {
>> -            error_free(*errp);
>> -            *errp = NULL;
>> +        if (errp_in) {
>> +            error_free(*errp_in);
>> +            *errp_in = NULL;
>>           }
>>           return 0;
>>       }
> 
> This function isn't trivial, and lacks a contract, so let's figure out
> what it does and how it's used.
> 
> @ret can be:
> 
> * Zero
> 
>    Trace EOF, call vnc_disconnect_start(), free the error, return zero.
> 
>    Aside: freeing the error without looking at it feels odd.  Can this
>    happen?
> 
> * Negative other than QIO_CHANNEL_ERR_BLOCK
> 
>    Trace the error if any, else "Unknown" error, call
>    vnc_disconnect_start(), free the error if any, return zero.
> 
>    Note that we can't have errp && !*errp here, or else tracing crashes
>    in error_get_pretty().
> 
> * QIO_CHANNEL_ERR_BLOCK
> 
>    Free the error, return zero
> 
> * Positive
> 
>    Do nothing, return @ret
> 
> Callers pass one of the following:
> 
> * ret = -1 and errp = NULL
> 
>    This uses case "Negative other than QIO_CHANNEL_ERR_BLOCK".  Since
>    error is null, it traces an "Unknown" error.
> 
> * ret and &err, where ret = FUN(..., &err), and FUN is
>    qio_channel_read() or qio_channel_write().
> 
>    qio_channel_read(), _write() are documented to return non-negative on
>    success, QIO_CHANNEL_ERR_BLOCK on "would block", and -1 on other
>    error.  By convention, they set an error exactly when they fail,
>    i.e. when they return a negative value.
> 
>    When qio_channel_read() / _write() succeed, we use case "Positive" or
>    "Zero".  We don't free the error, which is fine, as none was returned.
>    Aside: I *guess* the channel is non-blocking, and "zero" can happen
>    only when read hits EOF.
> 
>    When qio_channel_read() / _write() fail, we use one of the error
>    cases.
> 
> Looks like vnc_client_io_error() takes an error code @ret and an
> optional error object in @errp with additional details.  If @ret is
> non-negative, @errp must be null or point to null.  If @ret is negative,
> @errp must be null or point to non-null.
> 
> vnc_client_io_error() frees the error.  It leaves nothing for the caller
> to clean up.
> 
> I think we can again peel off an indirection.  The two kinds of calls
> become:
> 
> * ret = -1 and err = NULL
> 
>    No textual change, but the NULL gets converted to Error * instead of
>    Error **.
> 
> * ret and err
> 
>    Pass the (possibly null) error object instead of a pointer to the
>    local variable.
> 
>> diff --git a/util/error.c b/util/error.c
>> index d4532ce318..b3ff3832d6 100644
>> --- a/util/error.c
>> +++ b/util/error.c
>> @@ -271,11 +271,11 @@ void error_free(Error *err)
>>       }
>>   }
>>   
>> -void error_free_or_abort(Error **errp)
>> +void error_free_or_abort(Error **errp_in)
>>   {
>> -    assert(errp && *errp);
>> -    error_free(*errp);
>> -    *errp = NULL;
>> +    assert(errp_in && *errp_in);
>> +    error_free(*errp_in);
>> +    *errp_in = NULL;
>>   }
>>   
>>   void error_propagate(Error **dst_errp, Error *local_err)
> 
> This functions frees the error.  It leaves nothing for the caller to
> clean up.
> 
> All callers pass &ERR, where ERR is a local variable.  We can peel off
> an indirection.


But if we drop indirection, we'll have to set local variable to NULL by
hand. Is it good?

Look at test_keyval_parse_list() for example: it uses local err object
several times, so it depends on the fact that error_free_or_abort
sets pointer to NULL.


-- 
Best regards,
Vladimir

Re: [PATCH v4 01/31] errp: rename errp to errp_in where it is IN-argument

[Vladimir Sementsov-Ogievskiy]

08.10.2019 15:05, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 08.10.2019 12:08, Markus Armbruster wrote:
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>
>>>> Error **errp is almost always OUT-argument: it's assumed to be NULL, or
>>>> pointer to NULL-initialized pointer, or pointer to error_abort or
>>>> error_fatal, for callee to report error.
>>>
>>> Yes.
>>>
>>>> But very few functions instead get Error **errp as IN-argument:
>>>> it's assumed to be set, and callee should clean it.
>>>
>>> What do you mean by "callee should clean"?  Let's see below.
>>>
>>>> In such cases, rename errp to errp_in.
>>>
>>> I acknowledge that errp arguments that don't have the usual meaning can
>>> be confusing.
>>>
>>> Naming can help, comments can help, but perhaps we can tweak the code to
>>> avoid the problem instead.  Let's see:
>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
> [...]
>>> We can avoid the confusing Error **errp in all three cases by peeling
>>> off an indirection.  What do you think?
>>>
>>
>> I like the idea, thanks! I think, I'll try to make a patch.
>>
>> But you are right, unfortunately there more cases, at least, pointed by
>> Greg
>>
>> error_append_socket_sockfd_hint and
>> error_append_security_model_hint
>>
>> Which don't free error..
> 
> Neither do error_append_hint() and error_prepend().
> 
> For anything named error_append_FOO_hint() or error_prepend_FOO(),
> confusion seems unlikely.
> 
>>                           But if they append hint, they should always be called
>> on wrapped errp, accordingly to the problem about fatal_error, so they may
>> be converted to Error *err too.. Hmm, I should think about the script to find
>> such functions.
> 
> I figure I better read more of your series before I comment on this
> thought.
> 

Me trying to find more such functions:

script:
# cat ../up-fix-error_append_hint/find.py
#!/usr/bin/env python
import re
import sys

ret_type = r'^[^{};#]+( |\*|\*\*)'
name = r'(?P<name>\w+)'
args = r'\([^{};#]*Error \*\*errp[^{};#]*\)'
body_before_errp = r'((?<!errp)[^}]|(?<!^)})*'

caller = '(if ?|assert|' \
           'error_(v?prepend|get_pretty|append_hint|free|free_or_abort)|' \
           '(warn|error)_reportf?_err)'

# Match 'caller(errp', 'caller(*errp', 'errp ?'
access_errp = '(' + caller + r'\(\*?errp|errp \?)'

r = re.compile(ret_type + name + args + '\s*^\{' + body_before_errp + access_errp, re.M)

with open(sys.argv[1]) as f:
     text = f.read()

for m in r.finditer(text):
     print(m.groupdict()['name'])


search:
# git ls-files | grep '\.\(h\|c\)$' | while read f; do ../up-fix-error_append_hint/find.py $f; done
vmdk_co_create_opts_cb
error_append_security_model_hint
error_append_socket_sockfd_hint
qemu_file_get_error_obj
hmp_handle_error
qbus_list_bus
qbus_list_dev
kvmppc_hint_smt_possible
vnc_client_io_error
error_handle_fatal
error_setv
error_prepend
error_setg_win32_internal
error_free_or_abort

vmdk_co_create_opts_cb and qemu_file_get_error_obj are false positives I think


-- 
Best regards,
Vladimir

Re: [PATCH v4 01/31] errp: rename errp to errp_in where it is IN-argument

[Markus Armbruster]

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

> 08.10.2019 15:05, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 
>>> 08.10.2019 12:08, Markus Armbruster wrote:
>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>
>>>>> Error **errp is almost always OUT-argument: it's assumed to be NULL, or
>>>>> pointer to NULL-initialized pointer, or pointer to error_abort or
>>>>> error_fatal, for callee to report error.
>>>>
>>>> Yes.
>>>>
>>>>> But very few functions instead get Error **errp as IN-argument:
>>>>> it's assumed to be set, and callee should clean it.
>>>>
>>>> What do you mean by "callee should clean"?  Let's see below.
>>>>
>>>>> In such cases, rename errp to errp_in.
>>>>
>>>> I acknowledge that errp arguments that don't have the usual meaning can
>>>> be confusing.
>>>>
>>>> Naming can help, comments can help, but perhaps we can tweak the code to
>>>> avoid the problem instead.  Let's see:
>>>>
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> [...]
>>>> We can avoid the confusing Error **errp in all three cases by peeling
>>>> off an indirection.  What do you think?
>>>>
>>>
>>> I like the idea, thanks! I think, I'll try to make a patch.
>>>
>>> But you are right, unfortunately there more cases, at least, pointed by
>>> Greg
>>>
>>> error_append_socket_sockfd_hint and
>>> error_append_security_model_hint
>>>
>>> Which don't free error..
>> 
>> Neither do error_append_hint() and error_prepend().
>> 
>> For anything named error_append_FOO_hint() or error_prepend_FOO(),
>> confusion seems unlikely.
>> 
>>>                           But if they append hint, they should always be called
>>> on wrapped errp, accordingly to the problem about fatal_error, so they may
>>> be converted to Error *err too.. Hmm, I should think about the script to find
>>> such functions.
>> 
>> I figure I better read more of your series before I comment on this
>> thought.
>> 
>
> Me trying to find more such functions:
>
> script:
> # cat ../up-fix-error_append_hint/find.py
> #!/usr/bin/env python
> import re
> import sys
>
> ret_type = r'^[^{};#]+( |\*|\*\*)'
> name = r'(?P<name>\w+)'
> args = r'\([^{};#]*Error \*\*errp[^{};#]*\)'
> body_before_errp = r'((?<!errp)[^}]|(?<!^)})*'
>
> caller = '(if ?|assert|' \
>            'error_(v?prepend|get_pretty|append_hint|free|free_or_abort)|' \
>            '(warn|error)_reportf?_err)'
>
> # Match 'caller(errp', 'caller(*errp', 'errp ?'
> access_errp = '(' + caller + r'\(\*?errp|errp \?)'
>
> r = re.compile(ret_type + name + args + '\s*^\{' + body_before_errp + access_errp, re.M)
>
> with open(sys.argv[1]) as f:
>      text = f.read()
>
> for m in r.finditer(text):
>      print(m.groupdict()['name'])
>
>
> search:
> # git ls-files | grep '\.\(h\|c\)$' | while read f; do ../up-fix-error_append_hint/find.py $f; done
> vmdk_co_create_opts_cb

Forwards errp to vmdk_create_extent().

Also asserts errp == NULL, which looks suspicious.  Not your problem.

> error_append_security_model_hint
> error_append_socket_sockfd_hint

Convenience functions to append a canned hint with error_append_hint().
Their name makes confusion unlikely.

> qemu_file_get_error_obj

Returns an error object in an unusual way: error_copy() instead of
error_setg().

Suspicious-looking qemu_file_set_error_obj() nearby: it either stores
@err in @f, or reports it to stderr / current monitor.  Not your
problem.

> hmp_handle_error

Covered by your patch, already discussed.

> qbus_list_bus
> qbus_list_dev

Convenience functions to append hints with error_append_hint().
Function names do not hint at that (pardon the pun).

> kvmppc_hint_smt_possible

Convenience function to append hints with error_append_hint().  Function
name hints weakly.

> vnc_client_io_error

Covered by your patch, already discussed.

> error_handle_fatal
> error_setv
> error_prepend
> error_setg_win32_internal
> error_free_or_abort

Let's not worry about error.c itself.

> vmdk_co_create_opts_cb and qemu_file_get_error_obj are false positives I think

Agree.

Re: [PATCH v4 01/31] errp: rename errp to errp_in where it is IN-argument

[Markus Armbruster]

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

> 08.10.2019 12:08, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
[...]
>>> diff --git a/util/error.c b/util/error.c
>>> index d4532ce318..b3ff3832d6 100644
>>> --- a/util/error.c
>>> +++ b/util/error.c
>>> @@ -271,11 +271,11 @@ void error_free(Error *err)
>>>       }
>>>   }
>>>   
>>> -void error_free_or_abort(Error **errp)
>>> +void error_free_or_abort(Error **errp_in)
>>>   {
>>> -    assert(errp && *errp);
>>> -    error_free(*errp);
>>> -    *errp = NULL;
>>> +    assert(errp_in && *errp_in);
>>> +    error_free(*errp_in);
>>> +    *errp_in = NULL;
>>>   }
>>>   
>>>   void error_propagate(Error **dst_errp, Error *local_err)
>> 
>> This functions frees the error.  It leaves nothing for the caller to
>> clean up.
>> 
>> All callers pass &ERR, where ERR is a local variable.  We can peel off
>> an indirection.
>
>
> But if we drop indirection, we'll have to set local variable to NULL by
> hand. Is it good?
>
> Look at test_keyval_parse_list() for example: it uses local err object
> several times, so it depends on the fact that error_free_or_abort
> sets pointer to NULL.

You're right, peeling off the indirection would make
error_free_or_abort() worse.

It's a convenience function for tests.  Confusion seems unlikely to me.
Let's not worry about it.

Re: [PATCH v4 04/31] error: auto propagated local_err

[Markus Armbruster]

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

> 09.10.2019 11:04, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 
>>> Here is introduced ERRP_AUTO_PROPAGATE macro, to be used at start of
>>> functions with errp OUT parameter.
>>>
>>> It has three goals:
>>>
>>> 1. Fix issue with error_fatal & error_prepend/error_append_hint: user
>>> can't see this additional information, because exit() happens in
>>> error_setg earlier than information is added. [Reported by Greg Kurz]
>> 
>> Done in PATCH 07-31.
>> 
>>> 2. Fix issue with error_abort & error_propagate: when we wrap
>>> error_abort by local_err+error_propagate, resulting coredump will
>>> refer to error_propagate and not to the place where error happened.
>>> (the macro itself doesn't fix the issue, but it allows to [3.] drop all
>>> local_err+error_propagate pattern, which will definitely fix the issue)
>>> [Reported by Kevin Wolf]
>>>
>>> 3. Drop local_err+error_propagate pattern, which is used to workaround
>>> void functions with errp parameter, when caller wants to know resulting
>>> status. (Note: actually these functions could be merely updated to
>>> return int error code).
>> 
>> Not done.  Can you prototype this part?  A few manually done examples
>> would give us an idea how the complete solution would look like.  A
>> (semi-)automated complete conversion of a subsystem would additionally
>> give us an idea how to actually do the conversion.
>> 
>> We can discuss applying ERRP_AUTO_PROPAGATE() as a bug fix for 1., and
>> leave 2. and 3. for later.  Feels like a half-done job to me.  We've got
>> too many of those in the tree already.  Dunno.
>> 
>
> If update everything it's about 90-140 patches.
>
> The whole thing was done in "[RFC v2 0/9] error: auto propagated local_err"
> <20190923161231.22028-1-vsementsov@virtuozzo.com>
> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg05157.html
>
> - it was so huge, that I decided to postpone 2 and 3...

Suggest to post unsplit patches as RFC.  This lets us see how the macro
gets used, and can also help us decide on a sensible split.  The
automatic split isn't quite right in places, and feels excessively
fine-grained to me.

Re: [PATCH v4 07/31] s390: Fix error_append_hint/error_prepend usage

[Vladimir Sementsov-Ogievskiy]

09.10.2019 10:42, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 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.
>>
>> This commit (together with its neighbors) was generated by
>>
>> git grep -l 'error_\(append_hint\|prepend\)(errp' | while read f; do \
>> spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci \
>> --in-place $f; done
> 
> Make that:
> 
>      $ spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff `git grep -l 'error_\(append_hint\|prepend\)(errp' \*.[ch]`
> 
> Adding --macro-file is essential, as Eric noted.  Without it, we miss
> qcow2_store_persistent_dirty_bitmaps() in PATCH 23 and
> nbd_negotiate_send_rep_verr() in PATCH 29.  There should be a way to
> make spatch warn when it gives up parsing, but I can't find it right
> now.
> 
> Avoiding the loop is just for speed and simplicity.

Seems without loop it hangs or at least works slower. And loop provides a kind of progress, so I'll keep the loop.

> 
> --no-show-diff goes well with --in-place.
> 
> The even simpler
> 
>      $ spatch --sp-file scripts/coccinelle/fix-error-add-info.cocci --macro-file scripts/cocci-macro-file.h --in-place --no-show-diff --use-gitgrep
> 
> misses include/block/nbd.h's nbd_read() in PATCH 23 somehow.
> 
> I recommend to add the spatch invocation to the coccinelle script's
> commit message, too [PATCH 05].
> 
> [...]
> 


-- 
Best regards,
Vladimir