[PATCH v3 00/25] error: auto propagated local_err

[PATCH v3 00/25] 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: user can't see these
hints, because exit() happens in error_setg earlier than hint is
appended. [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.

v3: plan B: fix only error_append_hint users, as a first step.
01: - new errp-based API dropped, so now patch is smaller.
    - slightly adjust commit message
    - keep Eric's r-b
02: - no new API, so just open-code fix
    - keep Eric's r-b
03: - simplify the code [Eric]
04: - grammar and wording
    - drop extra unused variable
others: new

Vladimir Sementsov-Ogievskiy (25):
  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 coccinelle script to fix error_append_hint usage
  python: add commit-per-subsystem.py
  s390: Fix error_append_hint usage
  ARM TCG CPUs: Fix error_append_hint usage
  PowerPC TCG CPUs: Fix error_append_hint usage
  arm: Fix error_append_hint usage
  SmartFusion2: Fix error_append_hint usage
  PCI: Fix error_append_hint usage
  SCSI: Fix error_append_hint usage
  USB: Fix error_append_hint usage
  VFIO: Fix error_append_hint usage
  virtio: Fix error_append_hint usage
  virtio-9p: Fix error_append_hint usage
  block: Fix error_append_hint usage
  chardev: Fix error_append_hint usage
  cmdline: Fix error_append_hint usage
  QOM: Fix error_append_hint usage
  Migration: Fix error_append_hint usage
  Sockets: Fix error_append_hint usage
  nbd: Fix error_append_hint usage
  PVRDMA: Fix error_append_hint usage

 include/monitor/hmp.h                         |  2 +-
 include/qapi/error.h                          | 37 +++++++++-
 ui/vnc.h                                      |  2 +-
 block/backup.c                                |  1 +
 block/dirty-bitmap.c                          |  1 +
 block/file-posix.c                            |  3 +
 block/gluster.c                               |  2 +
 block/qcow.c                                  |  1 +
 block/qcow2.c                                 |  1 +
 block/vhdx-log.c                              |  1 +
 block/vpc.c                                   |  1 +
 chardev/spice.c                               |  1 +
 hw/9pfs/9p-local.c                            |  1 +
 hw/9pfs/9p-proxy.c                            |  1 +
 hw/arm/msf2-soc.c                             |  1 +
 hw/arm/virt.c                                 |  2 +
 hw/core/loader-fit.c                          |  1 +
 hw/intc/arm_gicv3_kvm.c                       |  1 +
 hw/misc/msf2-sysreg.c                         |  1 +
 hw/pci-bridge/pcie_root_port.c                |  1 +
 hw/ppc/mac_newworld.c                         |  1 +
 hw/ppc/spapr.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/usb/ccid-card-emulated.c                   |  1 +
 hw/vfio/common.c                              |  2 +
 hw/vfio/pci.c                                 |  2 +
 hw/virtio/virtio-pci.c                        |  2 +
 migration/migration.c                         |  1 +
 monitor/hmp-cmds.c                            |  8 +--
 nbd/client.c                                  |  1 +
 nbd/server.c                                  |  1 +
 net/net.c                                     | 17 ++---
 qdev-monitor.c                                |  2 +
 target/ppc/kvm.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 +++++++++++++++++++
 .../fix-error_append_hint-usage.cocci         | 25 +++++++
 44 files changed, 198 insertions(+), 26 deletions(-)
 create mode 100755 python/commit-per-subsystem.py
 create mode 100644 scripts/coccinelle/fix-error_append_hint-usage.cocci

CC: John Snow <jsnow@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
CC: Fam Zheng <fam@euphon.net>
CC: Jeff Cody <codyprime@gmail.com>
CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Greg Kurz <groug@kaod.org>
CC: Subbaraya Sundeep <sundeep.lkml@gmail.com>
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Paul Burton <pburton@wavecomp.com>
CC: Aleksandar Rikalo <arikalo@wavecomp.com>
CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
CC: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Yuval Shaia <yuval.shaia@oracle.com>
CC: Cornelia Huck <cohuck@redhat.com>
CC: Eric Farman <farman@linux.ibm.com>
CC: Richard Henderson <rth@twiddle.net>
CC: David Hildenbrand <david@redhat.com>
CC: Halil Pasic <pasic@linux.ibm.com>
CC: Christian Borntraeger <borntraeger@de.ibm.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Alex Williamson <alex.williamson@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
CC: Juan Quintela <quintela@redhat.com>
CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Jason Wang <jasowang@redhat.com>
CC: "Daniel P. Berrangé" <berrange@redhat.com>
CC: Eduardo Habkost <ehabkost@redhat.com>
CC: qemu-block@nongnu.org
CC: qemu-devel@nongnu.org
CC: integration@gluster.org
CC: qemu-arm@nongnu.org
CC: qemu-ppc@nongnu.org
CC: qemu-s390x@nongnu.org

-- 
2.21.0

[PATCH v3 01/25] 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 v3 02/25] hw/core/loader-fit: fix freeing errp in fit_load_fdt

[Vladimir Sementsov-Ogievskiy]

fit_load_fdt forget to zero errp. Fix it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 hw/core/loader-fit.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
index 953b16bc82..11e4fad595 100644
--- a/hw/core/loader-fit.c
+++ b/hw/core/loader-fit.c
@@ -201,6 +201,7 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
     if (err == -ENOENT) {
         load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
         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 v3 03/25] 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>
---
 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 v3 04/25] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

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

It has three goals:

1. Fix issue with error_fatal & error_append_hint: user can't see these
hints, because exit() happens in error_setg earlier than hint is
appended. [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: John Snow <jsnow@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
CC: Fam Zheng <fam@euphon.net>
CC: Jeff Cody <codyprime@gmail.com>
CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Greg Kurz <groug@kaod.org>
CC: Subbaraya Sundeep <sundeep.lkml@gmail.com>
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Paul Burton <pburton@wavecomp.com>
CC: Aleksandar Rikalo <arikalo@wavecomp.com>
CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
CC: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Yuval Shaia <yuval.shaia@oracle.com>
CC: Cornelia Huck <cohuck@redhat.com>
CC: Eric Farman <farman@linux.ibm.com>
CC: Richard Henderson <rth@twiddle.net>
CC: David Hildenbrand <david@redhat.com>
CC: Halil Pasic <pasic@linux.ibm.com>
CC: Christian Borntraeger <borntraeger@de.ibm.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Alex Williamson <alex.williamson@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
CC: Juan Quintela <quintela@redhat.com>
CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Jason Wang <jasowang@redhat.com>
CC: "Daniel P. Berrangé" <berrange@redhat.com>
CC: Eduardo Habkost <ehabkost@redhat.com>
CC: qemu-block@nongnu.org
CC: qemu-devel@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 | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 9376f59c35..fb41f7a790 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -322,6 +322,41 @@ 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_FUNCTION_BEGIN
+ *
+ * This macro is created to be the first line of a function with Error **errp
+ * parameter.
+ *
+ * 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_FUNCTION_BEGIN() \
+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 v3 05/25] scripts: add coccinelle script to fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

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

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

CC: John Snow <jsnow@redhat.com>
CC: Kevin Wolf <kwolf@redhat.com>
CC: Max Reitz <mreitz@redhat.com>
CC: Fam Zheng <fam@euphon.net>
CC: Jeff Cody <codyprime@gmail.com>
CC: "Marc-André Lureau" <marcandre.lureau@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: Greg Kurz <groug@kaod.org>
CC: Subbaraya Sundeep <sundeep.lkml@gmail.com>
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Paul Burton <pburton@wavecomp.com>
CC: Aleksandar Rikalo <arikalo@wavecomp.com>
CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
CC: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: Yuval Shaia <yuval.shaia@oracle.com>
CC: Cornelia Huck <cohuck@redhat.com>
CC: Eric Farman <farman@linux.ibm.com>
CC: Richard Henderson <rth@twiddle.net>
CC: David Hildenbrand <david@redhat.com>
CC: Halil Pasic <pasic@linux.ibm.com>
CC: Christian Borntraeger <borntraeger@de.ibm.com>
CC: Gerd Hoffmann <kraxel@redhat.com>
CC: Alex Williamson <alex.williamson@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Michael Roth <mdroth@linux.vnet.ibm.com>
CC: Juan Quintela <quintela@redhat.com>
CC: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
CC: Eric Blake <eblake@redhat.com>
CC: Jason Wang <jasowang@redhat.com>
CC: "Daniel P. Berrangé" <berrange@redhat.com>
CC: Eduardo Habkost <ehabkost@redhat.com>
CC: qemu-block@nongnu.org
CC: qemu-devel@nongnu.org
CC: integration@gluster.org
CC: qemu-arm@nongnu.org
CC: qemu-ppc@nongnu.org
CC: qemu-s390x@nongnu.org

 .../fix-error_append_hint-usage.cocci         | 25 +++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 scripts/coccinelle/fix-error_append_hint-usage.cocci

diff --git a/scripts/coccinelle/fix-error_append_hint-usage.cocci b/scripts/coccinelle/fix-error_append_hint-usage.cocci
new file mode 100644
index 0000000000..327fe6098c
--- /dev/null
+++ b/scripts/coccinelle/fix-error_append_hint-usage.cocci
@@ -0,0 +1,25 @@
+@rule0@
+// Add invocation to errp-functions
+identifier fn;
+@@
+
+ fn(..., Error **errp, ...)
+ {
++   ERRP_FUNCTION_BEGIN();
+    <+...
+    error_append_hint(errp, ...);
+    ...+>
+ }
+
+@@
+// Drop doubled invocation
+identifier rule0.fn;
+@@
+
+ fn(...)
+{
+    ERRP_FUNCTION_BEGIN();
+-   ERRP_FUNCTION_BEGIN();
+    ...
+}
+
-- 
2.21.0

[PATCH v3 06/25] 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>
---
 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 v3 07/25] s390: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

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

diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 0c5a5b60bd..c445dca2e1 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_FUNCTION_BEGIN();
     unsigned int cssid, ssid, devid;
     char dev_path[PATH_MAX] = {0}, *tmp;
 
-- 
2.21.0

[PATCH v3 08/25] ARM TCG CPUs: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

Reported-by: 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..21e4bdfb72 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_FUNCTION_BEGIN();
     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..e79a46b0b3 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_FUNCTION_BEGIN();
     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_FUNCTION_BEGIN();
     VirtMachineState *vms = VIRT_MACHINE(obj);
 
     if (!strcmp(value, "smmuv3")) {
-- 
2.21.0

[PATCH v3 09/25] PowerPC TCG CPUs: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

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

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index c5bbcc7433..939fdd258f 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_FUNCTION_BEGIN();
     Core99MachineState *cms = CORE99_MACHINE(obj);
 
     if (!strcmp(value, "cuda")) {
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 08a2a5a770..d0dd8aaa22 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_FUNCTION_BEGIN();
     SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     MachineState *ms = MACHINE(spapr);
     int vcpu_id;
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7b71ad7c74..172d6689d0 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_FUNCTION_BEGIN();
     /* 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..aacfe194e2 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_FUNCTION_BEGIN();
     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_FUNCTION_BEGIN();
     int i;
     GString *g;
     char *s;
-- 
2.21.0

[PATCH v3 10/25] arm: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

Reported-by: 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..42487fc62b 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_FUNCTION_BEGIN();
     GICv3State *s = KVM_ARM_GICV3(dev);
     KVMARMGICv3Class *kgc = KVM_ARM_GICV3_GET_CLASS(s);
     bool multiple_redist_region_allowed;
-- 
2.21.0

[PATCH v3 11/25] SmartFusion2: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

Reported-by: 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..a2f5310fe0 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_FUNCTION_BEGIN();
     MSF2SysregState *s = MSF2_SYSREG(dev);
 
     if ((s->apb0div > 32 || !is_power_of_2(s->apb0div))
-- 
2.21.0

[PATCH v3 12/25] PCI: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

Reported-by: 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..a0aacad711 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_FUNCTION_BEGIN();
     PCIEPort *p = PCIE_PORT(d);
     PCIESlot *s = PCIE_SLOT(d);
     PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(d);
-- 
2.21.0

[PATCH v3 13/25] SCSI: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

Reported-by: 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 +
 2 files changed, 2 insertions(+)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 915641a0f1..72ac308b6c 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_FUNCTION_BEGIN();
     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..e955f4e0a5 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_FUNCTION_BEGIN();
     int rc;
     int sg_version;
     struct sg_scsi_id scsiid;
-- 
2.21.0

[PATCH v3 14/25] USB: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

Reported-by: 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..9a2ea129dd 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_FUNCTION_BEGIN();
     EmulatedState *card = EMULATED_CCID_CARD(base);
     VCardEmulError ret;
     const EnumTable *ptable;
-- 
2.21.0

[PATCH v3 15/25] VFIO: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

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

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 3e03c495d8..d08276b1e6 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1437,6 +1437,7 @@ static void vfio_disconnect_container(VFIOGroup *group)
 
 VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp)
 {
+    ERRP_FUNCTION_BEGIN();
     VFIOGroup *group;
     char path[32];
     struct vfio_group_status status = { .argsz = sizeof(status) };
@@ -1526,6 +1527,7 @@ void vfio_put_group(VFIOGroup *group)
 int vfio_get_device(VFIOGroup *group, const char *name,
                     VFIODevice *vbasedev, Error **errp)
 {
+    ERRP_FUNCTION_BEGIN();
     struct vfio_device_info dev_info = { .argsz = sizeof(dev_info) };
     int ret, fd;
 
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index c5e6fe61cb..57208c7075 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2469,6 +2469,7 @@ int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
 
 static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
 {
+    ERRP_FUNCTION_BEGIN();
     VFIODevice *vbasedev = &vdev->vbasedev;
     struct vfio_region_info *reg_info;
     struct vfio_irq_info irq_info = { .argsz = sizeof(irq_info) };
@@ -2700,6 +2701,7 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice *vdev)
 
 static void vfio_realize(PCIDevice *pdev, Error **errp)
 {
+    ERRP_FUNCTION_BEGIN();
     VFIOPCIDevice *vdev = PCI_VFIO(pdev);
     VFIODevice *vbasedev_iter;
     VFIOGroup *group;
-- 
2.21.0

[PATCH v3 16/25] virtio: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

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

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index c6b47a9c73..291044c4e4 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_FUNCTION_BEGIN();
     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_FUNCTION_BEGIN();
     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 v3 17/25] virtio-9p: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

Reported-by: 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 +
 2 files changed, 2 insertions(+)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 08e673a79c..6b1a805575 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_FUNCTION_BEGIN();
     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..01ff411aea 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_FUNCTION_BEGIN();
     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"
-- 
2.21.0

[PATCH v3 18/25] block: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

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

diff --git a/block/backup.c b/block/backup.c
index 763f0d7ff6..8cb0201833 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_FUNCTION_BEGIN();
     int ret;
     BlockDriverInfo bdi;
 
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 134e0c9a0c..eba47490ea 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_FUNCTION_BEGIN();
     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..46818fe4fc 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_FUNCTION_BEGIN();
     BDRVRawState *s = bs->opaque;
     char *buf;
     size_t max_align = MAX(MAX_BLOCKSIZE, getpagesize());
@@ -817,6 +818,7 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
                                 uint64_t new_perm, uint64_t new_shared,
                                 Error **errp)
 {
+    ERRP_FUNCTION_BEGIN();
     BDRVRawState *s = bs->opaque;
     int ret = 0;
     Error *local_err = NULL;
@@ -2232,6 +2234,7 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs)
 static int coroutine_fn
 raw_co_create(BlockdevCreateOptions *options, Error **errp)
 {
+    ERRP_FUNCTION_BEGIN();
     BlockdevCreateOptionsFile *file_opts;
     Error *local_err = NULL;
     int fd;
diff --git a/block/gluster.c b/block/gluster.c
index 64028b2cba..7023807326 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_FUNCTION_BEGIN();
     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_FUNCTION_BEGIN();
     int ret;
     if (filename) {
         ret = qemu_gluster_parse_uri(gconf, filename);
diff --git a/block/qcow.c b/block/qcow.c
index 5bdf72ba33..33a004350b 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_FUNCTION_BEGIN();
     BDRVQcowState *s = bs->opaque;
     unsigned int len, i, shift;
     int ret;
diff --git a/block/qcow2.c b/block/qcow2.c
index 4d16393e61..d9aac1186d 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_FUNCTION_BEGIN();
     BDRVQcow2State *s = bs->opaque;
     unsigned int len, i;
     int ret = 0;
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index fdd3a7adc3..24e5efb46c 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_FUNCTION_BEGIN();
     int ret = 0;
     VHDXHeader *hdr;
     VHDXLogSequence logs = { 0 };
diff --git a/block/vpc.c b/block/vpc.c
index 5cd3890780..b12a2d964a 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_FUNCTION_BEGIN();
     BlockdevCreateOptionsVpc *vpc_opts;
     BlockBackend *blk = NULL;
     BlockDriverState *bs = NULL;
-- 
2.21.0

[PATCH v3 19/25] chardev: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

Reported-by: 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..1421c85464 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_FUNCTION_BEGIN();
     ChardevSpiceChannel *spicevmc = backend->u.spicevmc.data;
     const char *type = spicevmc->type;
     const char **psubtype = spice_server_char_device_recognized_subtypes();
-- 
2.21.0

[PATCH v3 20/25] cmdline: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

Reported-by: 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..7136d83b1d 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_FUNCTION_BEGIN();
     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_FUNCTION_BEGIN();
     QemuOpts *opts = NULL;
 
     if (id) {
-- 
2.21.0

[PATCH v3 21/25] QOM: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

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

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 148df9cacf..d0129411fb 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_FUNCTION_BEGIN();
     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_FUNCTION_BEGIN();
     BusChild *kid;
     const char *sep = " ";
 
-- 
2.21.0

[PATCH v3 22/25] Migration: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

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

diff --git a/migration/migration.c b/migration/migration.c
index 01863a95f5..031b85e670 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_FUNCTION_BEGIN();
     MigrationCapabilityStatusList *cap;
     bool old_postcopy_cap;
     MigrationIncomingState *mis = migration_incoming_get_current();
-- 
2.21.0

[PATCH v3 23/25] Sockets: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

Reported-by: 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 98ff3a1cce..fb21b78369 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -860,6 +860,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
                              int num,
                              Error **errp)
 {
+    ERRP_FUNCTION_BEGIN();
     struct sockaddr_un un;
     int sock, fd;
     char *pathbuf = NULL;
@@ -935,6 +936,7 @@ err:
 
 static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
 {
+    ERRP_FUNCTION_BEGIN();
     struct sockaddr_un un;
     int sock, rc;
     size_t pathlen;
-- 
2.21.0

[PATCH v3 24/25] nbd: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

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

diff --git a/nbd/client.c b/nbd/client.c
index b9dc829175..4d90a26c00 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -153,6 +153,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_FUNCTION_BEGIN();
     g_autofree char *msg = NULL;
 
     if (!(reply->type & (1 << 31))) {
diff --git a/nbd/server.c b/nbd/server.c
index 28c3c8be85..09565ad8dc 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1616,6 +1616,7 @@ void nbd_export_close(NBDExport *exp)
 
 void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
 {
+    ERRP_FUNCTION_BEGIN();
     if (mode == NBD_SERVER_REMOVE_MODE_HARD || QTAILQ_EMPTY(&exp->clients)) {
         nbd_export_close(exp);
         return;
-- 
2.21.0

[PATCH v3 25/25] PVRDMA: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
Otherwise hint will not be appended in case of errp == &fatal_err
(program will exit before error_append_hint() call). Fix such cases.

This commit (together with its neighbors) was generated by

git grep -l 'error_append_hint(errp' | while read f; do \
spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
--in-place $f; done

and then

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

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

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

Reported-by: 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..3cc02311bf 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_FUNCTION_BEGIN();
     int rc = 0;
     PVRDMADev *dev = PVRDMA_DEV(pdev);
     Object *memdev_root;
-- 
2.21.0

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

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1306 bytes --]

On 9/24/19 3:08 PM, Vladimir Sementsov-Ogievskiy wrote:
> fit_load_fdt forget to zero errp. Fix it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  hw/core/loader-fit.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
> index 953b16bc82..11e4fad595 100644
> --- a/hw/core/loader-fit.c
> +++ b/hw/core/loader-fit.c
> @@ -201,6 +201,7 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>      if (err == -ENOENT) {
>          load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>          error_free(*errp);
> +        *errp = NULL;

Actually, let's drop my R-b - I think we have a bigger bug here.  We are
blindly dereferencing *errp even if the caller passed in NULL.  The
correct way to write this function requires either the use of local_err
or the addition of auto-propagation.

(In v2, you still had this bug - your addition of error_free_errp(errp)
would still blindly dereference *errp, unless you tweak the
implementation of error_free_errp to tolerate a NULL pointer input)

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 03/25] net/net: fix local variable shadowing in net_client_init

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 461 bytes --]

On 9/24/19 3:08 PM, Vladimir Sementsov-Ogievskiy wrote:
> Don't shadow Error *err: it's a bad thing.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  net/net.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 04/25] error: auto propagated local_err

[Eric Blake]

On 9/24/19 3:08 PM, Vladimir Sementsov-Ogievskiy wrote:
> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of
> functions with errp parameter.
> 
> It has three goals:
> 
> 1. Fix issue with error_fatal & error_append_hint: user can't see these
> hints, because exit() happens in error_setg earlier than hint is
> appended. [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>
> ---
> 


> +/*
> + * ERRP_FUNCTION_BEGIN
> + *
> + * This macro is created to be the first line of a function with Error **errp
> + * parameter.

Since you're going with the reduced scope of only touching functions
with error_append_hint, we may want to tweak this sentence to call out
only the functions that REQUIRE the use of this macro (if they contain
*errp or error_apend_hint), while mentioning that it is still safe to
use on any function with an errp parameter.

I'm hoping Markus will be able to chime in with his preferences soon.


> + *
> + * 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_FUNCTION_BEGIN() \
> +g_auto(ErrorPropagator) __auto_errp_prop = {.errp = errp}; \
> +errp = ((errp == NULL || *errp == error_fatal) ? \
> +    &__auto_errp_prop.local_err : errp)
> +

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 v3 05/25] scripts: add coccinelle script to fix error_append_hint usage

[Eric Blake]

On 9/24/19 3:08 PM, Vladimir Sementsov-Ogievskiy wrote:
> error_append_hint will not work, if errp == &fatal_error, as program
> will exit before error_append_hint call. Fix this by use of special
> macro ERRP_FUNCTION_BEGIN.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

With the approach of a partial cleanup (rather than globally enforcing
it for all functions with errp parameter), we'll probably be rerunning
this Coccinelle script regularly, to track down any regressions.


> +++ b/scripts/coccinelle/fix-error_append_hint-usage.cocci
> @@ -0,0 +1,25 @@
> +@rule0@
> +// Add invocation to errp-functions
> +identifier fn;
> +@@
> +
> + fn(..., Error **errp, ...)
> + {
> ++   ERRP_FUNCTION_BEGIN();
> +    <+...
> +    error_append_hint(errp, ...);
> +    ...+>
> + }

Does not catch the case that we want to also use the macro for any use
of *errp, but we can augment that later.

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

This is smaller than the script you posted in v2, and thus I'm a bit
more confident in stating that it looks correct and idempotent.

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 v3 05/25] scripts: add coccinelle script to fix error_append_hint usage

[Eric Blake]

On 9/24/19 3:08 PM, Vladimir Sementsov-Ogievskiy wrote:
> error_append_hint will not work, if errp == &fatal_error, as program
> will exit before error_append_hint call. Fix this by use of special
> macro ERRP_FUNCTION_BEGIN.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

>  .../fix-error_append_hint-usage.cocci         | 25 +++++++++++++++++++
>  1 file changed, 25 insertions(+)
>  create mode 100644 scripts/coccinelle/fix-error_append_hint-usage.cocci
> 

> +}
> +
> 

Why are you creating a file with a trailing blank line?

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

Re: [PATCH v3 18/25] block: Fix error_append_hint usage

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1517 bytes --]

On 9/24/19 3:08 PM, Vladimir Sementsov-Ogievskiy wrote:
> If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
> Otherwise hint will not be appended in case of errp == &fatal_err
> (program will exit before error_append_hint() call). Fix such cases.
> 
> This commit (together with its neighbors) was generated by
> 
> git grep -l 'error_append_hint(errp' | while read f; do \
> spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
> --in-place $f; done
> 
> and then
> 
> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
> 
> (auto-msg was a file with this commit message)
> 

Your automation is cool!

> 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>
> ---
>  block/backup.c       | 1 +
>  block/dirty-bitmap.c | 1 +
>  block/file-posix.c   | 3 +++
>  block/gluster.c      | 2 ++
>  block/qcow.c         | 1 +
>  block/qcow2.c        | 1 +
>  block/vhdx-log.c     | 1 +
>  block/vpc.c          | 1 +
>  8 files changed, 11 insertions(+)

$ git grep -p 'error_append_hint(errp' block/ | grep '\.c=' | wc -l

produces 11 hits, very nicely matching up with your patch.

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 23/25] Sockets: Fix error_append_hint usage

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1229 bytes --]

On 9/24/19 3:09 PM, Vladimir Sementsov-Ogievskiy wrote:
> If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
> Otherwise hint will not be appended in case of errp == &fatal_err
> (program will exit before error_append_hint() call). Fix such cases.
> 
> This commit (together with its neighbors) was generated by
> 
> git grep -l 'error_append_hint(errp' | while read f; do \
> spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
> --in-place $f; done
> 
> and then
> 
> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
> 
> (auto-msg was a file with this commit message)
> 
> Still, for backporting it may be more comfortable to use only the first
> command and then do one huge commit.
> 
> Reported-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  util/qemu-sockets.c | 2 ++
>  1 file changed, 2 insertions(+)

git grep -p 'error_append_hint(errp' util/*sockets*

nicely shows these same two functions.

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 24/25] nbd: Fix error_append_hint usage

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 2734 bytes --]

On 9/24/19 3:09 PM, Vladimir Sementsov-Ogievskiy wrote:
> If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
> Otherwise hint will not be appended in case of errp == &fatal_err
> (program will exit before error_append_hint() call). Fix such cases.
> 

Copy-and-pasted, but if you want to tweak the grammar to all of the
patches with identical bodies:

If we want to append a hint to errp, we must use the ERRP_FUNCTION_BEGIN
macro.  Otherwise, the hint will not be appended when errp == &fatal_err
(the program will exit prior to the error_append_hint() call).  Fix such
cases.

> This commit (together with its neighbors) was generated by
> 
> git grep -l 'error_append_hint(errp' | while read f; do \
> spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
> --in-place $f; done
> 
> and then
> 
> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
> 
> (auto-msg was a file with this commit message)
> 
> Still, for backporting it may be more comfortable to use only the first
> command and then do one huge commit.
> 
> Reported-by: Greg Kurz <groug@kaod.org>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

Should the commit-per-subsystem.py script append a distinct CC: line as
long as it is already grouping files by maintainer?

>  nbd/client.c | 1 +
>  nbd/server.c | 1 +
>  2 files changed, 2 insertions(+)

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

It's probably easier to take this entire series through one maintainer
(Markus, since it is error-related), than to have me pick up this patch
by itself through the NBD tree.

> 
> diff --git a/nbd/client.c b/nbd/client.c
> index b9dc829175..4d90a26c00 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -153,6 +153,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_FUNCTION_BEGIN();
>      g_autofree char *msg = NULL;
>  
>      if (!(reply->type & (1 << 31))) {
> diff --git a/nbd/server.c b/nbd/server.c
> index 28c3c8be85..09565ad8dc 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1616,6 +1616,7 @@ void nbd_export_close(NBDExport *exp)
>  
>  void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
>  {
> +    ERRP_FUNCTION_BEGIN();
>      if (mode == NBD_SERVER_REMOVE_MODE_HARD || QTAILQ_EMPTY(&exp->clients)) {
>          nbd_export_close(exp);
>          return;
> 

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Vladimir Sementsov-Ogievskiy]

24.09.2019 23:38, Eric Blake wrote:
> On 9/24/19 3:08 PM, Vladimir Sementsov-Ogievskiy wrote:
>> fit_load_fdt forget to zero errp. Fix it.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
>> ---
>>   hw/core/loader-fit.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>> index 953b16bc82..11e4fad595 100644
>> --- a/hw/core/loader-fit.c
>> +++ b/hw/core/loader-fit.c
>> @@ -201,6 +201,7 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>       if (err == -ENOENT) {
>>           load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>>           error_free(*errp);
>> +        *errp = NULL;
> 
> Actually, let's drop my R-b - I think we have a bigger bug here.  We are
> blindly dereferencing *errp even if the caller passed in NULL.  The
> correct way to write this function requires either the use of local_err
> or the addition of auto-propagation.
> 
> (In v2, you still had this bug - your addition of error_free_errp(errp)
> would still blindly dereference *errp, unless you tweak the
> implementation of error_free_errp to tolerate a NULL pointer input)
> 

Oops, you are right! Still, I think in this case we can

if (errp) {
   error_free(*errp);
   *errp = NULL;
}

-- 
Best regards,
Vladimir

Re: [PATCH v3 24/25] nbd: Fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

25.09.2019 0:12, Eric Blake wrote:
> On 9/24/19 3:09 PM, Vladimir Sementsov-Ogievskiy wrote:
>> If we want append hint to errp, we must use ERRP_FUNCTION_BEGIN macro.
>> Otherwise hint will not be appended in case of errp == &fatal_err
>> (program will exit before error_append_hint() call). Fix such cases.
>>
> 
> Copy-and-pasted, but if you want to tweak the grammar to all of the
> patches with identical bodies:
> 
> If we want to append a hint to errp, we must use the ERRP_FUNCTION_BEGIN
> macro.  Otherwise, the hint will not be appended when errp == &fatal_err
> (the program will exit prior to the error_append_hint() call).  Fix such
> cases.

Will do, thanks.

> 
>> This commit (together with its neighbors) was generated by
>>
>> git grep -l 'error_append_hint(errp' | while read f; do \
>> spatch --sp-file scripts/coccinelle/fix-error_append_hint-usage.cocci \
>> --in-place $f; done
>>
>> and then
>>
>> ./python/commit-per-subsystem.py MAINTAINERS "$(< auto-msg)"
>>
>> (auto-msg was a file with this commit message)
>>
>> Still, for backporting it may be more comfortable to use only the first
>> command and then do one huge commit.
>>
>> Reported-by: Greg Kurz <groug@kaod.org>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
> 
> Should the commit-per-subsystem.py script append a distinct CC: line as
> long as it is already grouping files by maintainer?


Hmm, actually, it was in a first version. But then I decided that:
1. I have to use --cc-cmd to handle first three patches anyway
2. I don't like commit messages with "CC:" inside (any reason to keep this
    information in history forever, increasing false-recipients for those
    who forget to add --suppress-cc=all ?)

So, I decided to use --cc-cmd instead.


> 
>>   nbd/client.c | 1 +
>>   nbd/server.c | 1 +
>>   2 files changed, 2 insertions(+)
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> It's probably easier to take this entire series through one maintainer
> (Markus, since it is error-related), than to have me pick up this patch
> by itself through the NBD tree.

Yes, it simplifies backporting (even backporting something other)

> 
>>
>> diff --git a/nbd/client.c b/nbd/client.c
>> index b9dc829175..4d90a26c00 100644
>> --- a/nbd/client.c
>> +++ b/nbd/client.c
>> @@ -153,6 +153,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_FUNCTION_BEGIN();
>>       g_autofree char *msg = NULL;
>>   
>>       if (!(reply->type & (1 << 31))) {
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 28c3c8be85..09565ad8dc 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -1616,6 +1616,7 @@ void nbd_export_close(NBDExport *exp)
>>   
>>   void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
>>   {
>> +    ERRP_FUNCTION_BEGIN();
>>       if (mode == NBD_SERVER_REMOVE_MODE_HARD || QTAILQ_EMPTY(&exp->clients)) {
>>           nbd_export_close(exp);
>>           return;
>>
> 


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

25.09.2019 10:23, Vladimir Sementsov-Ogievskiy wrote:
> 24.09.2019 23:38, Eric Blake wrote:
>> On 9/24/19 3:08 PM, Vladimir Sementsov-Ogievskiy wrote:
>>> fit_load_fdt forget to zero errp. Fix it.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>   hw/core/loader-fit.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>>> index 953b16bc82..11e4fad595 100644
>>> --- a/hw/core/loader-fit.c
>>> +++ b/hw/core/loader-fit.c
>>> @@ -201,6 +201,7 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>       if (err == -ENOENT) {
>>>           load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>>>           error_free(*errp);
>>> +        *errp = NULL;
>>
>> Actually, let's drop my R-b - I think we have a bigger bug here.  We are
>> blindly dereferencing *errp even if the caller passed in NULL.  The
>> correct way to write this function requires either the use of local_err
>> or the addition of auto-propagation.
>>
>> (In v2, you still had this bug - your addition of error_free_errp(errp)
>> would still blindly dereference *errp, unless you tweak the
>> implementation of error_free_errp to tolerate a NULL pointer input)
>>
> 
> Oops, you are right! Still, I think in this case we can
> 
> if (errp) {
>    error_free(*errp);
>    *errp = NULL;
> }
> 

Hmm, possibly, it should be called not error_free_errp, but just error_unset, to be
correct pair to error_set.

-- 
Best regards,
Vladimir

Re: [PATCH v3 00/25] error: auto propagated local_err

[no-reply]

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



Hi,

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

Type: series
Message-id: 20190924200902.4703-1-vsementsov@virtuozzo.com
Subject: [PATCH v3 00/25] 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 ===

Switched to a new branch 'test'
2821824 PVRDMA: Fix error_append_hint usage
4fed9bd nbd: Fix error_append_hint usage
4dd593b Sockets: Fix error_append_hint usage
4b51e60 Migration: Fix error_append_hint usage
8badd1e QOM: Fix error_append_hint usage
cee12d7 cmdline: Fix error_append_hint usage
9ee000d chardev: Fix error_append_hint usage
9b82769 block: Fix error_append_hint usage
2680a9c virtio-9p: Fix error_append_hint usage
9563cc0 virtio: Fix error_append_hint usage
eaf3c5f VFIO: Fix error_append_hint usage
5a70d12 USB: Fix error_append_hint usage
a7307a2 SCSI: Fix error_append_hint usage
38ab236 PCI: Fix error_append_hint usage
1e3f98e SmartFusion2: Fix error_append_hint usage
4b06bcc arm: Fix error_append_hint usage
1e194b4 PowerPC TCG CPUs: Fix error_append_hint usage
16f7be8 ARM TCG CPUs: Fix error_append_hint usage
38baa0a s390: Fix error_append_hint usage
e596c72 python: add commit-per-subsystem.py
c82b758 scripts: add coccinelle script to fix error_append_hint usage
4c37d5c error: auto propagated local_err
1f5d983 net/net: fix local variable shadowing in net_client_init
118f617 hw/core/loader-fit: fix freeing errp in fit_load_fdt
2493069 errp: rename errp to errp_in where it is IN-argument

=== OUTPUT BEGIN ===
1/25 Checking commit 2493069b7603 (errp: rename errp to errp_in where it is IN-argument)
2/25 Checking commit 118f61787279 (hw/core/loader-fit: fix freeing errp in fit_load_fdt)
3/25 Checking commit 1f5d98328d6c (net/net: fix local variable shadowing in net_client_init)
4/25 Checking commit 4c37d5c6d228 (error: auto propagated local_err)
ERROR: Macros with multiple statements should be enclosed in a do - while loop
#69: FILE: include/qapi/error.h:355:
+#define ERRP_FUNCTION_BEGIN() \
+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, 41 lines checked

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

5/25 Checking commit c82b758af1da (scripts: add coccinelle script to fix error_append_hint usage)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#15: 
new file mode 100644

total: 0 errors, 1 warnings, 25 lines checked

Patch 5/25 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
6/25 Checking commit e596c720393c (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/25 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
7/25 Checking commit 38baa0a318a0 (s390: Fix error_append_hint usage)
8/25 Checking commit 16f7be86eeb1 (ARM TCG CPUs: Fix error_append_hint usage)
9/25 Checking commit 1e194b48e97b (PowerPC TCG CPUs: Fix error_append_hint usage)
10/25 Checking commit 4b06bccfe947 (arm: Fix error_append_hint usage)
11/25 Checking commit 1e3f98eb12d0 (SmartFusion2: Fix error_append_hint usage)
12/25 Checking commit 38ab2364f44b (PCI: Fix error_append_hint usage)
13/25 Checking commit a7307a29159e (SCSI: Fix error_append_hint usage)
14/25 Checking commit 5a70d127c9fc (USB: Fix error_append_hint usage)
15/25 Checking commit eaf3c5f855c8 (VFIO: Fix error_append_hint usage)
16/25 Checking commit 9563cc0b6e37 (virtio: Fix error_append_hint usage)
17/25 Checking commit 2680a9ca88fa (virtio-9p: Fix error_append_hint usage)
18/25 Checking commit 9b8276961550 (block: Fix error_append_hint usage)
19/25 Checking commit 9ee000da2508 (chardev: Fix error_append_hint usage)
20/25 Checking commit cee12d7c8697 (cmdline: Fix error_append_hint usage)
21/25 Checking commit 8badd1ec04da (QOM: Fix error_append_hint usage)
22/25 Checking commit 4b51e60a1e83 (Migration: Fix error_append_hint usage)
23/25 Checking commit 4dd593b9c7df (Sockets: Fix error_append_hint usage)
24/25 Checking commit 4fed9bd83a77 (nbd: Fix error_append_hint usage)
25/25 Checking commit 2821824d98bf (PVRDMA: Fix error_append_hint usage)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190924200902.4703-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 v3 00/25] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

25.09.2019 13:09, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20190924200902.4703-1-vsementsov@virtuozzo.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 

[..]

> 
> === OUTPUT BEGIN ===
> 1/25 Checking commit 2493069b7603 (errp: rename errp to errp_in where it is IN-argument)
> 2/25 Checking commit 118f61787279 (hw/core/loader-fit: fix freeing errp in fit_load_fdt)
> 3/25 Checking commit 1f5d98328d6c (net/net: fix local variable shadowing in net_client_init)
> 4/25 Checking commit 4c37d5c6d228 (error: auto propagated local_err)
> ERROR: Macros with multiple statements should be enclosed in a do - while loop

In general - yes. But this is a very special case and it worth an exception of the rule.

> #69: FILE: include/qapi/error.h:355:
> +#define ERRP_FUNCTION_BEGIN() \
> +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, 41 lines checked



-- 
Best regards,
Vladimir

Re: [PATCH v3 05/25] scripts: add coccinelle script to fix error_append_hint usage

[Vladimir Sementsov-Ogievskiy]

24.09.2019 23:48, Eric Blake wrote:
> On 9/24/19 3:08 PM, Vladimir Sementsov-Ogievskiy wrote:
>> error_append_hint will not work, if errp == &fatal_error, as program
>> will exit before error_append_hint call. Fix this by use of special
>> macro ERRP_FUNCTION_BEGIN.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
> 
> With the approach of a partial cleanup (rather than globally enforcing
> it for all functions with errp parameter), we'll probably be rerunning
> this Coccinelle script regularly, to track down any regressions.
> 
> 
>> +++ b/scripts/coccinelle/fix-error_append_hint-usage.cocci
>> @@ -0,0 +1,25 @@
>> +@rule0@
>> +// Add invocation to errp-functions
>> +identifier fn;
>> +@@
>> +
>> + fn(..., Error **errp, ...)
>> + {
>> ++   ERRP_FUNCTION_BEGIN();
>> +    <+...
>> +    error_append_hint(errp, ...);
>> +    ...+>
>> + }
> 
> Does not catch the case that we want to also use the macro for any use
> of *errp, but we can augment that later.

I don't want to include *errp here, as actually a lot of *errp invocations in
code are correct: they do it if errp is not NULL. So, it's not related to plan B.

Still, I think we forget about error_prepend :)))

I've checked, if I include error_prepend here, series becomes 30 patches, which is
not significantly larger. So I think, I'll cover error_prepend in v4.

> 
>> +
>> +@@
>> +// Drop doubled invocation
>> +identifier rule0.fn;
>> +@@
>> +
>> + fn(...)
>> +{
>> +    ERRP_FUNCTION_BEGIN();
>> +-   ERRP_FUNCTION_BEGIN();
>> +    ...
>> +}
> 
> This is smaller than the script you posted in v2, and thus I'm a bit
> more confident in stating that it looks correct and idempotent.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v3 04/25] error: auto propagated local_err

[Kevin Wolf]

Am 24.09.2019 um 22:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of
> functions with errp parameter.

A bit of bike shedding, but FOO_BEGIN suggests to me that a FOO_END will
follow. Can we find a different name, especially now that we won't use
this macro in every function that uses an errp, so even the "errp
function" part isn't really correct any more?

How about ERRP_AUTO_PROPAGATE?

Kevin

Re: [PATCH v3 04/25] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

30.09.2019 18:12, Kevin Wolf wrote:
> Am 24.09.2019 um 22:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of
>> functions with errp parameter.
> 
> A bit of bike shedding, but FOO_BEGIN suggests to me that a FOO_END will
> follow. Can we find a different name, especially now that we won't use
> this macro in every function that uses an errp, so even the "errp
> function" part isn't really correct any more?
> 
> How about ERRP_AUTO_PROPAGATE?
> 

I have an idea that with this macro we can (optionally) get the whole call stack
of the error and print it to log, so it's good to give it more generic name, not
limited to propagation..

WRAP_ERRP

AUTO_ERRP

AUTOMATE_ERRP

or what do you think?

-- 
Best regards,
Vladimir

Re: [PATCH v3 04/25] error: auto propagated local_err

[Kevin Wolf]

Am 30.09.2019 um 17:19 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 30.09.2019 18:12, Kevin Wolf wrote:
> > Am 24.09.2019 um 22:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of
> >> functions with errp parameter.
> > 
> > A bit of bike shedding, but FOO_BEGIN suggests to me that a FOO_END will
> > follow. Can we find a different name, especially now that we won't use
> > this macro in every function that uses an errp, so even the "errp
> > function" part isn't really correct any more?
> > 
> > How about ERRP_AUTO_PROPAGATE?
> 
> I have an idea that with this macro we can (optionally) get the whole call stack
> of the error and print it to log, so it's good to give it more generic name, not
> limited to propagation..

Hm, what's the context for this feature?

The obvious one where you want to have a stack trace is &error_abort,
but that one crashes, so you get it automatically. If it's just a normal
error (like a QAPI option contains an invalid value and some function
down the call chain checks it), why would anyone want to know what the
call chain in the QEMU code was?

Kevin

Re: [PATCH v3 04/25] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

30.09.2019 19:00, Kevin Wolf wrote:
> Am 30.09.2019 um 17:19 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 30.09.2019 18:12, Kevin Wolf wrote:
>>> Am 24.09.2019 um 22:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of
>>>> functions with errp parameter.
>>>
>>> A bit of bike shedding, but FOO_BEGIN suggests to me that a FOO_END will
>>> follow. Can we find a different name, especially now that we won't use
>>> this macro in every function that uses an errp, so even the "errp
>>> function" part isn't really correct any more?
>>>
>>> How about ERRP_AUTO_PROPAGATE?
>>
>> I have an idea that with this macro we can (optionally) get the whole call stack
>> of the error and print it to log, so it's good to give it more generic name, not
>> limited to propagation..
> 
> Hm, what's the context for this feature?
> 
> The obvious one where you want to have a stack trace is &error_abort,
> but that one crashes, so you get it automatically. If it's just a normal
> error (like a QAPI option contains an invalid value and some function
> down the call chain checks it), why would anyone want to know what the
> call chain in the QEMU code was?
> 

When I have bug from testers, call stack would be a lot more descriptive, than just
an error message.

We may add trace point which will print this information, so with disabled trace point
- no extra output.



-- 
Best regards,
Vladimir

Re: [PATCH v3 04/25] error: auto propagated local_err

[Kevin Wolf]

Am 30.09.2019 um 18:26 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 30.09.2019 19:00, Kevin Wolf wrote:
> > Am 30.09.2019 um 17:19 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> 30.09.2019 18:12, Kevin Wolf wrote:
> >>> Am 24.09.2019 um 22:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>>> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of
> >>>> functions with errp parameter.
> >>>
> >>> A bit of bike shedding, but FOO_BEGIN suggests to me that a FOO_END will
> >>> follow. Can we find a different name, especially now that we won't use
> >>> this macro in every function that uses an errp, so even the "errp
> >>> function" part isn't really correct any more?
> >>>
> >>> How about ERRP_AUTO_PROPAGATE?
> >>
> >> I have an idea that with this macro we can (optionally) get the whole call stack
> >> of the error and print it to log, so it's good to give it more generic name, not
> >> limited to propagation..
> > 
> > Hm, what's the context for this feature?
> > 
> > The obvious one where you want to have a stack trace is &error_abort,
> > but that one crashes, so you get it automatically. If it's just a normal
> > error (like a QAPI option contains an invalid value and some function
> > down the call chain checks it), why would anyone want to know what the
> > call chain in the QEMU code was?
> > 
> 
> When I have bug from testers, call stack would be a lot more descriptive, than just
> an error message.
> 
> We may add trace point which will print this information, so with disabled trace point
> - no extra output.

But wouldn't it make much more sense then to optionally add this
functionality to any trace point? I really don't see how this is related
specifically to user-visible error messages.

However, even if we decide that we want to have this in Error objects,
wouldn't it make much more sense to use the real C stack trace and save
it from the innermost error_set() using backtrace() or compiler
built-ins rather than relying on an error_propagate() chain?

Kevin

Re: [PATCH v3 04/25] error: auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

30.09.2019 19:39, Kevin Wolf wrote:
> Am 30.09.2019 um 18:26 hat Vladimir Sementsov-Ogievskiy geschrieben:
>> 30.09.2019 19:00, Kevin Wolf wrote:
>>> Am 30.09.2019 um 17:19 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>> 30.09.2019 18:12, Kevin Wolf wrote:
>>>>> Am 24.09.2019 um 22:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
>>>>>> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of
>>>>>> functions with errp parameter.
>>>>>
>>>>> A bit of bike shedding, but FOO_BEGIN suggests to me that a FOO_END will
>>>>> follow. Can we find a different name, especially now that we won't use
>>>>> this macro in every function that uses an errp, so even the "errp
>>>>> function" part isn't really correct any more?
>>>>>
>>>>> How about ERRP_AUTO_PROPAGATE?
>>>>
>>>> I have an idea that with this macro we can (optionally) get the whole call stack
>>>> of the error and print it to log, so it's good to give it more generic name, not
>>>> limited to propagation..
>>>
>>> Hm, what's the context for this feature?
>>>
>>> The obvious one where you want to have a stack trace is &error_abort,
>>> but that one crashes, so you get it automatically. If it's just a normal
>>> error (like a QAPI option contains an invalid value and some function
>>> down the call chain checks it), why would anyone want to know what the
>>> call chain in the QEMU code was?
>>>
>>
>> When I have bug from testers, call stack would be a lot more descriptive, than just
>> an error message.
>>
>> We may add trace point which will print this information, so with disabled trace point
>> - no extra output.
> 
> But wouldn't it make much more sense then to optionally add this
> functionality to any trace point? I really don't see how this is related
> specifically to user-visible error messages.

Interesting idea

> 
> However, even if we decide that we want to have this in Error objects,
> wouldn't it make much more sense to use the real C stack trace and save
> it from the innermost error_set() using backtrace() or compiler
> built-ins rather than relying on an error_propagate() chain?
> 
Hmm, I thought about this.. And in concatenation with the fact that we'll have macro not
everywhere, backtrace may be better..

On the other hand, backtrace will not show coroutine entries..

OK, anyway, if we will track some additional information in trace-events or in macros or
in error_* API functions, it's not bad to track some additional information in macro
named ERRP_AUTO_PROPAGATE.

-- 
Best regards,
Vladimir

Re: [PATCH v3 04/25] error: auto propagated local_err

[Kevin Wolf]

Am 01.10.2019 um 10:39 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 30.09.2019 19:39, Kevin Wolf wrote:
> > Am 30.09.2019 um 18:26 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >> 30.09.2019 19:00, Kevin Wolf wrote:
> >>> Am 30.09.2019 um 17:19 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>>> 30.09.2019 18:12, Kevin Wolf wrote:
> >>>>> Am 24.09.2019 um 22:08 hat Vladimir Sementsov-Ogievskiy geschrieben:
> >>>>>> Here is introduced ERRP_FUNCTION_BEGIN macro, to be used at start of
> >>>>>> functions with errp parameter.
> >>>>>
> >>>>> A bit of bike shedding, but FOO_BEGIN suggests to me that a FOO_END will
> >>>>> follow. Can we find a different name, especially now that we won't use
> >>>>> this macro in every function that uses an errp, so even the "errp
> >>>>> function" part isn't really correct any more?
> >>>>>
> >>>>> How about ERRP_AUTO_PROPAGATE?
> >>>>
> >>>> I have an idea that with this macro we can (optionally) get the whole call stack
> >>>> of the error and print it to log, so it's good to give it more generic name, not
> >>>> limited to propagation..
> >>>
> >>> Hm, what's the context for this feature?
> >>>
> >>> The obvious one where you want to have a stack trace is &error_abort,
> >>> but that one crashes, so you get it automatically. If it's just a normal
> >>> error (like a QAPI option contains an invalid value and some function
> >>> down the call chain checks it), why would anyone want to know what the
> >>> call chain in the QEMU code was?
> >>>
> >>
> >> When I have bug from testers, call stack would be a lot more descriptive, than just
> >> an error message.
> >>
> >> We may add trace point which will print this information, so with disabled trace point
> >> - no extra output.
> > 
> > But wouldn't it make much more sense then to optionally add this
> > functionality to any trace point? I really don't see how this is related
> > specifically to user-visible error messages.
> 
> Interesting idea
> 
> > 
> > However, even if we decide that we want to have this in Error objects,
> > wouldn't it make much more sense to use the real C stack trace and save
> > it from the innermost error_set() using backtrace() or compiler
> > built-ins rather than relying on an error_propagate() chain?
> > 
> Hmm, I thought about this.. And in concatenation with the fact that
> we'll have macro not everywhere, backtrace may be better..
> 
> On the other hand, backtrace will not show coroutine entries..

Hm, good point. I wonder if we can easily get a stack trace not starting
at the current point, but from a jmp_buf. Then we could just switch to
the coroutine caller whenever we reach coroutine_trampoline().

But glibc doesn't seem to support this case easily, so that might mean
rewriting all of the stack unwinding inside QEMU... Maybe not then.

> OK, anyway, if we will track some additional information in
> trace-events or in macros or in error_* API functions, it's not bad to
> track some additional information in macro named ERRP_AUTO_PROPAGATE.

Yes, I think tracking the information where we use ERRP_AUTO_PROPAGATE
anyway is okay. I just wouldn't add the macro everywhere just for the
sake of the additional information.

Kevin