[Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint()

[Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint()

[Greg Kurz]

A recent post unveiled a potential problem when passing errp to
error_append_hint():

https://patchwork.ozlabs.org/patch/1162171/

The issue is that error_append_hint() may end up not being called
at all if errp is equal to &error_fatal or &error_abort. It turns
out that the only way to ensure error_append_hint() can do its job
regardless of how the error is handled is to use a local error
object and to propagate it to the caller.

As suggested by Markus Armbruster, this series does:
(1) update error_append_hint()'s documentation in <qapi/error.h> to
    spell this out
(2) fix other misuses of error_append_hint() in the tree

The following command was used to detect candidates for (2), which
were then manually inspected. It is possible that many of them never
exhibit the unwanted behaviour in practice because errp happens to
never be equal to &error_fatal or &error_abort. It isn't trivial to
assess though, and this is fragile and can be easily broken if the
code gets rearranged. Also, passing errp directly is a shortcut that
doesn't bring much, neither for clarity, nor for performance that
we don't really care for on an error path). Better safe than sorry,
so fix them all.

I tried to group the changes that are supposed to go through the same
tree together.

$ git grep -n error_append_hint\(errp | cut -d: -f 1-2
block/backup.c:608
block/dirty-bitmap.c:256
block/file-posix.c:393
block/file-posix.c:854
block/file-posix.c:2282
block/gluster.c:480
block/gluster.c:483
block/gluster.c:489
block/gluster.c:702
block/gluster.c:711
block/qcow.c:162
block/qcow.c:195
block/qcow2.c:1363
block/vhdx-log.c:811
block/vpc.c:1033

=> Patch for block

chardev/spice.c:284

=> Patch for chardev

hw/9pfs/9p-local.c:1474
hw/9pfs/9p-proxy.c:1119

Already posted https://patchwork.ozlabs.org/patch/1162171/

hw/arm/msf2-soc.c:131
hw/arm/virt.c:1808
hw/arm/virt.c:1836
hw/intc/arm_gicv3_kvm.c:815
hw/misc/msf2-sysreg.c:135

=> Patch for arm

hw/pci-bridge/pcie_root_port.c:76
hw/pci-bridge/pcie_root_port.c:90

=> Patch for pci

hw/ppc/mac_newworld.c:622
hw/ppc/spapr.c:4341
hw/ppc/spapr_pci.c:1874
hw/ppc/spapr_pci.c:1876
hw/ppc/spapr_pci.c:1878

=> Patch for ppc

hw/rdma/vmw/pvrdma_main.c:670

=> Patch for pvrdma

hw/s390x/s390-ccw.c:63

=> Patch for s390

hw/scsi/scsi-disk.c:2624
hw/scsi/scsi-generic.c:679

=> Patch for scsi

hw/usb/ccid-card-emulated.c:516

=> Patch for usb

hw/vfio/common.c:1473
hw/vfio/common.c:1536
hw/vfio/pci.c:2532
hw/vfio/pci.c:2718

=> Patch for vfio

hw/virtio/virtio-pci.c:1548
hw/virtio/virtio-pci.c:1742

=> Patch for virtio

migration/migration.c:988
migration/migration.c:997

=> Patch for migration

nbd/client.c:230
nbd/server.c:1627

=> Patch for nbd

qdev-monitor.c:334
qdev-monitor.c:337
qdev-monitor.c:340
qdev-monitor.c:348
qdev-monitor.c:351
qdev-monitor.c:354
qdev-monitor.c:358

No misuse here as these aren't preceded by a call to error_setg() or
error_propagate() that could terminate QEMU.

target/ppc/kvm.c:246
target/ppc/kvm.c:2089
target/ppc/kvm.c:2092

=> Patch for kvm

util/qemu-option.c:160
util/qemu-option.c:669

=> Patch for option

util/qemu-sockets.c:886
util/qemu-sockets.c:956

=> Patch for sockets

As a bonus, also teach checkpatch to detect that. Since the convention
of using the errp naming seems to be strictly followed, the check is
just to detect "error_append_hint(errp" and emit a warning, for the sake
of simplicity.

--
Greg

---

Greg Kurz (17):
      error: Update error_append_hint()'s documentation
      block: Pass local error object pointer to error_append_hint()
      char/spice: Pass local error object pointer to error_append_hint()
      ppc: Pass local error object pointer to error_append_hint()
      arm: Pass local error object pointer to error_append_hint()
      vfio: Pass local error object pointer to error_append_hint()
      virtio-pci: Pass local error object pointer to error_append_hint()
      pcie_root_port: Pass local error object pointer to error_append_hint()
      hw/rdma: Fix missing conversion to rdma_error_report()
      s390x/css: Pass local error object pointer to error_append_hint()
      scsi: Pass local error object pointer to error_append_hint()
      migration: Pass local error object pointer to error_append_hint()
      nbd: Pass local error object pointer to error_append_hint()
      ccid-card-emul: Pass local error object pointer to error_append_hint()
      option: Pass local error object pointer to error_append_hint()
      socket: Pass local error object pointer to error_append_hint()
      checkpatch: Warn when errp is passed to error_append_hint()


 block/backup.c                 |    7 +++++--
 block/dirty-bitmap.c           |    7 +++++--
 block/file-posix.c             |   20 +++++++++++++-------
 block/gluster.c                |   23 +++++++++++++++--------
 block/qcow.c                   |   10 ++++++----
 block/qcow2.c                  |    7 +++++--
 block/vhdx-log.c               |    7 +++++--
 block/vpc.c                    |    7 +++++--
 chardev/spice.c                |    6 ++++--
 hw/arm/msf2-soc.c              |    5 +++--
 hw/arm/virt.c                  |   14 ++++++++++----
 hw/intc/arm_gicv3_kvm.c        |    5 +++--
 hw/misc/msf2-sysreg.c          |    6 ++++--
 hw/pci-bridge/pcie_root_port.c |   11 +++++++----
 hw/ppc/mac_newworld.c          |    7 +++++--
 hw/ppc/spapr.c                 |    7 +++++--
 hw/ppc/spapr_pci.c             |    9 +++++----
 hw/rdma/vmw/pvrdma_main.c      |    2 +-
 hw/s390x/s390-ccw.c            |    6 ++++--
 hw/scsi/scsi-disk.c            |    7 +++++--
 hw/scsi/scsi-generic.c         |    7 +++++--
 hw/usb/ccid-card-emulated.c    |    7 +++++--
 hw/vfio/common.c               |   14 ++++++++++----
 hw/vfio/pci.c                  |   12 ++++++++----
 hw/virtio/virtio-pci.c         |   14 ++++++++++----
 include/qapi/error.h           |   11 ++++++++++-
 migration/migration.c          |   14 ++++++++++----
 nbd/client.c                   |   24 +++++++++++++-----------
 nbd/server.c                   |    7 +++++--
 scripts/checkpatch.pl          |    4 ++++
 target/ppc/kvm.c               |   13 +++++++++----
 util/qemu-option.c             |   14 ++++++++++----
 util/qemu-sockets.c            |   14 ++++++++++----
 33 files changed, 224 insertions(+), 104 deletions(-)

[Qemu-devel] [PATCH 01/17] error: Update error_append_hint()'s documentation

[Greg Kurz]

error_setg() and error_propagate(), as well as their variants, cause
QEMU to terminate when called with &error_fatal or &error_abort. This
prevents to add hints since error_append_hint() isn't even called in
this case.

It means that error_append_hint() should only be used with a local
error object, and then propagate this local error to the caller.

Document this in <qapi/error.h> .

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 include/qapi/error.h |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 3f95141a01a8..28174329ba71 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -27,6 +27,13 @@
  *     error_setg(&err, "invalid quark\n"
  *                "Valid quarks are up, down, strange, charm, top, bottom.");
  *
+ * Dot *not* pass the errp parameter from the caller
+ *     error_setg(errp, "invalid quark");
+ *     error_append_hint(errp, "Valid quarks are up, down, strange, "
+ *                       "charm, top, bottom.\n");
+ * because error_setg() terminates QEMU if @errp is &error_fatal or
+ * &error_abort, and the hints are not even added.
+ *
  * Report an error to the current monitor if we have one, else stderr:
  *     error_report_err(err);
  * This frees the error object.
@@ -252,7 +259,9 @@ void error_prepend(Error **errp, const char *fmt, ...)
  * error message.
  * @errp may be NULL, but not &error_fatal or &error_abort.
  * Trivially the case if you call it only after error_setg() or
- * error_propagate().
+ * error_propagate(). CAUTION: error_setg() and error_propagate() do
+ * not return when passed &error_fatal. Always use a local error object
+ * and propagate it to the caller.
  * May be called multiple times.  The resulting hint should end with a
  * newline.
  */

[Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()

[Greg Kurz]

Ensure that hints are added even if errp is &error_fatal or &error_abort.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 block/backup.c       |    7 +++++--
 block/dirty-bitmap.c |    7 +++++--
 block/file-posix.c   |   20 +++++++++++++-------
 block/gluster.c      |   23 +++++++++++++++--------
 block/qcow.c         |   10 ++++++----
 block/qcow2.c        |    7 +++++--
 block/vhdx-log.c     |    7 +++++--
 block/vpc.c          |    7 +++++--
 8 files changed, 59 insertions(+), 29 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 763f0d7ff6db..d8c422a0e3bc 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
                     BACKUP_CLUSTER_SIZE_DEFAULT);
         return BACKUP_CLUSTER_SIZE_DEFAULT;
     } else if (ret < 0 && !target->backing) {
-        error_setg_errno(errp, -ret,
+        Error *local_err = NULL;
+
+        error_setg_errno(&local_err, -ret,
             "Couldn't determine the cluster size of the target image, "
             "which has no backing file");
-        error_append_hint(errp,
+        error_append_hint(&local_err,
             "Aborting, since this may create an unusable destination image\n");
+        error_propagate(errp, local_err);
         return ret;
     } else if (ret < 0 && target->backing) {
         /* Not fatal; just trudge on ahead. */
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index 134e0c9a0c8f..b31017a77d51 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -251,10 +251,13 @@ int bdrv_dirty_bitmap_check(const BdrvDirtyBitmap *bitmap, uint32_t flags,
 
     if ((flags & BDRV_BITMAP_INCONSISTENT) &&
         bdrv_dirty_bitmap_inconsistent(bitmap)) {
-        error_setg(errp, "Bitmap '%s' is inconsistent and cannot be used",
+        Error *local_err = NULL;
+
+        error_setg(&local_err, "Bitmap '%s' is inconsistent and cannot be used",
                    bitmap->name);
-        error_append_hint(errp, "Try block-dirty-bitmap-remove to delete"
+        error_append_hint(&local_err, "Try block-dirty-bitmap-remove to delete"
                           " this bitmap from disk");
+        error_propagate(errp, local_err);
         return -1;
     }
 
diff --git a/block/file-posix.c b/block/file-posix.c
index f12c06de2df5..9a95f7e94123 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -389,8 +389,11 @@ static void raw_probe_alignment(BlockDriverState *bs, int fd, Error **errp)
     }
 
     if (!s->buf_align || !bs->bl.request_alignment) {
-        error_setg(errp, "Could not find working O_DIRECT alignment");
-        error_append_hint(errp, "Try cache.direct=off\n");
+        Error *local_err = NULL;
+
+        error_setg(&local_err, "Could not find working O_DIRECT alignment");
+        error_append_hint(&local_err, "Try cache.direct=off\n");
+        error_propagate(errp, local_err);
     }
 }
 
@@ -845,16 +848,18 @@ static int raw_handle_perm_lock(BlockDriverState *bs,
         }
         ret = raw_apply_lock_bytes(s, s->fd, s->perm | new_perm,
                                    ~s->shared_perm | ~new_shared,
-                                   false, errp);
+                                   false, &local_err);
         if (!ret) {
-            ret = raw_check_lock_bytes(s->fd, new_perm, new_shared, errp);
+            ret = raw_check_lock_bytes(s->fd, new_perm, new_shared, &local_err);
             if (!ret) {
                 return 0;
             }
-            error_append_hint(errp,
+            error_append_hint(&local_err,
                               "Is another process using the image [%s]?\n",
                               bs->filename);
         }
+        error_propagate(errp, local_err);
+        local_err = NULL; /* for fall through */
         op = RAW_PL_ABORT;
         /* fall through to unlock bytes. */
     case RAW_PL_ABORT:
@@ -2277,11 +2282,12 @@ raw_co_create(BlockdevCreateOptions *options, Error **errp)
     }
 
     /* Step two: Check that nobody else has taken conflicting locks */
-    result = raw_check_lock_bytes(fd, perm, shared, errp);
+    result = raw_check_lock_bytes(fd, perm, shared, &local_err);
     if (result < 0) {
-        error_append_hint(errp,
+        error_append_hint(&local_err,
                           "Is another process using the image [%s]?\n",
                           file_opts->filename);
+        error_propagate(errp, local_err);
         goto out_unlock;
     }
 
diff --git a/block/gluster.c b/block/gluster.c
index 64028b2cba30..dce42884f97c 100644
--- a/block/gluster.c
+++ b/block/gluster.c
@@ -473,26 +473,29 @@ static struct glfs *qemu_gluster_glfs_init(BlockdevOptionsGluster *gconf,
 
     ret = glfs_init(glfs);
     if (ret) {
-        error_setg(errp, "Gluster connection for volume %s, path %s failed"
+        Error *local_err = NULL;
+
+        error_setg(&local_err, "Gluster connection for volume %s, path %s failed"
                          " to connect", gconf->volume, gconf->path);
         for (server = gconf->server; server; server = server->next) {
             if (server->value->type  == SOCKET_ADDRESS_TYPE_UNIX) {
-                error_append_hint(errp, "hint: failed on socket %s ",
+                error_append_hint(&local_err, "hint: failed on socket %s ",
                                   server->value->u.q_unix.path);
             } else {
-                error_append_hint(errp, "hint: failed on host %s and port %s ",
+                error_append_hint(&local_err, "hint: failed on host %s and port %s ",
                                   server->value->u.inet.host,
                                   server->value->u.inet.port);
             }
         }
 
-        error_append_hint(errp, "Please refer to gluster logs for more info\n");
+        error_append_hint(&local_err, "Please refer to gluster logs for more info\n");
 
         /* glfs_init sometimes doesn't set errno although docs suggest that */
         if (errno == 0) {
             errno = EINVAL;
         }
 
+        error_propagate(errp, local_err);
         goto out;
     }
     return glfs;
@@ -695,20 +698,23 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf,
                               QDict *options, Error **errp)
 {
     int ret;
+    Error *local_err = NULL;
+
     if (filename) {
         ret = qemu_gluster_parse_uri(gconf, filename);
         if (ret < 0) {
-            error_setg(errp, "invalid URI %s", filename);
-            error_append_hint(errp, "Usage: file=gluster[+transport]://"
+            error_setg(&local_err, "invalid URI %s", filename);
+            error_append_hint(&local_err, "Usage: file=gluster[+transport]://"
                                     "[host[:port]]volume/path[?socket=...]"
                                     "[,file.debug=N]"
                                     "[,file.logfile=/path/filename.log]\n");
+            error_propagate(errp, local_err);
             return ret;
         }
     } else {
-        ret = qemu_gluster_parse_json(gconf, options, errp);
+        ret = qemu_gluster_parse_json(gconf, options, &local_err);
         if (ret < 0) {
-            error_append_hint(errp, "Usage: "
+            error_append_hint(&local_err, "Usage: "
                              "-drive driver=qcow2,file.driver=gluster,"
                              "file.volume=testvol,file.path=/path/a.qcow2"
                              "[,file.debug=9]"
@@ -719,6 +725,7 @@ static int qemu_gluster_parse(BlockdevOptionsGluster *gconf,
                              "file.server.1.transport=unix,"
                              "file.server.1.path=/var/run/glusterd.socket ..."
                              "\n");
+            error_propagate(errp, local_err);
             return ret;
         }
     }
diff --git a/block/qcow.c b/block/qcow.c
index 5bdf72ba33ce..9049573b52b2 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -156,11 +156,12 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
         goto fail;
     }
     if (header.version != QCOW_VERSION) {
-        error_setg(errp, "qcow (v%d) does not support qcow version %" PRIu32,
+        error_setg(&local_err, "qcow (v%d) does not support qcow version %" PRIu32,
                    QCOW_VERSION, header.version);
         if (header.version == 2 || header.version == 3) {
-            error_append_hint(errp, "Try the 'qcow2' driver instead.\n");
+            error_append_hint(&local_err, "Try the 'qcow2' driver instead.\n");
         }
+        error_propagate(errp, local_err);
 
         ret = -ENOTSUP;
         goto fail;
@@ -189,14 +190,15 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
     if (s->crypt_method_header) {
         if (bdrv_uses_whitelist() &&
             s->crypt_method_header == QCOW_CRYPT_AES) {
-            error_setg(errp,
+            error_setg(&local_err,
                        "Use of AES-CBC encrypted qcow images is no longer "
                        "supported in system emulators");
-            error_append_hint(errp,
+            error_append_hint(&local_err,
                               "You can use 'qemu-img convert' to convert your "
                               "image to an alternative supported format, such "
                               "as unencrypted qcow, or raw with the LUKS "
                               "format instead.\n");
+            error_propagate(errp, local_err);
             ret = -ENOSYS;
             goto fail;
         }
diff --git a/block/qcow2.c b/block/qcow2.c
index 57734f20cf8d..2c52139b7385 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1357,14 +1357,17 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
     if (s->crypt_method_header) {
         if (bdrv_uses_whitelist() &&
             s->crypt_method_header == QCOW_CRYPT_AES) {
-            error_setg(errp,
+            Error *local_err = NULL;
+
+            error_setg(&local_err,
                        "Use of AES-CBC encrypted qcow2 images is no longer "
                        "supported in system emulators");
-            error_append_hint(errp,
+            error_append_hint(&local_err,
                               "You can use 'qemu-img convert' to convert your "
                               "image to an alternative supported format, such "
                               "as unencrypted qcow2, or raw with the LUKS "
                               "format instead.\n");
+            error_propagate(errp, local_err);
             ret = -ENOSYS;
             goto fail;
         }
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index fdd3a7adc378..9f4de9cbcdb6 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -802,15 +802,18 @@ int vhdx_parse_log(BlockDriverState *bs, BDRVVHDXState *s, bool *flushed,
 
     if (logs.valid) {
         if (bs->read_only) {
+            Error *local_err = NULL;
+
             bdrv_refresh_filename(bs);
             ret = -EPERM;
-            error_setg(errp,
+            error_setg(&local_err,
                        "VHDX image file '%s' opened read-only, but "
                        "contains a log that needs to be replayed",
                        bs->filename);
-            error_append_hint(errp,  "To replay the log, run:\n"
+            error_append_hint(&local_err,  "To replay the log, run:\n"
                               "qemu-img check -r all '%s'\n",
                               bs->filename);
+            error_propagate(errp, local_err);
             goto exit;
         }
         /* now flush the log */
diff --git a/block/vpc.c b/block/vpc.c
index 5cd38907808b..facd7cdb2c98 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1028,12 +1028,15 @@ static int coroutine_fn vpc_co_create(BlockdevCreateOptions *opts,
     }
 
     if (total_size != total_sectors * BDRV_SECTOR_SIZE) {
-        error_setg(errp, "The requested image size cannot be represented in "
+        Error *local_err = NULL;
+
+        error_setg(&local_err, "The requested image size cannot be represented in "
                          "CHS geometry");
-        error_append_hint(errp, "Try size=%llu or force-size=on (the "
+        error_append_hint(&local_err, "Try size=%llu or force-size=on (the "
                                 "latter makes the image incompatible with "
                                 "Virtual PC)",
                           total_sectors * BDRV_SECTOR_SIZE);
+        error_propagate(errp, local_err);
         ret = -EINVAL;
         goto out;
     }

[Qemu-devel] [PATCH 03/17] char/spice: Pass local error object pointer to error_append_hint()

[Greg Kurz]

Ensure that hints are added even if errp is &error_fatal or &error_abort.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 chardev/spice.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/chardev/spice.c b/chardev/spice.c
index 241e2b7770eb..ed5996467c32 100644
--- a/chardev/spice.c
+++ b/chardev/spice.c
@@ -279,10 +279,12 @@ static void qemu_chr_open_spice_vmc(Chardev *chr,
     if (*psubtype == NULL) {
         char *subtypes = g_strjoinv(", ",
             (gchar **)spice_server_char_device_recognized_subtypes());
+        Error *local_err = NULL;
 
-        error_setg(errp, "unsupported type name: %s", type);
-        error_append_hint(errp, "allowed spice char type names: %s\n",
+        error_setg(&local_err, "unsupported type name: %s", type);
+        error_append_hint(&local_err, "allowed spice char type names: %s\n",
                           subtypes);
+        error_propagate(errp, local_err);
 
         g_free(subtypes);
         return;

[Qemu-devel] [PATCH 04/17] ppc: Pass local error object pointer to error_append_hint()

[Greg Kurz]

Ensure that hints are added even if errp is &error_fatal or &error_abort.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/mac_newworld.c |    7 +++++--
 hw/ppc/spapr.c        |    7 +++++--
 hw/ppc/spapr_pci.c    |    9 +++++----
 target/ppc/kvm.c      |   13 +++++++++----
 4 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index c5bbcc743352..aca8c40bf395 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -618,8 +618,11 @@ static void core99_set_via_config(Object *obj, const char *value, Error **errp)
     } else if (!strcmp(value, "pmu-adb")) {
         cms->via_config = CORE99_VIA_CONFIG_PMU_ADB;
     } else {
-        error_setg(errp, "Invalid via value");
-        error_append_hint(errp, "Valid values are cuda, pmu, pmu-adb.\n");
+        Error *local_err = NULL;
+
+        error_setg(&local_err, "Invalid via value");
+        error_append_hint(&local_err, "Valid values are cuda, pmu, pmu-adb.\n");
+        error_propagate(errp, local_err);
     }
 }
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 08a2a5a77092..39d6f57d014e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4337,10 +4337,13 @@ void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
     vcpu_id = spapr_vcpu_id(spapr, cpu_index);
 
     if (kvm_enabled() && !kvm_vcpu_id_is_valid(vcpu_id)) {
-        error_setg(errp, "Can't create CPU with id %d in KVM", vcpu_id);
-        error_append_hint(errp, "Adjust the number of cpus to %d "
+        Error *local_err = NULL;
+
+        error_setg(&local_err, "Can't create CPU with id %d in KVM", vcpu_id);
+        error_append_hint(&local_err, "Adjust the number of cpus to %d "
                           "or try to raise the number of threads per core\n",
                           vcpu_id * ms->smp.threads / spapr->vsmt);
+        error_propagate(errp, local_err);
         return;
     }
 
diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 7b71ad7c74f1..4b7e9a1c8666 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1870,12 +1870,13 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     if (spapr_pci_find_phb(spapr, sphb->buid)) {
         SpaprPhbState *s;
 
-        error_setg(errp, "PCI host bridges must have unique indexes");
-        error_append_hint(errp, "The following indexes are already in use:");
+        error_setg(&local_err, "PCI host bridges must have unique indexes");
+        error_append_hint(&local_err, "The following indexes are already in use:");
         QLIST_FOREACH(s, &spapr->phbs, list) {
-            error_append_hint(errp, " %d", s->index);
+            error_append_hint(&local_err, " %d", s->index);
         }
-        error_append_hint(errp, "\nTry another value for the index property\n");
+        error_append_hint(&local_err, "\nTry another value for the index property\n");
+        error_propagate(errp, local_err);
         return;
     }
 
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 8c5b1f25cc95..c6876b08c726 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -242,8 +242,11 @@ static void kvm_get_smmu_info(struct kvm_ppc_smmu_info *info, Error **errp)
     assert(kvm_state != NULL);
 
     if (!kvm_check_extension(kvm_state, KVM_CAP_PPC_GET_SMMU_INFO)) {
-        error_setg(errp, "KVM doesn't expose the MMU features it supports");
-        error_append_hint(errp, "Consider switching to a newer KVM\n");
+        Error *local_err = NULL;
+
+        error_setg(&local_err, "KVM doesn't expose the MMU features it supports");
+        error_append_hint(&local_err, "Consider switching to a newer KVM\n");
+        error_propagate(errp, local_err);
         return;
     }
 
@@ -2076,6 +2079,7 @@ void kvmppc_hint_smt_possible(Error **errp)
     int i;
     GString *g;
     char *s;
+    Error *local_err = NULL;
 
     assert(kvm_enabled());
     if (cap_ppc_smt_possible) {
@@ -2086,12 +2090,13 @@ void kvmppc_hint_smt_possible(Error **errp)
             }
         }
         s = g_string_free(g, false);
-        error_append_hint(errp, "%s.\n", s);
+        error_append_hint(&local_err, "%s.\n", s);
         g_free(s);
     } else {
-        error_append_hint(errp,
+        error_append_hint(&local_err,
                           "This KVM seems to be too old to support VSMT.\n");
     }
+    error_propagate(errp, local_err);
 }
 
 

[Qemu-devel] [PATCH 05/17] arm: Pass local error object pointer to error_append_hint()

[Greg Kurz]

Ensure that hints are added even if errp is &error_fatal or &error_abort.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/arm/msf2-soc.c       |    5 +++--
 hw/arm/virt.c           |   14 ++++++++++----
 hw/intc/arm_gicv3_kvm.c |    5 +++--
 hw/misc/msf2-sysreg.c   |    6 ++++--
 4 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/hw/arm/msf2-soc.c b/hw/arm/msf2-soc.c
index 008fd9327aa4..f606cfe7d139 100644
--- a/hw/arm/msf2-soc.c
+++ b/hw/arm/msf2-soc.c
@@ -127,8 +127,9 @@ static void m2sxxx_soc_realize(DeviceState *dev_soc, Error **errp)
     }
 
     if (!s->m3clk) {
-        error_setg(errp, "Invalid m3clk value");
-        error_append_hint(errp, "m3clk can not be zero\n");
+        error_setg(&err, "Invalid m3clk value");
+        error_append_hint(&err, "m3clk can not be zero\n");
+        error_propagate(errp, err);
         return;
     }
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d74538b0212e..7ac00dab581e 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1804,8 +1804,11 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
     } else if (!strcmp(value, "max")) {
         vms->gic_version = -1; /* Will probe later */
     } else {
-        error_setg(errp, "Invalid gic-version value");
-        error_append_hint(errp, "Valid values are 3, 2, host, max.\n");
+        Error *err = NULL;
+
+        error_setg(&err, "Invalid gic-version value");
+        error_append_hint(&err, "Valid values are 3, 2, host, max.\n");
+        error_propagate(errp, err);
     }
 }
 
@@ -1832,8 +1835,11 @@ static void virt_set_iommu(Object *obj, const char *value, Error **errp)
     } else if (!strcmp(value, "none")) {
         vms->iommu = VIRT_IOMMU_NONE;
     } else {
-        error_setg(errp, "Invalid iommu value");
-        error_append_hint(errp, "Valid values are none, smmuv3.\n");
+        Error *err = NULL;
+
+        error_setg(&err, "Invalid iommu value");
+        error_append_hint(&err, "Valid values are none, smmuv3.\n");
+        error_propagate(errp, err);
     }
 }
 
diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
index 9c7f4ab8711c..09784f06f9ad 100644
--- a/hw/intc/arm_gicv3_kvm.c
+++ b/hw/intc/arm_gicv3_kvm.c
@@ -810,10 +810,11 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
                               KVM_VGIC_V3_ADDR_TYPE_REDIST_REGION);
 
     if (!multiple_redist_region_allowed && s->nb_redist_regions > 1) {
-        error_setg(errp, "Multiple VGICv3 redistributor regions are not "
+        error_setg(&local_err, "Multiple VGICv3 redistributor regions are not "
                    "supported by this host kernel");
-        error_append_hint(errp, "A maximum of %d VCPUs can be used",
+        error_append_hint(&local_err, "A maximum of %d VCPUs can be used",
                           s->redist_region_count[0]);
+        error_propagate(errp, local_err);
         return;
     }
 
diff --git a/hw/misc/msf2-sysreg.c b/hw/misc/msf2-sysreg.c
index ddc5a30c80f7..23c7890ac022 100644
--- a/hw/misc/msf2-sysreg.c
+++ b/hw/misc/msf2-sysreg.c
@@ -128,12 +128,14 @@ static Property msf2_sysreg_properties[] = {
 static void msf2_sysreg_realize(DeviceState *dev, Error **errp)
 {
     MSF2SysregState *s = MSF2_SYSREG(dev);
+    Error *local_err = NULL;
 
     if ((s->apb0div > 32 || !is_power_of_2(s->apb0div))
         || (s->apb1div > 32 || !is_power_of_2(s->apb1div))) {
-        error_setg(errp, "Invalid apb divisor value");
-        error_append_hint(errp, "apb divisor must be a power of 2"
+        error_setg(&local_err, "Invalid apb divisor value");
+        error_append_hint(&local_err, "apb divisor must be a power of 2"
                            " and maximum value is 32\n");
+        error_propagate(errp, local_err);
     }
 }
 

[Qemu-devel] [PATCH 06/17] vfio: Pass local error object pointer to error_append_hint()

[Greg Kurz]

Ensure that hints are added even if errp is &error_fatal or &error_abort.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/vfio/common.c |   14 ++++++++++----
 hw/vfio/pci.c    |   12 ++++++++----
 2 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 3e03c495d868..05b1c39a426f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1469,10 +1469,13 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as, Error **errp)
     }
 
     if (!(status.flags & VFIO_GROUP_FLAGS_VIABLE)) {
-        error_setg(errp, "group %d is not viable", groupid);
-        error_append_hint(errp,
+        Error *err = NULL;
+
+        error_setg(&err, "group %d is not viable", groupid);
+        error_append_hint(&err,
                           "Please ensure all devices within the iommu_group "
                           "are bound to their vfio bus driver.\n");
+        error_propagate(errp, err);
         goto close_fd_exit;
     }
 
@@ -1531,11 +1534,14 @@ int vfio_get_device(VFIOGroup *group, const char *name,
 
     fd = ioctl(group->fd, VFIO_GROUP_GET_DEVICE_FD, name);
     if (fd < 0) {
-        error_setg_errno(errp, errno, "error getting device from group %d",
+        Error *err = NULL;
+
+        error_setg_errno(&err, errno, "error getting device from group %d",
                          group->groupid);
-        error_append_hint(errp,
+        error_append_hint(&err,
                       "Verify all devices in group %d are bound to vfio-<bus> "
                       "or pci-stub and not already in use\n", group->groupid);
+        error_propagate(errp, err);
         return fd;
     }
 
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index dc3479c374e3..b037dec2215b 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2527,10 +2527,13 @@ static void vfio_populate_device(VFIOPCIDevice *vdev, Error **errp)
     g_free(reg_info);
 
     if (vdev->features & VFIO_FEATURE_ENABLE_VGA) {
-        ret = vfio_populate_vga(vdev, errp);
+        Error *err = NULL;
+
+        ret = vfio_populate_vga(vdev, &err);
         if (ret) {
-            error_append_hint(errp, "device does not support "
+            error_append_hint(&err, "device does not support "
                               "requested feature x-vga\n");
+            error_propagate(errp, err);
             return;
         }
     }
@@ -2714,9 +2717,10 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
     if (!vdev->vbasedev.sysfsdev) {
         if (!(~vdev->host.domain || ~vdev->host.bus ||
               ~vdev->host.slot || ~vdev->host.function)) {
-            error_setg(errp, "No provided host device");
-            error_append_hint(errp, "Use -device vfio-pci,host=DDDD:BB:DD.F "
+            error_setg(&err, "No provided host device");
+            error_append_hint(&err, "Use -device vfio-pci,host=DDDD:BB:DD.F "
                               "or -device vfio-pci,sysfsdev=PATH_TO_DEVICE\n");
+            error_propagate(errp, err);
             return;
         }
         vdev->vbasedev.sysfsdev =

[Qemu-devel] [PATCH 07/17] virtio-pci: Pass local error object pointer to error_append_hint()

[Greg Kurz]

Ensure that hints are added even if errp is &error_fatal or &error_abort.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/virtio/virtio-pci.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index c6b47a9c7385..1e58822ba186 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -1543,9 +1543,12 @@ static void virtio_pci_device_plugged(DeviceState *d, Error **errp)
         virtio_pci_disable_modern(proxy);
 
         if (!legacy) {
-            error_setg(errp, "Device doesn't support modern mode, and legacy"
+            Error *local_err = NULL;
+
+            error_setg(&local_err, "Device doesn't support modern mode, and legacy"
                              " mode is disabled");
-            error_append_hint(errp, "Set disable-legacy to off\n");
+            error_append_hint(&local_err, "Set disable-legacy to off\n");
+            error_propagate(errp, local_err);
 
             return;
         }
@@ -1737,10 +1740,13 @@ static void virtio_pci_realize(PCIDevice *pci_dev, Error **errp)
     }
 
     if (!virtio_pci_modern(proxy) && !virtio_pci_legacy(proxy)) {
-        error_setg(errp, "device cannot work as neither modern nor legacy mode"
+        Error *local_err = NULL;
+
+        error_setg(&local_err, "device cannot work as neither modern nor legacy mode"
                    " is enabled");
-        error_append_hint(errp, "Set either disable-modern or disable-legacy"
+        error_append_hint(&local_err, "Set either disable-modern or disable-legacy"
                           " to off\n");
+        error_propagate(errp, local_err);
         return;
     }
 

[Qemu-devel] [PATCH 08/17] pcie_root_port: Pass local error object pointer to error_append_hint()

[Greg Kurz]

Ensure that hints are added even if errp is &error_fatal or &error_abort.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/pci-bridge/pcie_root_port.c |   11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 012c2cb12c10..086f9c17723d 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -65,15 +65,17 @@ static void rp_realize(PCIDevice *d, Error **errp)
     PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(d);
     PCIERootPortClass *rpc = PCIE_ROOT_PORT_GET_CLASS(d);
     int rc;
+    Error *local_err = NULL;
 
     pci_config_set_interrupt_pin(d->config, 1);
     pci_bridge_initfn(d, TYPE_PCIE_BUS);
     pcie_port_init_reg(d);
 
     rc = pci_bridge_ssvid_init(d, rpc->ssvid_offset, dc->vendor_id,
-                               rpc->ssid, errp);
+                               rpc->ssid, &local_err);
     if (rc < 0) {
-        error_append_hint(errp, "Can't init SSV ID, error %d\n", rc);
+        error_append_hint(&local_err, "Can't init SSV ID, error %d\n", rc);
+        error_propagate(errp, local_err);
         goto err_bridge;
     }
 
@@ -85,10 +87,11 @@ static void rp_realize(PCIDevice *d, Error **errp)
     }
 
     rc = pcie_cap_init(d, rpc->exp_offset, PCI_EXP_TYPE_ROOT_PORT,
-                       p->port, errp);
+                       p->port, &local_err);
     if (rc < 0) {
-        error_append_hint(errp, "Can't add Root Port capability, "
+        error_append_hint(&local_err, "Can't add Root Port capability, "
                           "error %d\n", rc);
+        error_propagate(errp, local_err);
         goto err_int;
     }
 

[Qemu-devel] [PATCH 09/17] hw/rdma: Fix missing conversion to rdma_error_report()

[Greg Kurz]

Commit 4d71b38ae8fa converted many error_setg() call sites to
rdma_error_report(), but it forgot to convert a companion
error_append_hint(). Since no guy doesn't set errp anymore in
pvrdma_realize(), errp remains NULL and error_append_hint() does
nothing.

Also error_append_hint() was a poor choice since its "intended use
is adding helpful hints on the human user interface" and "not for
clarifying a confusing error message".

Call rdma_error_report() instead.

Fixes: 4d71b38ae8fa "hw/rdma: Switch to generic error reporting way"
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/rdma/vmw/pvrdma_main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 3e36e130139c..d370ae07ca6a 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -667,7 +667,7 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
 out:
     if (rc) {
         pvrdma_fini(pdev);
-        error_append_hint(errp, "Device failed to load\n");
+        rdma_error_report("Device failed to load");
     }
 }
 

[Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()

[Greg Kurz]

Ensure that hints are added even if errp is &error_fatal or &error_abort.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/s390x/s390-ccw.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/s390-ccw.c b/hw/s390x/s390-ccw.c
index 0c5a5b60bd6b..9566be800a3b 100644
--- a/hw/s390x/s390-ccw.c
+++ b/hw/s390x/s390-ccw.c
@@ -57,11 +57,13 @@ static void s390_ccw_get_dev_info(S390CCWDevice *cdev,
 {
     unsigned int cssid, ssid, devid;
     char dev_path[PATH_MAX] = {0}, *tmp;
+    Error *err = NULL;
 
     if (!sysfsdev) {
-        error_setg(errp, "No host device provided");
-        error_append_hint(errp,
+        error_setg(&err, "No host device provided");
+        error_append_hint(&err,
                           "Use -device vfio-ccw,sysfsdev=PATH_TO_DEVICE\n");
+        error_propagate(errp, err);
         return;
     }
 

[Qemu-devel] [PATCH 11/17] scsi: Pass local error object pointer to error_append_hint()

[Greg Kurz]

Ensure that hints are added even if errp is &error_fatal or &error_abort.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/scsi/scsi-disk.c    |    7 +++++--
 hw/scsi/scsi-generic.c |    7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/hw/scsi/scsi-disk.c b/hw/scsi/scsi-disk.c
index 915641a0f1b4..3d5e257bb66b 100644
--- a/hw/scsi/scsi-disk.c
+++ b/hw/scsi/scsi-disk.c
@@ -2619,9 +2619,12 @@ static void scsi_block_realize(SCSIDevice *dev, Error **errp)
     /* check we are using a driver managing SG_IO (version 3 and after) */
     rc = blk_ioctl(s->qdev.conf.blk, SG_GET_VERSION_NUM, &sg_version);
     if (rc < 0) {
-        error_setg_errno(errp, -rc, "cannot get SG_IO version number");
+        Error *local_err = NULL;
+
+        error_setg_errno(&local_err, -rc, "cannot get SG_IO version number");
         if (rc != -EPERM) {
-            error_append_hint(errp, "Is this a SCSI device?\n");
+            error_append_hint(&local_err, "Is this a SCSI device?\n");
+            error_propagate(errp, local_err);
         }
         goto out;
     }
diff --git a/hw/scsi/scsi-generic.c b/hw/scsi/scsi-generic.c
index e7798ebcd0d4..f6a5b538cbcc 100644
--- a/hw/scsi/scsi-generic.c
+++ b/hw/scsi/scsi-generic.c
@@ -674,9 +674,12 @@ static void scsi_generic_realize(SCSIDevice *s, Error **errp)
     /* check we are using a driver managing SG_IO (version 3 and after */
     rc = blk_ioctl(s->conf.blk, SG_GET_VERSION_NUM, &sg_version);
     if (rc < 0) {
-        error_setg_errno(errp, -rc, "cannot get SG_IO version number");
+        Error *local_err = NULL;
+
+        error_setg_errno(&local_err, -rc, "cannot get SG_IO version number");
         if (rc != -EPERM) {
-            error_append_hint(errp, "Is this a SCSI device?\n");
+            error_append_hint(&local_err, "Is this a SCSI device?\n");
+            error_propagate(errp, local_err);
         }
         return;
     }

[Qemu-devel] [PATCH 12/17] migration: Pass local error object pointer to error_append_hint()

[Greg Kurz]

Ensure that hints are added even if errp is &error_fatal or &error_abort.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 migration/migration.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 01863a95f5fe..6724173ce34e 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -983,18 +983,24 @@ static bool migrate_caps_check(bool *cap_list,
 
 #ifndef CONFIG_LIVE_BLOCK_MIGRATION
     if (cap_list[MIGRATION_CAPABILITY_BLOCK]) {
-        error_setg(errp, "QEMU compiled without old-style (blk/-b, inc/-i) "
+        Error *local_err = NULL;
+
+        error_setg(&local_err, "QEMU compiled without old-style (blk/-b, inc/-i) "
                    "block migration");
-        error_append_hint(errp, "Use drive_mirror+NBD instead.\n");
+        error_append_hint(&local_err, "Use drive_mirror+NBD instead.\n");
+        error_propagate(errp, local_err);
         return false;
     }
 #endif
 
 #ifndef CONFIG_REPLICATION
     if (cap_list[MIGRATION_CAPABILITY_X_COLO]) {
-        error_setg(errp, "QEMU compiled without replication module"
+        Error *local_err = NULL;
+
+        error_setg(&local_err, "QEMU compiled without replication module"
                    " can't enable COLO");
-        error_append_hint(errp, "Please enable replication before COLO.\n");
+        error_append_hint(&local_err, "Please enable replication before COLO.\n");
+        error_propagate(errp, local_err);
         return false;
     }
 #endif

[Qemu-devel] [PATCH 13/17] nbd: Pass local error object pointer to error_append_hint()

[Greg Kurz]

Ensure that hints are added even if errp is &error_fatal or &error_abort.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 nbd/client.c |   24 +++++++++++++-----------
 nbd/server.c |    7 +++++--
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/nbd/client.c b/nbd/client.c
index b9dc829175f9..c6e6e4046fd5 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -154,6 +154,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
                                 bool strict, Error **errp)
 {
     g_autofree char *msg = NULL;
+    Error *local_err = NULL;
 
     if (!(reply->type & (1 << 31))) {
         return 1;
@@ -161,14 +162,14 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
 
     if (reply->length) {
         if (reply->length > NBD_MAX_BUFFER_SIZE) {
-            error_setg(errp, "server error %" PRIu32
+            error_setg(&local_err, "server error %" PRIu32
                        " (%s) message is too long",
                        reply->type, nbd_rep_lookup(reply->type));
             goto err;
         }
         msg = g_malloc(reply->length + 1);
         if (nbd_read(ioc, msg, reply->length, NULL, errp) < 0) {
-            error_prepend(errp, "Failed to read option error %" PRIu32
+            error_prepend(&local_err, "Failed to read option error %" PRIu32
                           " (%s) message: ",
                           reply->type, nbd_rep_lookup(reply->type));
             goto err;
@@ -187,50 +188,51 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
 
     switch (reply->type) {
     case NBD_REP_ERR_POLICY:
-        error_setg(errp, "Denied by server for option %" PRIu32 " (%s)",
+        error_setg(&local_err, "Denied by server for option %" PRIu32 " (%s)",
                    reply->option, nbd_opt_lookup(reply->option));
         break;
 
     case NBD_REP_ERR_INVALID:
-        error_setg(errp, "Invalid parameters for option %" PRIu32 " (%s)",
+        error_setg(&local_err, "Invalid parameters for option %" PRIu32 " (%s)",
                    reply->option, nbd_opt_lookup(reply->option));
         break;
 
     case NBD_REP_ERR_PLATFORM:
-        error_setg(errp, "Server lacks support for option %" PRIu32 " (%s)",
+        error_setg(&local_err, "Server lacks support for option %" PRIu32 " (%s)",
                    reply->option, nbd_opt_lookup(reply->option));
         break;
 
     case NBD_REP_ERR_TLS_REQD:
-        error_setg(errp, "TLS negotiation required before option %" PRIu32
+        error_setg(&local_err, "TLS negotiation required before option %" PRIu32
                    " (%s)", reply->option, nbd_opt_lookup(reply->option));
         break;
 
     case NBD_REP_ERR_UNKNOWN:
-        error_setg(errp, "Requested export not available");
+        error_setg(&local_err, "Requested export not available");
         break;
 
     case NBD_REP_ERR_SHUTDOWN:
-        error_setg(errp, "Server shutting down before option %" PRIu32 " (%s)",
+        error_setg(&local_err, "Server shutting down before option %" PRIu32 " (%s)",
                    reply->option, nbd_opt_lookup(reply->option));
         break;
 
     case NBD_REP_ERR_BLOCK_SIZE_REQD:
-        error_setg(errp, "Server requires INFO_BLOCK_SIZE for option %" PRIu32
+        error_setg(&local_err, "Server requires INFO_BLOCK_SIZE for option %" PRIu32
                    " (%s)", reply->option, nbd_opt_lookup(reply->option));
         break;
 
     default:
-        error_setg(errp, "Unknown error code when asking for option %" PRIu32
+        error_setg(&local_err, "Unknown error code when asking for option %" PRIu32
                    " (%s)", reply->option, nbd_opt_lookup(reply->option));
         break;
     }
 
     if (msg) {
-        error_append_hint(errp, "server reported: %s\n", msg);
+        error_append_hint(&local_err, "server reported: %s\n", msg);
     }
 
  err:
+    error_propagate(errp, local_err);
     nbd_send_opt_abort(ioc);
     return -1;
 }
diff --git a/nbd/server.c b/nbd/server.c
index 28c3c8be854c..6d9ca2563cce 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1616,6 +1616,8 @@ void nbd_export_close(NBDExport *exp)
 
 void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
 {
+    Error *local_err = NULL;
+
     if (mode == NBD_SERVER_REMOVE_MODE_HARD || QTAILQ_EMPTY(&exp->clients)) {
         nbd_export_close(exp);
         return;
@@ -1623,8 +1625,9 @@ void nbd_export_remove(NBDExport *exp, NbdServerRemoveMode mode, Error **errp)
 
     assert(mode == NBD_SERVER_REMOVE_MODE_SAFE);
 
-    error_setg(errp, "export '%s' still in use", exp->name);
-    error_append_hint(errp, "Use mode='hard' to force client disconnect\n");
+    error_setg(&local_err, "export '%s' still in use", exp->name);
+    error_append_hint(&local_err, "Use mode='hard' to force client disconnect\n");
+    error_propagate(errp, local_err);
 }
 
 void nbd_export_get(NBDExport *exp)

[Qemu-devel] [PATCH 14/17] ccid-card-emul: Pass local error object pointer to error_append_hint()

[Greg Kurz]

Ensure that hints are added even if errp is &error_fatal or &error_abort.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/usb/ccid-card-emulated.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/usb/ccid-card-emulated.c b/hw/usb/ccid-card-emulated.c
index 291e41db8a1e..3bc397341090 100644
--- a/hw/usb/ccid-card-emulated.c
+++ b/hw/usb/ccid-card-emulated.c
@@ -511,10 +511,13 @@ static void emulated_realize(CCIDCardState *base, Error **errp)
     }
 
     if (card->backend == 0) {
-        error_setg(errp, "backend must be one of:");
+        Error *local_err = NULL;
+
+        error_setg(&local_err, "backend must be one of:");
         for (ptable = backend_enum_table; ptable->name != NULL; ++ptable) {
-            error_append_hint(errp, "%s\n", ptable->name);
+            error_append_hint(&local_err, "%s\n", ptable->name);
         }
+        error_propagate(errp, local_err);
         goto out2;
     }
 

[Qemu-devel] [PATCH 15/17] option: Pass local error object pointer to error_append_hint()

[Greg Kurz]

Ensure that hints are added even if errp is &error_fatal or &error_abort.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 util/qemu-option.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 97172b5eaa7f..2a45dfa585d4 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -155,11 +155,14 @@ void parse_option_size(const char *name, const char *value,
         return;
     }
     if (err) {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, name,
+        Error *local_err = NULL;
+
+        error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, name,
                    "a non-negative number below 2^64");
-        error_append_hint(errp, "Optional suffix k, M, G, T, P or E means"
+        error_append_hint(&local_err, "Optional suffix k, M, G, T, P or E means"
                           " kilo-, mega-, giga-, tera-, peta-\n"
                           "and exabytes, respectively.\n");
+        error_propagate(errp, local_err);
         return;
     }
     *ret = size;
@@ -664,10 +667,13 @@ QemuOpts *qemu_opts_create(QemuOptsList *list, const char *id,
 
     if (id) {
         if (!id_wellformed(id)) {
-            error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "id",
+            Error *local_err = NULL;
+
+            error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "id",
                        "an identifier");
-            error_append_hint(errp, "Identifiers consist of letters, digits, "
+            error_append_hint(&local_err, "Identifiers consist of letters, digits, "
                               "'-', '.', '_', starting with a letter.\n");
+            error_propagate(errp, local_err);
             return NULL;
         }
         opts = qemu_opts_find(list, id);

[Qemu-devel] [PATCH 16/17] socket: Pass local error object pointer to error_append_hint()

[Greg Kurz]

Ensure that hints are added even if errp is &error_fatal or &error_abort.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 util/qemu-sockets.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 98ff3a1cce64..5da7b5f2fa0d 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -882,9 +882,12 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
 
     pathlen = strlen(path);
     if (pathlen > sizeof(un.sun_path)) {
-        error_setg(errp, "UNIX socket path '%s' is too long", path);
-        error_append_hint(errp, "Path must be less than %zu bytes\n",
+        Error *local_err = NULL;
+
+        error_setg(&local_err, "UNIX socket path '%s' is too long", path);
+        error_append_hint(&local_err, "Path must be less than %zu bytes\n",
                           sizeof(un.sun_path));
+        error_propagate(errp, local_err);
         goto err;
     }
 
@@ -952,9 +955,12 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp)
 
     pathlen = strlen(saddr->path);
     if (pathlen > sizeof(un.sun_path)) {
-        error_setg(errp, "UNIX socket path '%s' is too long", saddr->path);
-        error_append_hint(errp, "Path must be less than %zu bytes\n",
+        Error *local_err = NULL;
+
+        error_setg(&local_err, "UNIX socket path '%s' is too long", saddr->path);
+        error_append_hint(&local_err, "Path must be less than %zu bytes\n",
                           sizeof(un.sun_path));
+        error_propagate(errp, local_err);
         goto err;
     }
 

[Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed to error_append_hint()

[Greg Kurz]

Passing errp from the argument list to error_append_hint()
isn't recommended because it may cause unwanted behaviours
when errp is equal to &error_fatal or &error_abort.

First, error_append_hint() aborts QEMU when passed either of
those.

Second, consider the following:

    void foo(Error **errp)
    {
         error_setg(errp, "foo");
         error_append_hint(errp, "Try bar\n");
    }

error_setg() causes QEMU to exit or abort, and hints aren't
added.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 scripts/checkpatch.pl |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index aa9a354a0e7e..17ce026282a6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2902,6 +2902,10 @@ sub process {
 		}
 	}
 
+		if ($line =~ /error_append_hint\(errp/) {
+		    WARN("suspicious errp passed to error_append_hint()\n" .
+			 $herecurr);
+		}
 # check for non-portable libc calls that have portable alternatives in QEMU
 		if ($line =~ /\bffs\(/) {
 			ERROR("use ctz32() instead of ffs()\n" . $herecurr);

Re: [Qemu-devel] [PATCH 12/17] migration: Pass local error object pointer to error_append_hint()

[Dr. David Alan Gilbert]

* Greg Kurz (groug@kaod.org) wrote:
> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

A bit painful, but if it solves the problem,


Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c |   14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 01863a95f5fe..6724173ce34e 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -983,18 +983,24 @@ static bool migrate_caps_check(bool *cap_list,
>  
>  #ifndef CONFIG_LIVE_BLOCK_MIGRATION
>      if (cap_list[MIGRATION_CAPABILITY_BLOCK]) {
> -        error_setg(errp, "QEMU compiled without old-style (blk/-b, inc/-i) "
> +        Error *local_err = NULL;
> +
> +        error_setg(&local_err, "QEMU compiled without old-style (blk/-b, inc/-i) "
>                     "block migration");
> -        error_append_hint(errp, "Use drive_mirror+NBD instead.\n");
> +        error_append_hint(&local_err, "Use drive_mirror+NBD instead.\n");
> +        error_propagate(errp, local_err);
>          return false;
>      }
>  #endif
>  
>  #ifndef CONFIG_REPLICATION
>      if (cap_list[MIGRATION_CAPABILITY_X_COLO]) {
> -        error_setg(errp, "QEMU compiled without replication module"
> +        Error *local_err = NULL;
> +
> +        error_setg(&local_err, "QEMU compiled without replication module"
>                     " can't enable COLO");
> -        error_append_hint(errp, "Please enable replication before COLO.\n");
> +        error_append_hint(&local_err, "Please enable replication before COLO.\n");
> +        error_propagate(errp, local_err);
>          return false;
>      }
>  #endif
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed to error_append_hint()

[Philippe Mathieu-Daudé]

On 9/17/19 12:22 PM, Greg Kurz wrote:
> Passing errp from the argument list to error_append_hint()
> isn't recommended because it may cause unwanted behaviours
> when errp is equal to &error_fatal or &error_abort.
> 
> First, error_append_hint() aborts QEMU when passed either of
> those.
> 
> Second, consider the following:
> 
>     void foo(Error **errp)
>     {
>          error_setg(errp, "foo");
>          error_append_hint(errp, "Try bar\n");
>     }
> 
> error_setg() causes QEMU to exit or abort, and hints aren't
> added.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  scripts/checkpatch.pl |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index aa9a354a0e7e..17ce026282a6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2902,6 +2902,10 @@ sub process {
>  		}
>  	}
>  
> +		if ($line =~ /error_append_hint\(errp/) {

Checking for 'errp' variable name seems fragile, but all the codebase
uses exactly this name, so it is sufficient.

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

> +		    WARN("suspicious errp passed to error_append_hint()\n" .
> +			 $herecurr);
> +		}
>  # check for non-portable libc calls that have portable alternatives in QEMU
>  		if ($line =~ /\bffs\(/) {
>  			ERROR("use ctz32() instead of ffs()\n" . $herecurr);
> 
> 

Re: [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint()

[Philippe Mathieu-Daudé]

For some reason your email client escaped incorrectly Daniel's email:

  "Daniel P. Berrangé\" <berrange@redhat.com>

Which makes my email client very unhappy (Thunderbird):

There are non-ASCII characters in the local part of the recipient
address "Daniel P. Berrangé <berrange@redhat.com>,
qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
qemu-s390x@nongnu.org"@d06av23.portsmouth.uk.ibm.com. This is not yet
supported. Please change this address and try again.

Neither is MTA:

An error occurred while sending mail. The mail server responded:
5.1.2 The recipient address <qemu-s390x@nongnu.org@d06av22.portsmouth.uk.ibm
5.1.2 .com> is not a valid RFC-5321 address. r28sm2465872wrr.94 - gsmtp.
 Please check the message recipient
""qemu-s390x@nongnu.org"@d06av22.portsmouth.uk.ibm.com" and try again.

Re: [Qemu-devel] [PATCH 01/17] error: Update error_append_hint()'s documentation

[Cornelia Huck]

On Tue, 17 Sep 2019 12:20:37 +0200
Greg Kurz <groug@kaod.org> wrote:

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

It's a bit awkward requiring a local object, but better safe than sorry.

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

Re: [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()

[Cornelia Huck]

On Tue, 17 Sep 2019 12:21:34 +0200
Greg Kurz <groug@kaod.org> wrote:

> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/s390x/s390-ccw.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

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

Can also take this via the s390 tree, let me know what would work best.

Re: [Qemu-devel] [PATCH 06/17] vfio: Pass local error object pointer to error_append_hint()

[Cornelia Huck]

On Tue, 17 Sep 2019 12:21:08 +0200
Greg Kurz <groug@kaod.org> wrote:

> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/vfio/common.c |   14 ++++++++++----
>  hw/vfio/pci.c    |   12 ++++++++----
>  2 files changed, 18 insertions(+), 8 deletions(-)

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

Re: [Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed to error_append_hint()

[Cornelia Huck]

On Tue, 17 Sep 2019 12:22:18 +0200
Greg Kurz <groug@kaod.org> wrote:

> Passing errp from the argument list to error_append_hint()
> isn't recommended because it may cause unwanted behaviours
> when errp is equal to &error_fatal or &error_abort.
> 
> First, error_append_hint() aborts QEMU when passed either of
> those.
> 
> Second, consider the following:
> 
>     void foo(Error **errp)
>     {
>          error_setg(errp, "foo");
>          error_append_hint(errp, "Try bar\n");
>     }
> 
> error_setg() causes QEMU to exit or abort, and hints aren't
> added.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  scripts/checkpatch.pl |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index aa9a354a0e7e..17ce026282a6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2902,6 +2902,10 @@ sub process {
>  		}
>  	}
>  

Maybe add a comment here?

# using errp is common practice, so that check should hopefully be enough

> +		if ($line =~ /error_append_hint\(errp/) {
> +		    WARN("suspicious errp passed to error_append_hint()\n" .

Add "(use a local error object)"?

> +			 $herecurr);
> +		}
>  # check for non-portable libc calls that have portable alternatives in QEMU
>  		if ($line =~ /\bffs\(/) {
>  			ERROR("use ctz32() instead of ffs()\n" . $herecurr);
> 

Re: [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()

[David Hildenbrand]

On 17.09.19 13:24, Cornelia Huck wrote:
> On Tue, 17 Sep 2019 12:21:34 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>>
>> Signed-off-by: Greg Kurz <groug@kaod.org>
>> ---
>>  hw/s390x/s390-ccw.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> Can also take this via the s390 tree, let me know what would work best.
> 

Thanks for fixing the cc list :)

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

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint()

[Greg Kurz]

On Tue, 17 Sep 2019 13:00:37 +0200
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> For some reason your email client escaped incorrectly Daniel's email:
> 
>   "Daniel P. Berrangé\" <berrange@redhat.com>
> 

The client is stgit's "stg mail" command, and indeed it generates this:

 "Daniel P. =?utf-8?q?Berrang=C3=A9=22?= <berrange@redhat.com>
                                   ^^^
                                double-quote

and if I turn the 'é' into a 'e':

 "Daniel P. Berrange" <berrange@redhat.com>

This looks like a bug in "stg mail"...

> Which makes my email client very unhappy (Thunderbird):
> 
> There are non-ASCII characters in the local part of the recipient
> address "Daniel P. Berrangé <berrange@redhat.com>,
> qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
> qemu-s390x@nongnu.org"@d06av23.portsmouth.uk.ibm.com. This is not yet
> supported. Please change this address and try again.
> 
> Neither is MTA:
> 
> An error occurred while sending mail. The mail server responded:
> 5.1.2 The recipient address <qemu-s390x@nongnu.org@d06av22.portsmouth.uk.ibm
> 5.1.2 .com> is not a valid RFC-5321 address. r28sm2465872wrr.94 - gsmtp.
>  Please check the message recipient
> ""qemu-s390x@nongnu.org"@d06av22.portsmouth.uk.ibm.com" and try again.

Drat... this may prevent recipients starting with Daniel from receiving
the mails... ie, the sub-lists IIUC. Maybe not worth reposting for that.
Hopefully, Daniel may catch up on qemu-devel.

Re: [Qemu-devel] [PATCH 17/17] checkpatch: Warn when errp is passed to error_append_hint()

[Greg Kurz]

On Tue, 17 Sep 2019 13:29:36 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 17 Sep 2019 12:22:18 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > Passing errp from the argument list to error_append_hint()
> > isn't recommended because it may cause unwanted behaviours
> > when errp is equal to &error_fatal or &error_abort.
> > 
> > First, error_append_hint() aborts QEMU when passed either of
> > those.
> > 
> > Second, consider the following:
> > 
> >     void foo(Error **errp)
> >     {
> >          error_setg(errp, "foo");
> >          error_append_hint(errp, "Try bar\n");
> >     }
> > 
> > error_setg() causes QEMU to exit or abort, and hints aren't
> > added.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  scripts/checkpatch.pl |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index aa9a354a0e7e..17ce026282a6 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2902,6 +2902,10 @@ sub process {
> >  		}
> >  	}
> >  
> 
> Maybe add a comment here?
> 
> # using errp is common practice, so that check should hopefully be enough
> 
> > +		if ($line =~ /error_append_hint\(errp/) {
> > +		    WARN("suspicious errp passed to error_append_hint()\n" .
> 
> Add "(use a local error object)"?
> 

Sure. I'll add both.

> > +			 $herecurr);
> > +		}
> >  # check for non-portable libc calls that have portable alternatives in QEMU
> >  		if ($line =~ /\bffs\(/) {
> >  			ERROR("use ctz32() instead of ffs()\n" . $herecurr);
> > 
> 

Re: [Qemu-devel] [PATCH 00/17] Fix usage of error_append_hint()

[Daniel P. Berrangé]

On Tue, Sep 17, 2019 at 01:45:29PM +0200, Greg Kurz wrote:
> On Tue, 17 Sep 2019 13:00:37 +0200
> Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
> 
> > For some reason your email client escaped incorrectly Daniel's email:
> > 
> >   "Daniel P. Berrangé\" <berrange@redhat.com>
> > 
> 
> The client is stgit's "stg mail" command, and indeed it generates this:
> 
>  "Daniel P. =?utf-8?q?Berrang=C3=A9=22?= <berrange@redhat.com>
>                                    ^^^
>                                 double-quote
> 
> and if I turn the 'é' into a 'e':
> 
>  "Daniel P. Berrange" <berrange@redhat.com>
> 
> This looks like a bug in "stg mail"...

Happy to see that my name is detecting bugs :-)

I should add an emoji to it to really flush out the edge cases...

> > Which makes my email client very unhappy (Thunderbird):
> > 
> > There are non-ASCII characters in the local part of the recipient
> > address "Daniel P. Berrangé <berrange@redhat.com>,
> > qemu-block@nongnu.org, qemu-arm@nongnu.org, qemu-ppc@nongnu.org,
> > qemu-s390x@nongnu.org"@d06av23.portsmouth.uk.ibm.com. This is not yet
> > supported. Please change this address and try again.
> > 
> > Neither is MTA:
> > 
> > An error occurred while sending mail. The mail server responded:
> > 5.1.2 The recipient address <qemu-s390x@nongnu.org@d06av22.portsmouth.uk.ibm
> > 5.1.2 .com> is not a valid RFC-5321 address. r28sm2465872wrr.94 - gsmtp.
> >  Please check the message recipient
> > ""qemu-s390x@nongnu.org"@d06av22.portsmouth.uk.ibm.com" and try again.
> 
> Drat... this may prevent recipients starting with Daniel from receiving
> the mails... ie, the sub-lists IIUC. Maybe not worth reposting for that.

Yeah, I wouldn't bother reposting just for that.

> Hopefully, Daniel may catch up on qemu-devel.

Indeed.

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

Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()

[Vladimir Sementsov-Ogievskiy]

17.09.2019 13:20, Greg Kurz wrote:
> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>   block/backup.c       |    7 +++++--
>   block/dirty-bitmap.c |    7 +++++--
>   block/file-posix.c   |   20 +++++++++++++-------
>   block/gluster.c      |   23 +++++++++++++++--------
>   block/qcow.c         |   10 ++++++----
>   block/qcow2.c        |    7 +++++--
>   block/vhdx-log.c     |    7 +++++--
>   block/vpc.c          |    7 +++++--
>   8 files changed, 59 insertions(+), 29 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 763f0d7ff6db..d8c422a0e3bc 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
>                       BACKUP_CLUSTER_SIZE_DEFAULT);
>           return BACKUP_CLUSTER_SIZE_DEFAULT;
>       } else if (ret < 0 && !target->backing) {
> -        error_setg_errno(errp, -ret,
> +        Error *local_err = NULL;
> +
> +        error_setg_errno(&local_err, -ret,
>               "Couldn't determine the cluster size of the target image, "
>               "which has no backing file");
> -        error_append_hint(errp,
> +        error_append_hint(&local_err,
>               "Aborting, since this may create an unusable destination image\n");
> +        error_propagate(errp, local_err);
>           return ret;
>       } else if (ret < 0 && target->backing) {
>           /* Not fatal; just trudge on ahead. */


Pain.. Do we need these hints, if they are so painful?

At least for cases like this, we can create helper function

error_setg_errno_hint(..., error, hint)

But what could be done when we call function, which may or may not set errp?

ret = f(errp);
if (ret) {
    error_append_hint(errp, hint);
}

Hmmm..

Can it look like

ret = f(..., hint_helper(errp, hint))

?

I can't imagine how to do it, as someone should remove hint from error_abort object on
success path..

But seems, the following is possible, which seems better for me than local-error approach:

error_push_hint(errp, hint);
ret = f(.., errp);
error_pop_hint(errp);

===

Continue thinking on this:

It may look like just
ret = f(..., set_hint(errp, hint));

or (just to split long line):
set_hint(errp, hint);
ret = f(..., errp);

if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
which may be just one call at function start of macro, which will call error_push_hint(errp) and
define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
exit..

Or, may be, more direct way to set cleanup for function exists?

===

Also, we can implement some code generation, to generate for functions with errp argument
wrappers with additional hint parameter, and just use these wrappers..

===

If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
real issue and much effort has already been spent. May be one day I'll try to rewrite it...

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 01/17] error: Update error_append_hint()'s documentation

[Eric Blake]

On 9/17/19 5:20 AM, Greg Kurz wrote:
> error_setg() and error_propagate(), as well as their variants, cause
> QEMU to terminate when called with &error_fatal or &error_abort. This
> prevents to add hints since error_append_hint() isn't even called in

s/to add/adding/

> this case.
> 
> It means that error_append_hint() should only be used with a local
> error object, and then propagate this local error to the caller.
> 
> Document this in <qapi/error.h> .
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  include/qapi/error.h |   11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 

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: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()

[Eric Blake]

On 9/17/19 5:20 AM, Greg Kurz wrote:
> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  block/backup.c       |    7 +++++--
>  block/dirty-bitmap.c |    7 +++++--
>  block/file-posix.c   |   20 +++++++++++++-------
>  block/gluster.c      |   23 +++++++++++++++--------
>  block/qcow.c         |   10 ++++++----
>  block/qcow2.c        |    7 +++++--
>  block/vhdx-log.c     |    7 +++++--
>  block/vpc.c          |    7 +++++--
>  8 files changed, 59 insertions(+), 29 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 763f0d7ff6db..d8c422a0e3bc 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
>                      BACKUP_CLUSTER_SIZE_DEFAULT);
>          return BACKUP_CLUSTER_SIZE_DEFAULT;
>      } else if (ret < 0 && !target->backing) {
> -        error_setg_errno(errp, -ret,
> +        Error *local_err = NULL;

Can we go with the shorter name 'err' instead of 'local_err'?  I know,
we aren't consistent (both styles appear throughout the tree), but the
shorter style is more appealing to me.

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

Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()

[Kevin Wolf]

Am 17.09.2019 um 16:39 hat Eric Blake geschrieben:
> On 9/17/19 5:20 AM, Greg Kurz wrote:
> > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  block/backup.c       |    7 +++++--
> >  block/dirty-bitmap.c |    7 +++++--
> >  block/file-posix.c   |   20 +++++++++++++-------
> >  block/gluster.c      |   23 +++++++++++++++--------
> >  block/qcow.c         |   10 ++++++----
> >  block/qcow2.c        |    7 +++++--
> >  block/vhdx-log.c     |    7 +++++--
> >  block/vpc.c          |    7 +++++--
> >  8 files changed, 59 insertions(+), 29 deletions(-)
> > 
> > diff --git a/block/backup.c b/block/backup.c
> > index 763f0d7ff6db..d8c422a0e3bc 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> >                      BACKUP_CLUSTER_SIZE_DEFAULT);
> >          return BACKUP_CLUSTER_SIZE_DEFAULT;
> >      } else if (ret < 0 && !target->backing) {
> > -        error_setg_errno(errp, -ret,
> > +        Error *local_err = NULL;
> 
> Can we go with the shorter name 'err' instead of 'local_err'?  I know,
> we aren't consistent (both styles appear throughout the tree), but the
> shorter style is more appealing to me.

I like local_err better because it's easier to distinguish from errp.
The compiler might catch it if you use the wrong one because one is
Error* and the other is Error**, but as a reviewer, I can still get
confused.

Kevin

Re: [Qemu-devel] [PATCH 09/17] hw/rdma: Fix missing conversion to rdma_error_report()

[Yuval Shaia]

On Tue, Sep 17, 2019 at 12:21:27PM +0200, Greg Kurz wrote:
> Commit 4d71b38ae8fa converted many error_setg() call sites to
> rdma_error_report(), but it forgot to convert a companion
> error_append_hint(). Since no guy doesn't set errp anymore in
> pvrdma_realize(), errp remains NULL and error_append_hint() does
> nothing.
> 
> Also error_append_hint() was a poor choice since its "intended use
> is adding helpful hints on the human user interface" and "not for
> clarifying a confusing error message".
> 
> Call rdma_error_report() instead.

Thanks,
So are you suggesting to replace all other error_setg calls with
rdma_error_report instead?

> 
> Fixes: 4d71b38ae8fa "hw/rdma: Switch to generic error reporting way"
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/rdma/vmw/pvrdma_main.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index 3e36e130139c..d370ae07ca6a 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -667,7 +667,7 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
>  out:
>      if (rc) {
>          pvrdma_fini(pdev);
> -        error_append_hint(errp, "Device failed to load\n");
> +        rdma_error_report("Device failed to load");

Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>

>      }
>  }
>  
> 
> 

Re: [Qemu-devel] [PATCH 13/17] nbd: Pass local error object pointer to error_append_hint()

[Eric Blake]

On 9/17/19 5:21 AM, Greg Kurz wrote:
> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  nbd/client.c |   24 +++++++++++++-----------
>  nbd/server.c |    7 +++++--
>  2 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/nbd/client.c b/nbd/client.c
> index b9dc829175f9..c6e6e4046fd5 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -154,6 +154,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
>                                  bool strict, Error **errp)
>  {
>      g_autofree char *msg = NULL;
> +    Error *local_err = NULL;

I'd prefer 'err' here...

>  
>      if (!(reply->type & (1 << 31))) {
>          return 1;
> @@ -161,14 +162,14 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
>  
>      if (reply->length) {
>          if (reply->length > NBD_MAX_BUFFER_SIZE) {
> -            error_setg(errp, "server error %" PRIu32
> +            error_setg(&local_err, "server error %" PRIu32

so that &err doesn't change line lengths.

>      case NBD_REP_ERR_SHUTDOWN:
> -        error_setg(errp, "Server shutting down before option %" PRIu32 " (%s)",
> +        error_setg(&local_err, "Server shutting down before option %" PRIu32 " (%s)",

For example, here, you went beyond 80 columns.

At any rate, I'm assuming this will probably go in through Markus' error
tree as part of the whole series, rather than me needing to pick just
this patch through my NBD tree.

Whether or not you shorten the local variable name,

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: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()

[Greg Kurz]

On Tue, 17 Sep 2019 13:25:03 +0000
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 17.09.2019 13:20, Greg Kurz wrote:
> > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >   block/backup.c       |    7 +++++--
> >   block/dirty-bitmap.c |    7 +++++--
> >   block/file-posix.c   |   20 +++++++++++++-------
> >   block/gluster.c      |   23 +++++++++++++++--------
> >   block/qcow.c         |   10 ++++++----
> >   block/qcow2.c        |    7 +++++--
> >   block/vhdx-log.c     |    7 +++++--
> >   block/vpc.c          |    7 +++++--
> >   8 files changed, 59 insertions(+), 29 deletions(-)
> > 
> > diff --git a/block/backup.c b/block/backup.c
> > index 763f0d7ff6db..d8c422a0e3bc 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> >                       BACKUP_CLUSTER_SIZE_DEFAULT);
> >           return BACKUP_CLUSTER_SIZE_DEFAULT;
> >       } else if (ret < 0 && !target->backing) {
> > -        error_setg_errno(errp, -ret,
> > +        Error *local_err = NULL;
> > +
> > +        error_setg_errno(&local_err, -ret,
> >               "Couldn't determine the cluster size of the target image, "
> >               "which has no backing file");
> > -        error_append_hint(errp,
> > +        error_append_hint(&local_err,
> >               "Aborting, since this may create an unusable destination image\n");
> > +        error_propagate(errp, local_err);
> >           return ret;
> >       } else if (ret < 0 && target->backing) {
> >           /* Not fatal; just trudge on ahead. */
> 
> 
> Pain.. Do we need these hints, if they are so painful?
> 

I agree that the one above doesn't qualify as a useful hint.
It just tells that QEMU is giving up and gives no indication
to the user on how to avoid the issue. It should probably be
dropped.

> At least for cases like this, we can create helper function
> 
> error_setg_errno_hint(..., error, hint)

Not very useful if hint has to be forged separately with
g_sprintf(), and we can't have such a thing as:

error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...)

> 
> But what could be done when we call function, which may or may not set errp?
> 
> ret = f(errp);
> if (ret) {
>     error_append_hint(errp, hint);
> }
> 

Same problem. If errp is &error_fatal and f() does errno_setg(errp), it
ends up calling exit().

> Hmmm..
> 
> Can it look like
> 
> ret = f(..., hint_helper(errp, hint))
> 
> ?
> 

Nope, hint_helper() will get called before f() and things are worse.
If errp is NULL then error_append_hint() does nothing and if it is
&error_fatal then it aborts.

> I can't imagine how to do it, as someone should remove hint from error_abort object on
> success path..
> 
> But seems, the following is possible, which seems better for me than local-error approach:
> 
> error_push_hint(errp, hint);
> ret = f(.., errp);
> error_pop_hint(errp);
> 

Matter of taste... also, it looks awkward to come up with a hint
before knowing what happened. I mean the appropriate hint could
depend on the value returned by f() and/or errno for example.

> ===
> 
> Continue thinking on this:
> 
> It may look like just
> ret = f(..., set_hint(errp, hint));
> 
> or (just to split long line):
> set_hint(errp, hint);
> ret = f(..., errp);
> 
> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
> which may be just one call at function start of macro, which will call error_push_hint(errp) and
> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
> exit..
> 
> Or, may be, more direct way to set cleanup for function exists?
> 
> ===
> 
> Also, we can implement some code generation, to generate for functions with errp argument
> wrappers with additional hint parameter, and just use these wrappers..
> 
> ===
> 
> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
> real issue and much effort has already been spent. May be one day I'll try to rewrite it...
> 

For the reason exposed above, I don't think it makes sense to
build the hint before calling a function that calls error_setg().
I'm afraid we're stuck with local_err... it is then up to the
people to make it as less painful as possible.

Re: [Qemu-devel] [PATCH 09/17] hw/rdma: Fix missing conversion to rdma_error_report()

[Greg Kurz]

On Tue, 17 Sep 2019 17:51:57 +0300
Yuval Shaia <yuval.shaia@oracle.com> wrote:

> On Tue, Sep 17, 2019 at 12:21:27PM +0200, Greg Kurz wrote:
> > Commit 4d71b38ae8fa converted many error_setg() call sites to
> > rdma_error_report(), but it forgot to convert a companion
> > error_append_hint(). Since no guy doesn't set errp anymore in
> > pvrdma_realize(), errp remains NULL and error_append_hint() does
> > nothing.
> > 
> > Also error_append_hint() was a poor choice since its "intended use
> > is adding helpful hints on the human user interface" and "not for
> > clarifying a confusing error message".
> > 
> > Call rdma_error_report() instead.
> 
> Thanks,
> So are you suggesting to replace all other error_setg calls with
> rdma_error_report instead?
> 

No. I don't know what was the motivation behind 4d71b38ae8fa, I'm
just fixing what seems to be a leftover, which should have been
error_prepend() instead of error_append_hint() actually.

> > 
> > Fixes: 4d71b38ae8fa "hw/rdma: Switch to generic error reporting way"
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/rdma/vmw/pvrdma_main.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> > index 3e36e130139c..d370ae07ca6a 100644
> > --- a/hw/rdma/vmw/pvrdma_main.c
> > +++ b/hw/rdma/vmw/pvrdma_main.c
> > @@ -667,7 +667,7 @@ static void pvrdma_realize(PCIDevice *pdev, Error **errp)
> >  out:
> >      if (rc) {
> >          pvrdma_fini(pdev);
> > -        error_append_hint(errp, "Device failed to load\n");
> > +        rdma_error_report("Device failed to load");
> 
> Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
> 
> >      }
> >  }
> >  
> > 
> > 

Re: [Qemu-devel] [PATCH 13/17] nbd: Pass local error object pointer to error_append_hint()

[Greg Kurz]

On Tue, 17 Sep 2019 10:15:07 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 9/17/19 5:21 AM, Greg Kurz wrote:
> > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  nbd/client.c |   24 +++++++++++++-----------
> >  nbd/server.c |    7 +++++--
> >  2 files changed, 18 insertions(+), 13 deletions(-)
> > 
> > diff --git a/nbd/client.c b/nbd/client.c
> > index b9dc829175f9..c6e6e4046fd5 100644
> > --- a/nbd/client.c
> > +++ b/nbd/client.c
> > @@ -154,6 +154,7 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
> >                                  bool strict, Error **errp)
> >  {
> >      g_autofree char *msg = NULL;
> > +    Error *local_err = NULL;
> 
> I'd prefer 'err' here...
> 
> >  
> >      if (!(reply->type & (1 << 31))) {
> >          return 1;
> > @@ -161,14 +162,14 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
> >  
> >      if (reply->length) {
> >          if (reply->length > NBD_MAX_BUFFER_SIZE) {
> > -            error_setg(errp, "server error %" PRIu32
> > +            error_setg(&local_err, "server error %" PRIu32
> 
> so that &err doesn't change line lengths.
> 
> >      case NBD_REP_ERR_SHUTDOWN:
> > -        error_setg(errp, "Server shutting down before option %" PRIu32 " (%s)",
> > +        error_setg(&local_err, "Server shutting down before option %" PRIu32 " (%s)",
> 
> For example, here, you went beyond 80 columns.
> 
> At any rate, I'm assuming this will probably go in through Markus' error
> tree as part of the whole series, rather than me needing to pick just
> this patch through my NBD tree.
> 
> Whether or not you shorten the local variable name,
> 

Regardless of which tree this goes through, it will be code for
which you're the official maintainer in the end. I'll gladly fix
it to meet your needs :)

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

Re: [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()

[Greg Kurz]

On Tue, 17 Sep 2019 13:24:12 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Tue, 17 Sep 2019 12:21:34 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/s390x/s390-ccw.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> 
> Can also take this via the s390 tree, let me know what would work best.

I guess it would be easier to merge if each individual patch goes
through the corresponding sub-maintainer tree. But Eric mentioned
in another mail that the whole whole series could maybe go through
Markus' error tree... so, I don't know which is best. :)

Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()

[Greg Kurz]

On Tue, 17 Sep 2019 16:46:31 +0200
Kevin Wolf <kwolf@redhat.com> wrote:

> Am 17.09.2019 um 16:39 hat Eric Blake geschrieben:
> > On 9/17/19 5:20 AM, Greg Kurz wrote:
> > > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  block/backup.c       |    7 +++++--
> > >  block/dirty-bitmap.c |    7 +++++--
> > >  block/file-posix.c   |   20 +++++++++++++-------
> > >  block/gluster.c      |   23 +++++++++++++++--------
> > >  block/qcow.c         |   10 ++++++----
> > >  block/qcow2.c        |    7 +++++--
> > >  block/vhdx-log.c     |    7 +++++--
> > >  block/vpc.c          |    7 +++++--
> > >  8 files changed, 59 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/block/backup.c b/block/backup.c
> > > index 763f0d7ff6db..d8c422a0e3bc 100644
> > > --- a/block/backup.c
> > > +++ b/block/backup.c
> > > @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> > >                      BACKUP_CLUSTER_SIZE_DEFAULT);
> > >          return BACKUP_CLUSTER_SIZE_DEFAULT;
> > >      } else if (ret < 0 && !target->backing) {
> > > -        error_setg_errno(errp, -ret,
> > > +        Error *local_err = NULL;
> > 
> > Can we go with the shorter name 'err' instead of 'local_err'?  I know,
> > we aren't consistent (both styles appear throughout the tree), but the
> > shorter style is more appealing to me.
> 
> I like local_err better because it's easier to distinguish from errp.
> The compiler might catch it if you use the wrong one because one is
> Error* and the other is Error**, but as a reviewer, I can still get
> confused.
> 

I'll favor the official maintainer, hence keeping local_err :)

> Kevin

Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()

[Vladimir Sementsov-Ogievskiy]

17.09.2019 18:37, Greg Kurz wrote:
> On Tue, 17 Sep 2019 13:25:03 +0000
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> 17.09.2019 13:20, Greg Kurz wrote:
>>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>> ---
>>>    block/backup.c       |    7 +++++--
>>>    block/dirty-bitmap.c |    7 +++++--
>>>    block/file-posix.c   |   20 +++++++++++++-------
>>>    block/gluster.c      |   23 +++++++++++++++--------
>>>    block/qcow.c         |   10 ++++++----
>>>    block/qcow2.c        |    7 +++++--
>>>    block/vhdx-log.c     |    7 +++++--
>>>    block/vpc.c          |    7 +++++--
>>>    8 files changed, 59 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 763f0d7ff6db..d8c422a0e3bc 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
>>>                        BACKUP_CLUSTER_SIZE_DEFAULT);
>>>            return BACKUP_CLUSTER_SIZE_DEFAULT;
>>>        } else if (ret < 0 && !target->backing) {
>>> -        error_setg_errno(errp, -ret,
>>> +        Error *local_err = NULL;
>>> +
>>> +        error_setg_errno(&local_err, -ret,
>>>                "Couldn't determine the cluster size of the target image, "
>>>                "which has no backing file");
>>> -        error_append_hint(errp,
>>> +        error_append_hint(&local_err,
>>>                "Aborting, since this may create an unusable destination image\n");
>>> +        error_propagate(errp, local_err);
>>>            return ret;
>>>        } else if (ret < 0 && target->backing) {
>>>            /* Not fatal; just trudge on ahead. */
>>
>>
>> Pain.. Do we need these hints, if they are so painful?
>>
> 
> I agree that the one above doesn't qualify as a useful hint.
> It just tells that QEMU is giving up and gives no indication
> to the user on how to avoid the issue. It should probably be
> dropped.
> 
>> At least for cases like this, we can create helper function
>>
>> error_setg_errno_hint(..., error, hint)
> 
> Not very useful if hint has to be forged separately with
> g_sprintf(), and we can't have such a thing as:
> 
> error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...)
> 
>>
>> But what could be done when we call function, which may or may not set errp?
>>
>> ret = f(errp);
>> if (ret) {
>>      error_append_hint(errp, hint);
>> }
>>
> 
> Same problem. If errp is &error_fatal and f() does errno_setg(errp), it
> ends up calling exit().
> 
>> Hmmm..
>>
>> Can it look like
>>
>> ret = f(..., hint_helper(errp, hint))
>>
>> ?
>>
> 
> Nope, hint_helper() will get called before f() and things are worse.
> If errp is NULL then error_append_hint() does nothing and if it is
> &error_fatal then it aborts.
> 
>> I can't imagine how to do it, as someone should remove hint from error_abort object on
>> success path..
>>
>> But seems, the following is possible, which seems better for me than local-error approach:
>>
>> error_push_hint(errp, hint);
>> ret = f(.., errp);
>> error_pop_hint(errp);
>>
> 
> Matter of taste... also, it looks awkward to come up with a hint
> before knowing what happened. I mean the appropriate hint could
> depend on the value returned by f() and/or errno for example.
> 
>> ===
>>
>> Continue thinking on this:
>>
>> It may look like just
>> ret = f(..., set_hint(errp, hint));
>>
>> or (just to split long line):
>> set_hint(errp, hint);
>> ret = f(..., errp);
>>
>> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
>> which may be just one call at function start of macro, which will call error_push_hint(errp) and
>> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
>> exit..
>>
>> Or, may be, more direct way to set cleanup for function exists?
>>
>> ===
>>
>> Also, we can implement some code generation, to generate for functions with errp argument
>> wrappers with additional hint parameter, and just use these wrappers..
>>
>> ===
>>
>> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
>> real issue and much effort has already been spent. May be one day I'll try to rewrite it...
>>
> 
> For the reason exposed above, I don't think it makes sense to
> build the hint before calling a function that calls error_setg().
> I'm afraid we're stuck with local_err... it is then up to the
> people to make it as less painful as possible.
> 

Hmm. so, seems that in general we need local_err..

Then may be, may can make automated propagation?

It will look like

g_auto(ErrorPropagation) _error_prop = (ErrorPropagation){
   .errp = errp,
   .local_err = NULL,
}

errp = &_error_prop.local_err;

and this thing may be fully covered into macro,
to look like this at function start (to be honest it should exactly after all
local variable definitions):

MAKE_ERRP_SAFE(_error_prop, errp);


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()

[John Snow]

On 9/17/19 10:46 AM, Kevin Wolf wrote:
> Am 17.09.2019 um 16:39 hat Eric Blake geschrieben:
>> On 9/17/19 5:20 AM, Greg Kurz wrote:
>>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>>>
>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>> ---
>>>  block/backup.c       |    7 +++++--
>>>  block/dirty-bitmap.c |    7 +++++--
>>>  block/file-posix.c   |   20 +++++++++++++-------
>>>  block/gluster.c      |   23 +++++++++++++++--------
>>>  block/qcow.c         |   10 ++++++----
>>>  block/qcow2.c        |    7 +++++--
>>>  block/vhdx-log.c     |    7 +++++--
>>>  block/vpc.c          |    7 +++++--
>>>  8 files changed, 59 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/block/backup.c b/block/backup.c
>>> index 763f0d7ff6db..d8c422a0e3bc 100644
>>> --- a/block/backup.c
>>> +++ b/block/backup.c
>>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
>>>                      BACKUP_CLUSTER_SIZE_DEFAULT);
>>>          return BACKUP_CLUSTER_SIZE_DEFAULT;
>>>      } else if (ret < 0 && !target->backing) {
>>> -        error_setg_errno(errp, -ret,
>>> +        Error *local_err = NULL;
>>
>> Can we go with the shorter name 'err' instead of 'local_err'?  I know,
>> we aren't consistent (both styles appear throughout the tree), but the
>> shorter style is more appealing to me.
> 
> I like local_err better because it's easier to distinguish from errp.
> The compiler might catch it if you use the wrong one because one is
> Error* and the other is Error**, but as a reviewer, I can still get
> confused.
> 
> Kevin
> 

Doesn't that sound like a striking reason for condemnation of this
entire model?

What's going to prevent us from regressing on this technicality in the
future?

Re: [Qemu-devel] [PATCH 04/17] ppc: Pass local error object pointer to error_append_hint()

[David Gibson]

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

On Tue, Sep 17, 2019 at 12:20:56PM +0200, Greg Kurz wrote:
> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

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

> ---
>  hw/ppc/mac_newworld.c |    7 +++++--
>  hw/ppc/spapr.c        |    7 +++++--
>  hw/ppc/spapr_pci.c    |    9 +++++----
>  target/ppc/kvm.c      |   13 +++++++++----
>  4 files changed, 24 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index c5bbcc743352..aca8c40bf395 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -618,8 +618,11 @@ static void core99_set_via_config(Object *obj, const char *value, Error **errp)
>      } else if (!strcmp(value, "pmu-adb")) {
>          cms->via_config = CORE99_VIA_CONFIG_PMU_ADB;
>      } else {
> -        error_setg(errp, "Invalid via value");
> -        error_append_hint(errp, "Valid values are cuda, pmu, pmu-adb.\n");
> +        Error *local_err = NULL;
> +
> +        error_setg(&local_err, "Invalid via value");
> +        error_append_hint(&local_err, "Valid values are cuda, pmu, pmu-adb.\n");
> +        error_propagate(errp, local_err);
>      }
>  }
>  
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 08a2a5a77092..39d6f57d014e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4337,10 +4337,13 @@ void spapr_set_vcpu_id(PowerPCCPU *cpu, int cpu_index, Error **errp)
>      vcpu_id = spapr_vcpu_id(spapr, cpu_index);
>  
>      if (kvm_enabled() && !kvm_vcpu_id_is_valid(vcpu_id)) {
> -        error_setg(errp, "Can't create CPU with id %d in KVM", vcpu_id);
> -        error_append_hint(errp, "Adjust the number of cpus to %d "
> +        Error *local_err = NULL;
> +
> +        error_setg(&local_err, "Can't create CPU with id %d in KVM", vcpu_id);
> +        error_append_hint(&local_err, "Adjust the number of cpus to %d "
>                            "or try to raise the number of threads per core\n",
>                            vcpu_id * ms->smp.threads / spapr->vsmt);
> +        error_propagate(errp, local_err);
>          return;
>      }
>  
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 7b71ad7c74f1..4b7e9a1c8666 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -1870,12 +1870,13 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
>      if (spapr_pci_find_phb(spapr, sphb->buid)) {
>          SpaprPhbState *s;
>  
> -        error_setg(errp, "PCI host bridges must have unique indexes");
> -        error_append_hint(errp, "The following indexes are already in use:");
> +        error_setg(&local_err, "PCI host bridges must have unique indexes");
> +        error_append_hint(&local_err, "The following indexes are already in use:");
>          QLIST_FOREACH(s, &spapr->phbs, list) {
> -            error_append_hint(errp, " %d", s->index);
> +            error_append_hint(&local_err, " %d", s->index);
>          }
> -        error_append_hint(errp, "\nTry another value for the index property\n");
> +        error_append_hint(&local_err, "\nTry another value for the index property\n");
> +        error_propagate(errp, local_err);
>          return;
>      }
>  
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 8c5b1f25cc95..c6876b08c726 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -242,8 +242,11 @@ static void kvm_get_smmu_info(struct kvm_ppc_smmu_info *info, Error **errp)
>      assert(kvm_state != NULL);
>  
>      if (!kvm_check_extension(kvm_state, KVM_CAP_PPC_GET_SMMU_INFO)) {
> -        error_setg(errp, "KVM doesn't expose the MMU features it supports");
> -        error_append_hint(errp, "Consider switching to a newer KVM\n");
> +        Error *local_err = NULL;
> +
> +        error_setg(&local_err, "KVM doesn't expose the MMU features it supports");
> +        error_append_hint(&local_err, "Consider switching to a newer KVM\n");
> +        error_propagate(errp, local_err);
>          return;
>      }
>  
> @@ -2076,6 +2079,7 @@ void kvmppc_hint_smt_possible(Error **errp)
>      int i;
>      GString *g;
>      char *s;
> +    Error *local_err = NULL;
>  
>      assert(kvm_enabled());
>      if (cap_ppc_smt_possible) {
> @@ -2086,12 +2090,13 @@ void kvmppc_hint_smt_possible(Error **errp)
>              }
>          }
>          s = g_string_free(g, false);
> -        error_append_hint(errp, "%s.\n", s);
> +        error_append_hint(&local_err, "%s.\n", s);
>          g_free(s);
>      } else {
> -        error_append_hint(errp,
> +        error_append_hint(&local_err,
>                            "This KVM seems to be too old to support VSMT.\n");
>      }
> +    error_propagate(errp, local_err);
>  }
>  
>  
> 

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

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

Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()

[Kevin Wolf]

Am 17.09.2019 um 21:10 hat John Snow geschrieben:
> 
> 
> On 9/17/19 10:46 AM, Kevin Wolf wrote:
> > Am 17.09.2019 um 16:39 hat Eric Blake geschrieben:
> >> On 9/17/19 5:20 AM, Greg Kurz wrote:
> >>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> >>>
> >>> Signed-off-by: Greg Kurz <groug@kaod.org>
> >>> ---
> >>>  block/backup.c       |    7 +++++--
> >>>  block/dirty-bitmap.c |    7 +++++--
> >>>  block/file-posix.c   |   20 +++++++++++++-------
> >>>  block/gluster.c      |   23 +++++++++++++++--------
> >>>  block/qcow.c         |   10 ++++++----
> >>>  block/qcow2.c        |    7 +++++--
> >>>  block/vhdx-log.c     |    7 +++++--
> >>>  block/vpc.c          |    7 +++++--
> >>>  8 files changed, 59 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/block/backup.c b/block/backup.c
> >>> index 763f0d7ff6db..d8c422a0e3bc 100644
> >>> --- a/block/backup.c
> >>> +++ b/block/backup.c
> >>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> >>>                      BACKUP_CLUSTER_SIZE_DEFAULT);
> >>>          return BACKUP_CLUSTER_SIZE_DEFAULT;
> >>>      } else if (ret < 0 && !target->backing) {
> >>> -        error_setg_errno(errp, -ret,
> >>> +        Error *local_err = NULL;
> >>
> >> Can we go with the shorter name 'err' instead of 'local_err'?  I know,
> >> we aren't consistent (both styles appear throughout the tree), but the
> >> shorter style is more appealing to me.
> > 
> > I like local_err better because it's easier to distinguish from errp.
> > The compiler might catch it if you use the wrong one because one is
> > Error* and the other is Error**, but as a reviewer, I can still get
> > confused.
> 
> Doesn't that sound like a striking reason for condemnation of this
> entire model?

Might be, but do you have a better idea?

Kevin

Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()

[Greg Kurz]

On Tue, 17 Sep 2019 17:40:11 +0000
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 17.09.2019 18:37, Greg Kurz wrote:
> > On Tue, 17 Sep 2019 13:25:03 +0000
> > Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> > 
> >> 17.09.2019 13:20, Greg Kurz wrote:
> >>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> >>>
> >>> Signed-off-by: Greg Kurz <groug@kaod.org>
> >>> ---
> >>>    block/backup.c       |    7 +++++--
> >>>    block/dirty-bitmap.c |    7 +++++--
> >>>    block/file-posix.c   |   20 +++++++++++++-------
> >>>    block/gluster.c      |   23 +++++++++++++++--------
> >>>    block/qcow.c         |   10 ++++++----
> >>>    block/qcow2.c        |    7 +++++--
> >>>    block/vhdx-log.c     |    7 +++++--
> >>>    block/vpc.c          |    7 +++++--
> >>>    8 files changed, 59 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/block/backup.c b/block/backup.c
> >>> index 763f0d7ff6db..d8c422a0e3bc 100644
> >>> --- a/block/backup.c
> >>> +++ b/block/backup.c
> >>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
> >>>                        BACKUP_CLUSTER_SIZE_DEFAULT);
> >>>            return BACKUP_CLUSTER_SIZE_DEFAULT;
> >>>        } else if (ret < 0 && !target->backing) {
> >>> -        error_setg_errno(errp, -ret,
> >>> +        Error *local_err = NULL;
> >>> +
> >>> +        error_setg_errno(&local_err, -ret,
> >>>                "Couldn't determine the cluster size of the target image, "
> >>>                "which has no backing file");
> >>> -        error_append_hint(errp,
> >>> +        error_append_hint(&local_err,
> >>>                "Aborting, since this may create an unusable destination image\n");
> >>> +        error_propagate(errp, local_err);
> >>>            return ret;
> >>>        } else if (ret < 0 && target->backing) {
> >>>            /* Not fatal; just trudge on ahead. */
> >>
> >>
> >> Pain.. Do we need these hints, if they are so painful?
> >>
> > 
> > I agree that the one above doesn't qualify as a useful hint.
> > It just tells that QEMU is giving up and gives no indication
> > to the user on how to avoid the issue. It should probably be
> > dropped.
> > 
> >> At least for cases like this, we can create helper function
> >>
> >> error_setg_errno_hint(..., error, hint)
> > 
> > Not very useful if hint has to be forged separately with
> > g_sprintf(), and we can't have such a thing as:
> > 
> > error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...)
> > 
> >>
> >> But what could be done when we call function, which may or may not set errp?
> >>
> >> ret = f(errp);
> >> if (ret) {
> >>      error_append_hint(errp, hint);
> >> }
> >>
> > 
> > Same problem. If errp is &error_fatal and f() does errno_setg(errp), it
> > ends up calling exit().
> > 
> >> Hmmm..
> >>
> >> Can it look like
> >>
> >> ret = f(..., hint_helper(errp, hint))
> >>
> >> ?
> >>
> > 
> > Nope, hint_helper() will get called before f() and things are worse.
> > If errp is NULL then error_append_hint() does nothing and if it is
> > &error_fatal then it aborts.
> > 
> >> I can't imagine how to do it, as someone should remove hint from error_abort object on
> >> success path..
> >>
> >> But seems, the following is possible, which seems better for me than local-error approach:
> >>
> >> error_push_hint(errp, hint);
> >> ret = f(.., errp);
> >> error_pop_hint(errp);
> >>
> > 
> > Matter of taste... also, it looks awkward to come up with a hint
> > before knowing what happened. I mean the appropriate hint could
> > depend on the value returned by f() and/or errno for example.
> > 
> >> ===
> >>
> >> Continue thinking on this:
> >>
> >> It may look like just
> >> ret = f(..., set_hint(errp, hint));
> >>
> >> or (just to split long line):
> >> set_hint(errp, hint);
> >> ret = f(..., errp);
> >>
> >> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
> >> which may be just one call at function start of macro, which will call error_push_hint(errp) and
> >> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
> >> exit..
> >>
> >> Or, may be, more direct way to set cleanup for function exists?
> >>
> >> ===
> >>
> >> Also, we can implement some code generation, to generate for functions with errp argument
> >> wrappers with additional hint parameter, and just use these wrappers..
> >>
> >> ===
> >>
> >> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
> >> real issue and much effort has already been spent. May be one day I'll try to rewrite it...
> >>
> > 
> > For the reason exposed above, I don't think it makes sense to
> > build the hint before calling a function that calls error_setg().
> > I'm afraid we're stuck with local_err... it is then up to the
> > people to make it as less painful as possible.
> > 
> 
> Hmm. so, seems that in general we need local_err..
> 
> Then may be, may can make automated propagation?
> 
> It will look like
> 
> g_auto(ErrorPropagation) _error_prop = (ErrorPropagation){
>    .errp = errp,
>    .local_err = NULL,
> }
> 
> errp = &_error_prop.local_err;
> 
> and this thing may be fully covered into macro,
> to look like this at function start (to be honest it should exactly after all
> local variable definitions):
> 
> MAKE_ERRP_SAFE(_error_prop, errp);
> 
> 

Maybe you can send an RFC patch that converts a handful of
local_err users to g_auto() ?

Re: [Qemu-devel] [PATCH 02/17] block: Pass local error object pointer to error_append_hint()

[Vladimir Sementsov-Ogievskiy]

18.09.2019 10:58, Greg Kurz wrote:
> On Tue, 17 Sep 2019 17:40:11 +0000
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> 17.09.2019 18:37, Greg Kurz wrote:
>>> On Tue, 17 Sep 2019 13:25:03 +0000
>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>>>
>>>> 17.09.2019 13:20, Greg Kurz wrote:
>>>>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>>>>>
>>>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>>>> ---
>>>>>     block/backup.c       |    7 +++++--
>>>>>     block/dirty-bitmap.c |    7 +++++--
>>>>>     block/file-posix.c   |   20 +++++++++++++-------
>>>>>     block/gluster.c      |   23 +++++++++++++++--------
>>>>>     block/qcow.c         |   10 ++++++----
>>>>>     block/qcow2.c        |    7 +++++--
>>>>>     block/vhdx-log.c     |    7 +++++--
>>>>>     block/vpc.c          |    7 +++++--
>>>>>     8 files changed, 59 insertions(+), 29 deletions(-)
>>>>>
>>>>> diff --git a/block/backup.c b/block/backup.c
>>>>> index 763f0d7ff6db..d8c422a0e3bc 100644
>>>>> --- a/block/backup.c
>>>>> +++ b/block/backup.c
>>>>> @@ -602,11 +602,14 @@ static int64_t backup_calculate_cluster_size(BlockDriverState *target,
>>>>>                         BACKUP_CLUSTER_SIZE_DEFAULT);
>>>>>             return BACKUP_CLUSTER_SIZE_DEFAULT;
>>>>>         } else if (ret < 0 && !target->backing) {
>>>>> -        error_setg_errno(errp, -ret,
>>>>> +        Error *local_err = NULL;
>>>>> +
>>>>> +        error_setg_errno(&local_err, -ret,
>>>>>                 "Couldn't determine the cluster size of the target image, "
>>>>>                 "which has no backing file");
>>>>> -        error_append_hint(errp,
>>>>> +        error_append_hint(&local_err,
>>>>>                 "Aborting, since this may create an unusable destination image\n");
>>>>> +        error_propagate(errp, local_err);
>>>>>             return ret;
>>>>>         } else if (ret < 0 && target->backing) {
>>>>>             /* Not fatal; just trudge on ahead. */
>>>>
>>>>
>>>> Pain.. Do we need these hints, if they are so painful?
>>>>
>>>
>>> I agree that the one above doesn't qualify as a useful hint.
>>> It just tells that QEMU is giving up and gives no indication
>>> to the user on how to avoid the issue. It should probably be
>>> dropped.
>>>
>>>> At least for cases like this, we can create helper function
>>>>
>>>> error_setg_errno_hint(..., error, hint)
>>>
>>> Not very useful if hint has to be forged separately with
>>> g_sprintf(), and we can't have such a thing as:
>>>
>>> error_setg_errno_hint(errp, err_fmt, ..., hint_fmt, ...)
>>>
>>>>
>>>> But what could be done when we call function, which may or may not set errp?
>>>>
>>>> ret = f(errp);
>>>> if (ret) {
>>>>       error_append_hint(errp, hint);
>>>> }
>>>>
>>>
>>> Same problem. If errp is &error_fatal and f() does errno_setg(errp), it
>>> ends up calling exit().
>>>
>>>> Hmmm..
>>>>
>>>> Can it look like
>>>>
>>>> ret = f(..., hint_helper(errp, hint))
>>>>
>>>> ?
>>>>
>>>
>>> Nope, hint_helper() will get called before f() and things are worse.
>>> If errp is NULL then error_append_hint() does nothing and if it is
>>> &error_fatal then it aborts.
>>>
>>>> I can't imagine how to do it, as someone should remove hint from error_abort object on
>>>> success path..
>>>>
>>>> But seems, the following is possible, which seems better for me than local-error approach:
>>>>
>>>> error_push_hint(errp, hint);
>>>> ret = f(.., errp);
>>>> error_pop_hint(errp);
>>>>
>>>
>>> Matter of taste... also, it looks awkward to come up with a hint
>>> before knowing what happened. I mean the appropriate hint could
>>> depend on the value returned by f() and/or errno for example.
>>>
>>>> ===
>>>>
>>>> Continue thinking on this:
>>>>
>>>> It may look like just
>>>> ret = f(..., set_hint(errp, hint));
>>>>
>>>> or (just to split long line):
>>>> set_hint(errp, hint);
>>>> ret = f(..., errp);
>>>>
>>>> if in each function with errp does error_push_hint(errp) on start and error_pop_hint(errp) on exit,
>>>> which may be just one call at function start of macro, which will call error_push_hint(errp) and
>>>> define local variable by g_auto, with cleanup which will call error_pop_hint(errp) on function
>>>> exit..
>>>>
>>>> Or, may be, more direct way to set cleanup for function exists?
>>>>
>>>> ===
>>>>
>>>> Also, we can implement some code generation, to generate for functions with errp argument
>>>> wrappers with additional hint parameter, and just use these wrappers..
>>>>
>>>> ===
>>>>
>>>> If nobody likes any of my suggestions, then ignore them. I understand that this series fixes
>>>> real issue and much effort has already been spent. May be one day I'll try to rewrite it...
>>>>
>>>
>>> For the reason exposed above, I don't think it makes sense to
>>> build the hint before calling a function that calls error_setg().
>>> I'm afraid we're stuck with local_err... it is then up to the
>>> people to make it as less painful as possible.
>>>
>>
>> Hmm. so, seems that in general we need local_err..
>>
>> Then may be, may can make automated propagation?
>>
>> It will look like
>>
>> g_auto(ErrorPropagation) _error_prop = (ErrorPropagation){
>>     .errp = errp,
>>     .local_err = NULL,
>> }
>>
>> errp = &_error_prop.local_err;
>>
>> and this thing may be fully covered into macro,
>> to look like this at function start (to be honest it should exactly after all
>> local variable definitions):
>>
>> MAKE_ERRP_SAFE(_error_prop, errp);
>>
>>
> 
> Maybe you can send an RFC patch that converts a handful of
> local_err users to g_auto() ?
> 

Yes, I'll try

-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()

[Cornelia Huck]

On Tue, 17 Sep 2019 18:36:20 +0200
Greg Kurz <groug@kaod.org> wrote:

> On Tue, 17 Sep 2019 13:24:12 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Tue, 17 Sep 2019 12:21:34 +0200
> > Greg Kurz <groug@kaod.org> wrote:
> >   
> > > Ensure that hints are added even if errp is &error_fatal or &error_abort.
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  hw/s390x/s390-ccw.c |    6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)  
> > 
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > 
> > Can also take this via the s390 tree, let me know what would work best.  
> 
> I guess it would be easier to merge if each individual patch goes
> through the corresponding sub-maintainer tree. But Eric mentioned
> in another mail that the whole whole series could maybe go through
> Markus' error tree... so, I don't know which is best. :)

Ok, it's probably best to take this through the s390 tree, as I plan to
send a pull request tomorrow :)

Thanks, applied.

Re: [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()

[Eric Blake]

On 9/18/19 5:26 AM, Cornelia Huck wrote:
> On Tue, 17 Sep 2019 18:36:20 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
>> On Tue, 17 Sep 2019 13:24:12 +0200
>> Cornelia Huck <cohuck@redhat.com> wrote:
>>
>>> On Tue, 17 Sep 2019 12:21:34 +0200
>>> Greg Kurz <groug@kaod.org> wrote:
>>>   
>>>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
>>>>
>>>> Signed-off-by: Greg Kurz <groug@kaod.org>
>>>> ---
>>>>  hw/s390x/s390-ccw.c |    6 ++++--
>>>>  1 file changed, 4 insertions(+), 2 deletions(-)  
>>>
>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>
>>> Can also take this via the s390 tree, let me know what would work best.  
>>
>> I guess it would be easier to merge if each individual patch goes
>> through the corresponding sub-maintainer tree. But Eric mentioned
>> in another mail that the whole whole series could maybe go through
>> Markus' error tree... so, I don't know which is best. :)
> 
> Ok, it's probably best to take this through the s390 tree, as I plan to
> send a pull request tomorrow :)

If we go with Vladimir's idea of auto-propagation, this change just gets
rewritten again as part of our simplifications to drop all use of
'local_err' in favor of instead using the macro that makes errp always
safe to use locally.

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

Re: [Qemu-devel] [PATCH 10/17] s390x/css: Pass local error object pointer to error_append_hint()

[Cornelia Huck]

On Wed, 18 Sep 2019 12:46:42 -0500
Eric Blake <eblake@redhat.com> wrote:

> On 9/18/19 5:26 AM, Cornelia Huck wrote:
> > On Tue, 17 Sep 2019 18:36:20 +0200
> > Greg Kurz <groug@kaod.org> wrote:
> >   
> >> On Tue, 17 Sep 2019 13:24:12 +0200
> >> Cornelia Huck <cohuck@redhat.com> wrote:
> >>  
> >>> On Tue, 17 Sep 2019 12:21:34 +0200
> >>> Greg Kurz <groug@kaod.org> wrote:
> >>>     
> >>>> Ensure that hints are added even if errp is &error_fatal or &error_abort.
> >>>>
> >>>> Signed-off-by: Greg Kurz <groug@kaod.org>
> >>>> ---
> >>>>  hw/s390x/s390-ccw.c |    6 ++++--
> >>>>  1 file changed, 4 insertions(+), 2 deletions(-)    
> >>>
> >>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> >>>
> >>> Can also take this via the s390 tree, let me know what would work best.    
> >>
> >> I guess it would be easier to merge if each individual patch goes
> >> through the corresponding sub-maintainer tree. But Eric mentioned
> >> in another mail that the whole whole series could maybe go through
> >> Markus' error tree... so, I don't know which is best. :)  
> > 
> > Ok, it's probably best to take this through the s390 tree, as I plan to
> > send a pull request tomorrow :)  
> 
> If we go with Vladimir's idea of auto-propagation, this change just gets
> rewritten again as part of our simplifications to drop all use of
> 'local_err' in favor of instead using the macro that makes errp always
> safe to use locally.
> 

Fair enough. That auto-propagation approach really looks nice, so I'll
go ahead and unqueue this patch again.