[PATCH v7 00/21] error: prepare for auto propagated local_err

[PATCH v7 00/21] error: prepare for auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

Hi all!

This is the first part of the bit series, which contains mostly simple
cleanups.

v6 was sent in separate (I'm sorry for inconvenience)

v7: by Markus review (and with his prepared fixups, thanks a lot!):
  - don't rename Error** paramters
  - switch to Error *const * where appropriate
  last patch is new and replaces
   "nbd: well form nbd_iter_channel_error errp handler"

Vladimir Sementsov-Ogievskiy (21):
  hw/core/loader-fit: fix freeing errp in fit_load_fdt
  net/net: Clean up variable shadowing in net_client_init()
  error: rename errp to errp_in where it is IN-argument
  hmp: drop Error pointer indirection in hmp_handle_error
  vnc: drop Error pointer indirection in vnc_client_io_error
  qdev-monitor: well form error hint helpers
  ppc: well form kvmppc_hint_smt_possible error hint helper
  9pfs: well form error hint helpers
  hw/core/qdev: cleanup Error ** variables
  block/snapshot: rename Error ** parameter to more common errp
  hw/i386/amd_iommu: rename Error ** parameter to more common errp
  qga: rename Error ** parameter to more common errp
  monitor/qmp-cmds: rename Error ** parameter to more common errp
  hw/s390x: rename Error ** parameter to more common errp
  hw/sd: drop extra whitespace in sdhci_sysbus_realize() header
  hw/tpm: rename Error ** parameter to more common errp
  hw/usb: rename Error ** parameter to more common errp
  include/qom/object.h: rename Error ** parameter to more common errp
  backends/cryptodev: drop local_err from cryptodev_backend_complete()
  hw/vfio/ap: drop local_err from vfio_ap_realize
  nbd: assert that Error** is not NULL in nbd_iter_channel_error

Cc: "Gonglei (Arei)" <arei.gonglei@huawei.com>
Cc: Eric Blake <eblake@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
Cc: "Marc-André Lureau" <marcandre.lureau@redhat.com>
Cc: Greg Kurz <groug@kaod.org>
Cc: Paul Burton <pburton@wavecomp.com>
Cc: Aleksandar Rikalo <aleksandar.rikalo@rt-rk.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Cornelia Huck <cohuck@redhat.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Halil Pasic <pasic@linux.ibm.com>
Cc: Christian Borntraeger <borntraeger@de.ibm.com>
Cc: "Philippe Mathieu-Daudé" <philmd@redhat.com>
Cc: Stefan Berger <stefanb@linux.ibm.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Tony Krowiak <akrowiak@linux.ibm.com>
Cc: Pierre Morel <pmorel@linux.ibm.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>
Cc: Michael Roth <mdroth@linux.vnet.ibm.com>
Cc: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Cc: qemu-devel@nongnu.org
Cc: qemu-block@nongnu.org
Cc: qemu-ppc@nongnu.org
Cc: qemu-s390x@nongnu.org

 include/block/snapshot.h   |   2 +-
 include/monitor/hmp.h      |   2 +-
 include/qapi/error.h       |   6 +-
 include/qom/object.h       |   4 +-
 target/ppc/kvm_ppc.h       |   4 +-
 ui/vnc.h                   |   2 +-
 backends/cryptodev.c       |  11 +--
 block/nbd.c                |   1 +
 block/snapshot.c           |   4 +-
 dump/dump-hmp-cmds.c       |   4 +-
 hw/9pfs/9p-local.c         |   2 +-
 hw/9pfs/9p-proxy.c         |   2 +-
 hw/core/loader-fit.c       |   5 +-
 hw/core/machine-hmp-cmds.c |   6 +-
 hw/core/qdev.c             |  28 ++++---
 hw/i386/amd_iommu.c        |  14 ++--
 hw/ppc/spapr.c             |   2 +-
 hw/s390x/event-facility.c  |   2 +-
 hw/s390x/s390-stattrib.c   |   3 +-
 hw/sd/sdhci.c              |   2 +-
 hw/tpm/tpm_emulator.c      |   8 +-
 hw/usb/dev-network.c       |   2 +-
 hw/vfio/ap.c               |   9 +--
 monitor/hmp-cmds.c         | 155 ++++++++++++++++++-------------------
 monitor/qmp-cmds.c         |   2 +-
 net/net.c                  |  17 ++--
 qdev-monitor.c             |  16 ++--
 qga/commands-posix.c       |   2 +-
 qga/commands-win32.c       |   2 +-
 qga/commands.c             |  12 +--
 qom/qom-hmp-cmds.c         |   4 +-
 target/ppc/kvm.c           |   2 +-
 ui/vnc.c                   |  20 ++---
 util/error.c               |   6 +-
 34 files changed, 173 insertions(+), 190 deletions(-)

-- 
2.21.0

[PATCH v7 01/21] hw/core/loader-fit: fix freeing errp in fit_load_fdt

[Vladimir Sementsov-Ogievskiy]

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

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

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

[PATCH v7 02/21] net/net: Clean up variable shadowing in net_client_init()

[Vladimir Sementsov-Ogievskiy]

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

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

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

[PATCH v7 03/21] error: rename errp to errp_in where it is IN-argument

[Vladimir Sementsov-Ogievskiy]

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

But very few functions instead get Error **errp as IN-argument:
it's assumed to be set (or, maybe, NULL), and callee should clean it,
or add some information.

In such cases, rename errp to errp_in.

This patch updates only error API functions. There still a few
functions with errp-in semantics, they will be updated in further
commits.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 include/qapi/error.h | 6 +++---
 util/error.c         | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/qapi/error.h b/include/qapi/error.h
index 3f95141a01..ad5b6e896d 100644
--- a/include/qapi/error.h
+++ b/include/qapi/error.h
@@ -233,13 +233,13 @@ void error_propagate_prepend(Error **dst_errp, Error *local_err,
  * Prepend some text to @errp's human-readable error message.
  * The text is made by formatting @fmt, @ap like vprintf().
  */
-void error_vprepend(Error **errp, const char *fmt, va_list ap);
+void error_vprepend(Error *const *errp, const char *fmt, va_list ap);
 
 /*
  * Prepend some text to @errp's human-readable error message.
  * The text is made by formatting @fmt, ... like printf().
  */
-void error_prepend(Error **errp, const char *fmt, ...)
+void error_prepend(Error *const *errp, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
 
 /*
@@ -256,7 +256,7 @@ void error_prepend(Error **errp, const char *fmt, ...)
  * May be called multiple times.  The resulting hint should end with a
  * newline.
  */
-void error_append_hint(Error **errp, const char *fmt, ...)
+void error_append_hint(Error *const *errp, const char *fmt, ...)
     GCC_FMT_ATTR(2, 3);
 
 /*
diff --git a/util/error.c b/util/error.c
index d4532ce318..b6c89d1412 100644
--- a/util/error.c
+++ b/util/error.c
@@ -121,7 +121,7 @@ void error_setg_file_open_internal(Error **errp,
                               "Could not open '%s'", filename);
 }
 
-void error_vprepend(Error **errp, const char *fmt, va_list ap)
+void error_vprepend(Error *const *errp, const char *fmt, va_list ap)
 {
     GString *newmsg;
 
@@ -136,7 +136,7 @@ void error_vprepend(Error **errp, const char *fmt, va_list ap)
     (*errp)->msg = g_string_free(newmsg, 0);
 }
 
-void error_prepend(Error **errp, const char *fmt, ...)
+void error_prepend(Error *const *errp, const char *fmt, ...)
 {
     va_list ap;
 
@@ -145,7 +145,7 @@ void error_prepend(Error **errp, const char *fmt, ...)
     va_end(ap);
 }
 
-void error_append_hint(Error **errp, const char *fmt, ...)
+void error_append_hint(Error *const *errp, const char *fmt, ...)
 {
     va_list ap;
     int saved_errno = errno;
-- 
2.21.0

Re: [PATCH v7 03/21] error: rename errp to errp_in where it is IN-argument

[Greg Kurz]

On Thu,  5 Dec 2019 18:20:01 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> Error **errp is almost always OUT-argument: it's assumed to be NULL, or
> pointer to NULL-initialized pointer, or pointer to error_abort or
> error_fatal, for callee to report error.
> 
> But very few functions instead get Error **errp as IN-argument:
> it's assumed to be set (or, maybe, NULL), and callee should clean it,
> or add some information.
> 
> In such cases, rename errp to errp_in.
> 

This is no longer what the patch does. The subject needs to be amended too.

> This patch updates only error API functions. There still a few
> functions with errp-in semantics, they will be updated in further
> commits.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/qapi/error.h | 6 +++---
>  util/error.c         | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/qapi/error.h b/include/qapi/error.h
> index 3f95141a01..ad5b6e896d 100644
> --- a/include/qapi/error.h
> +++ b/include/qapi/error.h
> @@ -233,13 +233,13 @@ void error_propagate_prepend(Error **dst_errp, Error *local_err,
>   * Prepend some text to @errp's human-readable error message.
>   * The text is made by formatting @fmt, @ap like vprintf().
>   */
> -void error_vprepend(Error **errp, const char *fmt, va_list ap);
> +void error_vprepend(Error *const *errp, const char *fmt, va_list ap);
>  
>  /*
>   * Prepend some text to @errp's human-readable error message.
>   * The text is made by formatting @fmt, ... like printf().
>   */
> -void error_prepend(Error **errp, const char *fmt, ...)
> +void error_prepend(Error *const *errp, const char *fmt, ...)
>      GCC_FMT_ATTR(2, 3);
>  
>  /*
> @@ -256,7 +256,7 @@ void error_prepend(Error **errp, const char *fmt, ...)
>   * May be called multiple times.  The resulting hint should end with a
>   * newline.
>   */
> -void error_append_hint(Error **errp, const char *fmt, ...)
> +void error_append_hint(Error *const *errp, const char *fmt, ...)
>      GCC_FMT_ATTR(2, 3);
>  
>  /*
> diff --git a/util/error.c b/util/error.c
> index d4532ce318..b6c89d1412 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -121,7 +121,7 @@ void error_setg_file_open_internal(Error **errp,
>                                "Could not open '%s'", filename);
>  }
>  
> -void error_vprepend(Error **errp, const char *fmt, va_list ap)
> +void error_vprepend(Error *const *errp, const char *fmt, va_list ap)
>  {
>      GString *newmsg;
>  
> @@ -136,7 +136,7 @@ void error_vprepend(Error **errp, const char *fmt, va_list ap)
>      (*errp)->msg = g_string_free(newmsg, 0);
>  }
>  
> -void error_prepend(Error **errp, const char *fmt, ...)
> +void error_prepend(Error *const *errp, const char *fmt, ...)
>  {
>      va_list ap;
>  
> @@ -145,7 +145,7 @@ void error_prepend(Error **errp, const char *fmt, ...)
>      va_end(ap);
>  }
>  
> -void error_append_hint(Error **errp, const char *fmt, ...)
> +void error_append_hint(Error *const *errp, const char *fmt, ...)
>  {
>      va_list ap;
>      int saved_errno = errno;

[PATCH v7 04/21] hmp: drop Error pointer indirection in hmp_handle_error

[Vladimir Sementsov-Ogievskiy]

We don't need Error **, as all callers pass local Error object, which
isn't used after the call. Use Error * instead.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Acked-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 include/monitor/hmp.h      |   2 +-
 dump/dump-hmp-cmds.c       |   4 +-
 hw/core/machine-hmp-cmds.c |   6 +-
 monitor/hmp-cmds.c         | 155 ++++++++++++++++++-------------------
 qdev-monitor.c             |   4 +-
 qom/qom-hmp-cmds.c         |   4 +-
 6 files changed, 87 insertions(+), 88 deletions(-)

diff --git a/include/monitor/hmp.h b/include/monitor/hmp.h
index a0e9511440..3d329853b2 100644
--- a/include/monitor/hmp.h
+++ b/include/monitor/hmp.h
@@ -16,7 +16,7 @@
 
 #include "qemu/readline.h"
 
-void hmp_handle_error(Monitor *mon, Error **errp);
+void hmp_handle_error(Monitor *mon, Error *err);
 
 void hmp_info_name(Monitor *mon, const QDict *qdict);
 void hmp_info_version(Monitor *mon, const QDict *qdict);
diff --git a/dump/dump-hmp-cmds.c b/dump/dump-hmp-cmds.c
index 3dbf44372c..e5053b04cd 100644
--- a/dump/dump-hmp-cmds.c
+++ b/dump/dump-hmp-cmds.c
@@ -32,7 +32,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
 
     if (zlib + lzo + snappy + win_dmp > 1) {
         error_setg(&err, "only one of '-z|-l|-s|-w' can be set");
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
         return;
     }
 
@@ -66,7 +66,7 @@ void hmp_dump_guest_memory(Monitor *mon, const QDict *qdict)
 
     qmp_dump_guest_memory(paging, prot, true, detach, has_begin, begin,
                           has_length, length, true, dump_format, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
     g_free(prot);
 }
 
diff --git a/hw/core/machine-hmp-cmds.c b/hw/core/machine-hmp-cmds.c
index cd970cc4c5..b76f7223af 100644
--- a/hw/core/machine-hmp-cmds.c
+++ b/hw/core/machine-hmp-cmds.c
@@ -55,7 +55,7 @@ void hmp_cpu_add(Monitor *mon, const QDict *qdict)
 
     cpuid = qdict_get_int(qdict, "id");
     qmp_cpu_add(cpuid, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
@@ -66,7 +66,7 @@ void hmp_hotpluggable_cpus(Monitor *mon, const QDict *qdict)
     CpuInstanceProperties *c;
 
     if (err != NULL) {
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
         return;
     }
 
@@ -135,7 +135,7 @@ void hmp_info_memdev(Monitor *mon, const QDict *qdict)
     monitor_printf(mon, "\n");
 
     qapi_free_MemdevList(memdev_list);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_info_numa(Monitor *mon, const QDict *qdict)
diff --git a/monitor/hmp-cmds.c b/monitor/hmp-cmds.c
index b2551c16d1..c5dea307b6 100644
--- a/monitor/hmp-cmds.c
+++ b/monitor/hmp-cmds.c
@@ -60,11 +60,10 @@
 #include <spice/enums.h>
 #endif
 
-void hmp_handle_error(Monitor *mon, Error **errp)
+void hmp_handle_error(Monitor *mon, Error *err)
 {
-    assert(errp);
-    if (*errp) {
-        error_reportf_err(*errp, "Error: ");
+    if (err) {
+        error_reportf_err(err, "Error: ");
     }
 }
 
@@ -734,7 +733,7 @@ void hmp_info_vnc(Monitor *mon, const QDict *qdict)
 
     info2l = qmp_query_vnc_servers(&err);
     if (err) {
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
         return;
     }
     if (!info2l) {
@@ -850,7 +849,7 @@ void hmp_info_balloon(Monitor *mon, const QDict *qdict)
 
     info = qmp_query_balloon(&err);
     if (err) {
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
         return;
     }
 
@@ -1172,7 +1171,7 @@ void hmp_sync_profile(Monitor *mon, const QDict *qdict)
         Error *err = NULL;
 
         error_setg(&err, QERR_INVALID_PARAMETER, op);
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
     }
 }
 
@@ -1191,7 +1190,7 @@ void hmp_exit_preconfig(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_x_exit_preconfig(&err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_cpu(Monitor *mon, const QDict *qdict)
@@ -1220,7 +1219,7 @@ void hmp_memsave(Monitor *mon, const QDict *qdict)
     }
 
     qmp_memsave(addr, size, filename, true, cpu_index, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_pmemsave(Monitor *mon, const QDict *qdict)
@@ -1231,7 +1230,7 @@ void hmp_pmemsave(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_pmemsave(addr, size, filename, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_ringbuf_write(Monitor *mon, const QDict *qdict)
@@ -1242,7 +1241,7 @@ void hmp_ringbuf_write(Monitor *mon, const QDict *qdict)
 
     qmp_ringbuf_write(chardev, data, false, 0, &err);
 
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_ringbuf_read(Monitor *mon, const QDict *qdict)
@@ -1255,7 +1254,7 @@ void hmp_ringbuf_read(Monitor *mon, const QDict *qdict)
 
     data = qmp_ringbuf_read(chardev, size, false, 0, &err);
     if (err) {
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
         return;
     }
 
@@ -1280,7 +1279,7 @@ void hmp_cont(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_cont(&err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_system_wakeup(Monitor *mon, const QDict *qdict)
@@ -1288,7 +1287,7 @@ void hmp_system_wakeup(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_system_wakeup(&err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_nmi(Monitor *mon, const QDict *qdict)
@@ -1296,7 +1295,7 @@ void hmp_nmi(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_inject_nmi(&err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_set_link(Monitor *mon, const QDict *qdict)
@@ -1306,7 +1305,7 @@ void hmp_set_link(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_set_link(name, up, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_block_passwd(Monitor *mon, const QDict *qdict)
@@ -1316,7 +1315,7 @@ void hmp_block_passwd(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_block_passwd(true, device, false, NULL, password, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_balloon(Monitor *mon, const QDict *qdict)
@@ -1325,7 +1324,7 @@ void hmp_balloon(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_balloon(value, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_block_resize(Monitor *mon, const QDict *qdict)
@@ -1335,7 +1334,7 @@ void hmp_block_resize(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_block_resize(true, device, false, NULL, size, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
@@ -1358,11 +1357,11 @@ void hmp_drive_mirror(Monitor *mon, const QDict *qdict)
 
     if (!filename) {
         error_setg(&err, QERR_MISSING_PARAMETER, "target");
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
         return;
     }
     qmp_drive_mirror(&mirror, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_drive_backup(Monitor *mon, const QDict *qdict)
@@ -1388,12 +1387,12 @@ void hmp_drive_backup(Monitor *mon, const QDict *qdict)
 
     if (!filename) {
         error_setg(&err, QERR_MISSING_PARAMETER, "target");
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
         return;
     }
 
     qmp_drive_backup(&backup, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
@@ -1409,7 +1408,7 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
         /* In the future, if 'snapshot-file' is not specified, the snapshot
            will be taken internally. Today it's actually required. */
         error_setg(&err, QERR_MISSING_PARAMETER, "snapshot-file");
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
         return;
     }
 
@@ -1418,7 +1417,7 @@ void hmp_snapshot_blkdev(Monitor *mon, const QDict *qdict)
                                filename, false, NULL,
                                !!format, format,
                                true, mode, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict)
@@ -1428,7 +1427,7 @@ void hmp_snapshot_blkdev_internal(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_blockdev_snapshot_internal_sync(device, name, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict)
@@ -1440,7 +1439,7 @@ void hmp_snapshot_delete_blkdev_internal(Monitor *mon, const QDict *qdict)
 
     qmp_blockdev_snapshot_delete_internal_sync(device, !!id, id,
                                                true, name, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_loadvm(Monitor *mon, const QDict *qdict)
@@ -1454,7 +1453,7 @@ void hmp_loadvm(Monitor *mon, const QDict *qdict)
     if (load_snapshot(name, &err) == 0 && saved_vm_running) {
         vm_start();
     }
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_savevm(Monitor *mon, const QDict *qdict)
@@ -1462,7 +1461,7 @@ void hmp_savevm(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     save_snapshot(qdict_get_try_str(qdict, "name"), &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_delvm(Monitor *mon, const QDict *qdict)
@@ -1476,7 +1475,7 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
                       "deleting snapshot on device '%s': ",
                       bdrv_get_device_name(bs));
     }
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
@@ -1652,7 +1651,7 @@ void hmp_migrate_continue(Monitor *mon, const QDict *qdict)
         qmp_migrate_continue(val, &err);
     }
 
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
@@ -1662,7 +1661,7 @@ void hmp_migrate_incoming(Monitor *mon, const QDict *qdict)
 
     qmp_migrate_incoming(uri, &err);
 
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_migrate_recover(Monitor *mon, const QDict *qdict)
@@ -1672,7 +1671,7 @@ void hmp_migrate_recover(Monitor *mon, const QDict *qdict)
 
     qmp_migrate_recover(uri, &err);
 
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_migrate_pause(Monitor *mon, const QDict *qdict)
@@ -1681,7 +1680,7 @@ void hmp_migrate_pause(Monitor *mon, const QDict *qdict)
 
     qmp_migrate_pause(&err);
 
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 /* Kept for backwards compatibility */
@@ -1697,7 +1696,7 @@ void hmp_migrate_set_cache_size(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_migrate_set_cache_size(value, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 /* Kept for backwards compatibility */
@@ -1728,7 +1727,7 @@ void hmp_migrate_set_capability(Monitor *mon, const QDict *qdict)
 
 end:
     qapi_free_MigrationCapabilityStatusList(caps);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
@@ -1869,7 +1868,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
  cleanup:
     qapi_free_MigrateSetParameters(p);
     visit_free(v);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_client_migrate_info(Monitor *mon, const QDict *qdict)
@@ -1886,14 +1885,14 @@ void hmp_client_migrate_info(Monitor *mon, const QDict *qdict)
     qmp_client_migrate_info(protocol, hostname,
                             has_port, port, has_tls_port, tls_port,
                             !!cert_subject, cert_subject, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_migrate_start_postcopy(Monitor *mon, const QDict *qdict)
 {
     Error *err = NULL;
     qmp_migrate_start_postcopy(&err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
@@ -1901,7 +1900,7 @@ void hmp_x_colo_lost_heartbeat(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_x_colo_lost_heartbeat(&err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_set_password(Monitor *mon, const QDict *qdict)
@@ -1912,7 +1911,7 @@ void hmp_set_password(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_set_password(protocol, password, !!connected, connected, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_expire_password(Monitor *mon, const QDict *qdict)
@@ -1922,7 +1921,7 @@ void hmp_expire_password(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_expire_password(protocol, whenstr, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_eject(Monitor *mon, const QDict *qdict)
@@ -1932,7 +1931,7 @@ void hmp_eject(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_eject(true, device, false, NULL, true, force, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 #ifdef CONFIG_VNC
@@ -1978,7 +1977,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
                                 read_only,
                                 BLOCKDEV_CHANGE_READ_ONLY_MODE_RETAIN, &err);
             if (err) {
-                hmp_handle_error(mon, &err);
+                hmp_handle_error(mon, err);
                 return;
             }
         }
@@ -1988,7 +1987,7 @@ void hmp_change(Monitor *mon, const QDict *qdict)
                                    &err);
     }
 
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
@@ -2016,7 +2015,7 @@ void hmp_block_set_io_throttle(Monitor *mon, const QDict *qdict)
     }
 
     qmp_block_set_io_throttle(&throttle, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_block_stream(Monitor *mon, const QDict *qdict)
@@ -2031,7 +2030,7 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
                      BLOCKDEV_ON_ERROR_REPORT, false, false, false, false,
                      &error);
 
-    hmp_handle_error(mon, &error);
+    hmp_handle_error(mon, error);
 }
 
 void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict)
@@ -2042,7 +2041,7 @@ void hmp_block_job_set_speed(Monitor *mon, const QDict *qdict)
 
     qmp_block_job_set_speed(device, value, &error);
 
-    hmp_handle_error(mon, &error);
+    hmp_handle_error(mon, error);
 }
 
 void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
@@ -2053,7 +2052,7 @@ void hmp_block_job_cancel(Monitor *mon, const QDict *qdict)
 
     qmp_block_job_cancel(device, true, force, &error);
 
-    hmp_handle_error(mon, &error);
+    hmp_handle_error(mon, error);
 }
 
 void hmp_block_job_pause(Monitor *mon, const QDict *qdict)
@@ -2063,7 +2062,7 @@ void hmp_block_job_pause(Monitor *mon, const QDict *qdict)
 
     qmp_block_job_pause(device, &error);
 
-    hmp_handle_error(mon, &error);
+    hmp_handle_error(mon, error);
 }
 
 void hmp_block_job_resume(Monitor *mon, const QDict *qdict)
@@ -2073,7 +2072,7 @@ void hmp_block_job_resume(Monitor *mon, const QDict *qdict)
 
     qmp_block_job_resume(device, &error);
 
-    hmp_handle_error(mon, &error);
+    hmp_handle_error(mon, error);
 }
 
 void hmp_block_job_complete(Monitor *mon, const QDict *qdict)
@@ -2083,7 +2082,7 @@ void hmp_block_job_complete(Monitor *mon, const QDict *qdict)
 
     qmp_block_job_complete(device, &error);
 
-    hmp_handle_error(mon, &error);
+    hmp_handle_error(mon, error);
 }
 
 typedef struct HMPMigrationStatus
@@ -2143,7 +2142,7 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
     qmp_migrate(uri, !!blk, blk, !!inc, inc,
                 false, false, true, resume, &err);
     if (err) {
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
         return;
     }
 
@@ -2181,7 +2180,7 @@ void hmp_netdev_add(Monitor *mon, const QDict *qdict)
     }
 
 out:
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_netdev_del(Monitor *mon, const QDict *qdict)
@@ -2190,7 +2189,7 @@ void hmp_netdev_del(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_netdev_del(id, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_object_add(Monitor *mon, const QDict *qdict)
@@ -2201,7 +2200,7 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
 
     opts = qemu_opts_from_qdict(qemu_find_opts("object"), qdict, &err);
     if (err) {
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
         return;
     }
 
@@ -2209,7 +2208,7 @@ void hmp_object_add(Monitor *mon, const QDict *qdict)
     qemu_opts_del(opts);
 
     if (err) {
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
     }
     if (obj) {
         object_unref(obj);
@@ -2222,7 +2221,7 @@ void hmp_getfd(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_getfd(fdname, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_closefd(Monitor *mon, const QDict *qdict)
@@ -2231,7 +2230,7 @@ void hmp_closefd(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_closefd(fdname, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_sendkey(Monitor *mon, const QDict *qdict)
@@ -2290,7 +2289,7 @@ void hmp_sendkey(Monitor *mon, const QDict *qdict)
     }
 
     qmp_send_key(head, has_hold_time, hold_time, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 
 out:
     qapi_free_KeyValueList(head);
@@ -2309,7 +2308,7 @@ void hmp_screendump(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_screendump(filename, id != NULL, id, id != NULL, head, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
@@ -2364,7 +2363,7 @@ void hmp_nbd_server_start(Monitor *mon, const QDict *qdict)
     qapi_free_BlockInfoList(block_list);
 
 exit:
-    hmp_handle_error(mon, &local_err);
+    hmp_handle_error(mon, local_err);
 }
 
 void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
@@ -2376,7 +2375,7 @@ void hmp_nbd_server_add(Monitor *mon, const QDict *qdict)
 
     qmp_nbd_server_add(device, !!name, name, true, writable,
                        false, NULL, &local_err);
-    hmp_handle_error(mon, &local_err);
+    hmp_handle_error(mon, local_err);
 }
 
 void hmp_nbd_server_remove(Monitor *mon, const QDict *qdict)
@@ -2387,7 +2386,7 @@ void hmp_nbd_server_remove(Monitor *mon, const QDict *qdict)
 
     /* Rely on NBD_SERVER_REMOVE_MODE_SAFE being the default */
     qmp_nbd_server_remove(name, force, NBD_SERVER_REMOVE_MODE_HARD, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
@@ -2395,7 +2394,7 @@ void hmp_nbd_server_stop(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_nbd_server_stop(&err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_chardev_add(Monitor *mon, const QDict *qdict)
@@ -2411,7 +2410,7 @@ void hmp_chardev_add(Monitor *mon, const QDict *qdict)
         qemu_chr_new_from_opts(opts, NULL, &err);
         qemu_opts_del(opts);
     }
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_chardev_change(Monitor *mon, const QDict *qdict)
@@ -2445,7 +2444,7 @@ end:
     qapi_free_ChardevReturn(ret);
     qapi_free_ChardevBackend(backend);
     qemu_opts_del(opts);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_chardev_remove(Monitor *mon, const QDict *qdict)
@@ -2453,7 +2452,7 @@ void hmp_chardev_remove(Monitor *mon, const QDict *qdict)
     Error *local_err = NULL;
 
     qmp_chardev_remove(qdict_get_str(qdict, "id"), &local_err);
-    hmp_handle_error(mon, &local_err);
+    hmp_handle_error(mon, local_err);
 }
 
 void hmp_chardev_send_break(Monitor *mon, const QDict *qdict)
@@ -2461,7 +2460,7 @@ void hmp_chardev_send_break(Monitor *mon, const QDict *qdict)
     Error *local_err = NULL;
 
     qmp_chardev_send_break(qdict_get_str(qdict, "id"), &local_err);
-    hmp_handle_error(mon, &local_err);
+    hmp_handle_error(mon, local_err);
 }
 
 void hmp_qemu_io(Monitor *mon, const QDict *qdict)
@@ -2517,7 +2516,7 @@ void hmp_qemu_io(Monitor *mon, const QDict *qdict)
 
 fail:
     blk_unref(local_blk);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_object_del(Monitor *mon, const QDict *qdict)
@@ -2526,7 +2525,7 @@ void hmp_object_del(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     user_creatable_del(id, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
@@ -2576,7 +2575,7 @@ void hmp_info_memory_devices(Monitor *mon, const QDict *qdict)
     }
 
     qapi_free_MemoryDeviceInfoList(info_list);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_info_iothreads(Monitor *mon, const QDict *qdict)
@@ -2605,7 +2604,7 @@ void hmp_rocker(Monitor *mon, const QDict *qdict)
 
     rocker = qmp_query_rocker(name, &err);
     if (err != NULL) {
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
         return;
     }
 
@@ -2624,7 +2623,7 @@ void hmp_rocker_ports(Monitor *mon, const QDict *qdict)
 
     list = qmp_query_rocker_ports(name, &err);
     if (err != NULL) {
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
         return;
     }
 
@@ -2653,7 +2652,7 @@ void hmp_rocker_of_dpa_flows(Monitor *mon, const QDict *qdict)
 
     list = qmp_query_rocker_of_dpa_flows(name, tbl_id != -1, tbl_id, &err);
     if (err != NULL) {
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
         return;
     }
 
@@ -2804,7 +2803,7 @@ void hmp_rocker_of_dpa_groups(Monitor *mon, const QDict *qdict)
 
     list = qmp_query_rocker_of_dpa_groups(name, type != 9, type, &err);
     if (err != NULL) {
-        hmp_handle_error(mon, &err);
+        hmp_handle_error(mon, err);
         return;
     }
 
@@ -2911,7 +2910,7 @@ void hmp_info_vm_generation_id(Monitor *mon, const QDict *qdict)
     if (info) {
         monitor_printf(mon, "%s\n", info->guid);
     }
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
     qapi_free_GuidInfo(info);
 }
 
@@ -2930,5 +2929,5 @@ void hmp_info_memory_size_summary(Monitor *mon, const QDict *qdict)
 
         qapi_free_MemoryInfo(info);
     }
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
diff --git a/qdev-monitor.c b/qdev-monitor.c
index e6b112eb0a..29ed73e56a 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -890,7 +890,7 @@ void hmp_device_add(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_device_add((QDict *)qdict, NULL, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_device_del(Monitor *mon, const QDict *qdict)
@@ -899,7 +899,7 @@ void hmp_device_del(Monitor *mon, const QDict *qdict)
     Error *err = NULL;
 
     qmp_device_del(id, &err);
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 BlockBackend *blk_by_qdev_id(const char *id, Error **errp)
diff --git a/qom/qom-hmp-cmds.c b/qom/qom-hmp-cmds.c
index a268e01eb4..cd08233a4c 100644
--- a/qom/qom-hmp-cmds.c
+++ b/qom/qom-hmp-cmds.c
@@ -37,7 +37,7 @@ void hmp_qom_list(Monitor *mon, const QDict *qdict)
         }
         qapi_free_ObjectPropertyInfoList(start);
     }
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 void hmp_qom_set(Monitor *mon, const QDict *qdict)
@@ -59,7 +59,7 @@ void hmp_qom_set(Monitor *mon, const QDict *qdict)
         }
         object_property_parse(obj, value, property, &err);
     }
-    hmp_handle_error(mon, &err);
+    hmp_handle_error(mon, err);
 }
 
 typedef struct QOMCompositionState {
-- 
2.21.0

[PATCH v7 05/21] vnc: drop Error pointer indirection in vnc_client_io_error

[Vladimir Sementsov-Ogievskiy]

We don't need Error **, as all callers pass local Error object, which
isn't used after the call, or NULL. Use Error * instead.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 ui/vnc.h |  2 +-
 ui/vnc.c | 20 +++++++-------------
 2 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/ui/vnc.h b/ui/vnc.h
index fea79c2fc9..4e2637ce6c 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -547,7 +547,7 @@ uint32_t read_u32(uint8_t *data, size_t offset);
 
 /* Protocol stage functions */
 void vnc_client_error(VncState *vs);
-size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp);
+size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error *err);
 
 void start_client_init(VncState *vs);
 void start_auth_vnc(VncState *vs);
diff --git a/ui/vnc.c b/ui/vnc.c
index 87b8045afe..4100d6e404 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1312,7 +1312,7 @@ void vnc_disconnect_finish(VncState *vs)
     g_free(vs);
 }
 
-size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
+size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error *err)
 {
     if (ret <= 0) {
         if (ret == 0) {
@@ -1320,15 +1320,11 @@ size_t vnc_client_io_error(VncState *vs, ssize_t ret, Error **errp)
             vnc_disconnect_start(vs);
         } else if (ret != QIO_CHANNEL_ERR_BLOCK) {
             trace_vnc_client_io_error(vs, vs->ioc,
-                                      errp ? error_get_pretty(*errp) :
-                                      "Unknown");
+                                      err ? error_get_pretty(err) : "Unknown");
             vnc_disconnect_start(vs);
         }
 
-        if (errp) {
-            error_free(*errp);
-            *errp = NULL;
-        }
+        error_free(err);
         return 0;
     }
     return ret;
@@ -1361,10 +1357,9 @@ size_t vnc_client_write_buf(VncState *vs, const uint8_t *data, size_t datalen)
 {
     Error *err = NULL;
     ssize_t ret;
-    ret = qio_channel_write(
-        vs->ioc, (const char *)data, datalen, &err);
+    ret = qio_channel_write(vs->ioc, (const char *)data, datalen, &err);
     VNC_DEBUG("Wrote wire %p %zd -> %ld\n", data, datalen, ret);
-    return vnc_client_io_error(vs, ret, &err);
+    return vnc_client_io_error(vs, ret, err);
 }
 
 
@@ -1488,10 +1483,9 @@ size_t vnc_client_read_buf(VncState *vs, uint8_t *data, size_t datalen)
 {
     ssize_t ret;
     Error *err = NULL;
-    ret = qio_channel_read(
-        vs->ioc, (char *)data, datalen, &err);
+    ret = qio_channel_read(vs->ioc, (char *)data, datalen, &err);
     VNC_DEBUG("Read wire %p %zd -> %ld\n", data, datalen, ret);
-    return vnc_client_io_error(vs, ret, &err);
+    return vnc_client_io_error(vs, ret, err);
 }
 
 
-- 
2.21.0

[PATCH v7 06/21] qdev-monitor: well form error hint helpers

[Vladimir Sementsov-Ogievskiy]

Make qbus_list_bus and qbus_list_dev hint append helpers well formed:
rename errp to errp_in, as it is IN-parameter here (which is unusual
for errp), rename functions to be error_append_*_hint.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qdev-monitor.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index 29ed73e56a..3465a1e2d0 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -328,7 +328,8 @@ static Object *qdev_get_peripheral_anon(void)
     return dev;
 }
 
-static void qbus_list_bus(DeviceState *dev, Error **errp)
+static void qbus_error_append_bus_list_hint(DeviceState *dev,
+                                            Error *const *errp)
 {
     BusState *child;
     const char *sep = " ";
@@ -342,7 +343,8 @@ static void qbus_list_bus(DeviceState *dev, Error **errp)
     error_append_hint(errp, "\n");
 }
 
-static void qbus_list_dev(BusState *bus, Error **errp)
+static void qbus_error_append_dev_list_hint(BusState *bus,
+                                            Error *const *errp)
 {
     BusChild *kid;
     const char *sep = " ";
@@ -500,7 +502,7 @@ static BusState *qbus_find(const char *path, Error **errp)
         if (!dev) {
             error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND,
                       "Device '%s' not found", elem);
-            qbus_list_dev(bus, errp);
+            qbus_error_append_dev_list_hint(bus, errp);
             return NULL;
         }
 
@@ -518,7 +520,7 @@ static BusState *qbus_find(const char *path, Error **errp)
             if (dev->num_child_bus) {
                 error_setg(errp, "Device '%s' has multiple child buses",
                            elem);
-                qbus_list_bus(dev, errp);
+                qbus_error_append_bus_list_hint(dev, errp);
             } else {
                 error_setg(errp, "Device '%s' has no child bus", elem);
             }
@@ -534,7 +536,7 @@ static BusState *qbus_find(const char *path, Error **errp)
         bus = qbus_find_bus(dev, elem);
         if (!bus) {
             error_setg(errp, "Bus '%s' not found", elem);
-            qbus_list_bus(dev, errp);
+            qbus_error_append_bus_list_hint(dev, errp);
             return NULL;
         }
     }
-- 
2.21.0

Re: [PATCH v7 06/21] qdev-monitor: well form error hint helpers

[Eric Blake]

On 12/5/19 9:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> Make qbus_list_bus and qbus_list_dev hint append helpers well formed:
> rename errp to errp_in, as it is IN-parameter here (which is unusual
> for errp), rename functions to be error_append_*_hint.

Commit message mentions a rename...

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   qdev-monitor.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/qdev-monitor.c b/qdev-monitor.c
> index 29ed73e56a..3465a1e2d0 100644
> --- a/qdev-monitor.c
> +++ b/qdev-monitor.c
> @@ -328,7 +328,8 @@ static Object *qdev_get_peripheral_anon(void)
>       return dev;
>   }
>   
> -static void qbus_list_bus(DeviceState *dev, Error **errp)
> +static void qbus_error_append_bus_list_hint(DeviceState *dev,
> +                                            Error *const *errp)

...but instead you did a type change with the name left unchanged. 
Commit message needs a fix.

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

Re: [PATCH v7 06/21] qdev-monitor: well form error hint helpers

[Vladimir Sementsov-Ogievskiy]

05.12.2019 19:58, Eric Blake wrote:
> On 12/5/19 9:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Make qbus_list_bus and qbus_list_dev hint append helpers well formed:
>> rename errp to errp_in, as it is IN-parameter here (which is unusual
>> for errp), rename functions to be error_append_*_hint.
> 
> Commit message mentions a rename...
> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   qdev-monitor.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/qdev-monitor.c b/qdev-monitor.c
>> index 29ed73e56a..3465a1e2d0 100644
>> --- a/qdev-monitor.c
>> +++ b/qdev-monitor.c
>> @@ -328,7 +328,8 @@ static Object *qdev_get_peripheral_anon(void)
>>       return dev;
>>   }
>> -static void qbus_list_bus(DeviceState *dev, Error **errp)
>> +static void qbus_error_append_bus_list_hint(DeviceState *dev,
>> +                                            Error *const *errp)
> 
> ...but instead you did a type change with the name left unchanged. Commit message needs a fix.
> 

Oops. I blindly take Markus's fixum, and didn't fix commit messages. I'll resend.

-- 
Best regards,
Vladimir

[PATCH v7 07/21] ppc: well form kvmppc_hint_smt_possible error hint helper

[Vladimir Sementsov-Ogievskiy]

Make kvmppc_hint_smt_possible hint append helper well formed:
rename errp to errp_in, as it is IN-parameter here (which is unusual
for errp), rename function to be kvmppc_error_append_*_hint.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 target/ppc/kvm_ppc.h | 4 ++--
 hw/ppc/spapr.c       | 2 +-
 target/ppc/kvm.c     | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
index 98bd7d5da6..f22daabf51 100644
--- a/target/ppc/kvm_ppc.h
+++ b/target/ppc/kvm_ppc.h
@@ -28,7 +28,7 @@ void kvmppc_set_papr(PowerPCCPU *cpu);
 int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
 void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
 int kvmppc_smt_threads(void);
-void kvmppc_hint_smt_possible(Error **errp);
+void kvmppc_error_append_smt_possible_hint(Error *const *errp);
 int kvmppc_set_smt_threads(int smt);
 int kvmppc_clear_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
 int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
@@ -164,7 +164,7 @@ static inline int kvmppc_smt_threads(void)
     return 1;
 }
 
-static inline void kvmppc_hint_smt_possible(Error **errp)
+static inline void kvmppc_error_append_smt_possible_hint(Error *const *errp)
 {
     return;
 }
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e076f6023c..1b87eb0ffd 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -2564,7 +2564,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp)
                                       " requires the use of VSMT mode %d.\n",
                                       smp_threads, kvm_smt, spapr->vsmt);
                 }
-                kvmppc_hint_smt_possible(&local_err);
+                kvmppc_error_append_smt_possible_hint(&local_err);
                 goto out;
             }
         }
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index c77f9848ec..27ea3ce535 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -2076,7 +2076,7 @@ int kvmppc_set_smt_threads(int smt)
     return ret;
 }
 
-void kvmppc_hint_smt_possible(Error **errp)
+void kvmppc_error_append_smt_possible_hint(Error *const *errp)
 {
     int i;
     GString *g;
-- 
2.21.0

Re: [PATCH v7 07/21] ppc: well form kvmppc_hint_smt_possible error hint helper

[Greg Kurz]

On Thu,  5 Dec 2019 18:20:05 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> Make kvmppc_hint_smt_possible hint append helper well formed:
> rename errp to errp_in, as it is IN-parameter here (which is unusual
> for errp), rename function to be kvmppc_error_append_*_hint.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

The changelog needs to be amended. Apart from that:

Reviewed-by: Greg Kurz <groug@kaod.org>

>  target/ppc/kvm_ppc.h | 4 ++--
>  hw/ppc/spapr.c       | 2 +-
>  target/ppc/kvm.c     | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 98bd7d5da6..f22daabf51 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -28,7 +28,7 @@ void kvmppc_set_papr(PowerPCCPU *cpu);
>  int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
>  void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
>  int kvmppc_smt_threads(void);
> -void kvmppc_hint_smt_possible(Error **errp);
> +void kvmppc_error_append_smt_possible_hint(Error *const *errp);
>  int kvmppc_set_smt_threads(int smt);
>  int kvmppc_clear_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
>  int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
> @@ -164,7 +164,7 @@ static inline int kvmppc_smt_threads(void)
>      return 1;
>  }
>  
> -static inline void kvmppc_hint_smt_possible(Error **errp)
> +static inline void kvmppc_error_append_smt_possible_hint(Error *const *errp)
>  {
>      return;
>  }
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e076f6023c..1b87eb0ffd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2564,7 +2564,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp)
>                                        " requires the use of VSMT mode %d.\n",
>                                        smp_threads, kvm_smt, spapr->vsmt);
>                  }
> -                kvmppc_hint_smt_possible(&local_err);
> +                kvmppc_error_append_smt_possible_hint(&local_err);
>                  goto out;
>              }
>          }
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index c77f9848ec..27ea3ce535 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2076,7 +2076,7 @@ int kvmppc_set_smt_threads(int smt)
>      return ret;
>  }
>  
> -void kvmppc_hint_smt_possible(Error **errp)
> +void kvmppc_error_append_smt_possible_hint(Error *const *errp)
>  {
>      int i;
>      GString *g;

Re: [PATCH v7 07/21] ppc: well form kvmppc_hint_smt_possible error hint helper

[David Gibson]

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

On Thu, Dec 05, 2019 at 06:20:05PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Make kvmppc_hint_smt_possible hint append helper well formed:
> rename errp to errp_in, as it is IN-parameter here (which is unusual
> for errp), rename function to be kvmppc_error_append_*_hint.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy
> <vsementsov@virtuozzo.com>

Apart from Greg's point about the changelog,

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

> ---
>  target/ppc/kvm_ppc.h | 4 ++--
>  hw/ppc/spapr.c       | 2 +-
>  target/ppc/kvm.c     | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index 98bd7d5da6..f22daabf51 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -28,7 +28,7 @@ void kvmppc_set_papr(PowerPCCPU *cpu);
>  int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
>  void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
>  int kvmppc_smt_threads(void);
> -void kvmppc_hint_smt_possible(Error **errp);
> +void kvmppc_error_append_smt_possible_hint(Error *const *errp);
>  int kvmppc_set_smt_threads(int smt);
>  int kvmppc_clear_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
>  int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
> @@ -164,7 +164,7 @@ static inline int kvmppc_smt_threads(void)
>      return 1;
>  }
>  
> -static inline void kvmppc_hint_smt_possible(Error **errp)
> +static inline void kvmppc_error_append_smt_possible_hint(Error *const *errp)
>  {
>      return;
>  }
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e076f6023c..1b87eb0ffd 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -2564,7 +2564,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp)
>                                        " requires the use of VSMT mode %d.\n",
>                                        smp_threads, kvm_smt, spapr->vsmt);
>                  }
> -                kvmppc_hint_smt_possible(&local_err);
> +                kvmppc_error_append_smt_possible_hint(&local_err);
>                  goto out;
>              }
>          }
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index c77f9848ec..27ea3ce535 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -2076,7 +2076,7 @@ int kvmppc_set_smt_threads(int smt)
>      return ret;
>  }
>  
> -void kvmppc_hint_smt_possible(Error **errp)
> +void kvmppc_error_append_smt_possible_hint(Error *const *errp)
>  {
>      int i;
>      GString *g;

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

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

Re: [PATCH v7 07/21] ppc: well form kvmppc_hint_smt_possible error hint helper

[Vladimir Sementsov-Ogievskiy]

06.12.2019 3:02, David Gibson wrote:
> On Thu, Dec 05, 2019 at 06:20:05PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> Make kvmppc_hint_smt_possible hint append helper well formed:
>> rename errp to errp_in, as it is IN-parameter here (which is unusual
>> for errp), rename function to be kvmppc_error_append_*_hint.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> <vsementsov@virtuozzo.com>
> 
> Apart from Greg's point about the changelog,
> 
> Acked-by: David Gibson <david@gibson.dropbear.id.au>

Thanks! actually, v8 is out with fixed commit message

> 
>> ---
>>   target/ppc/kvm_ppc.h | 4 ++--
>>   hw/ppc/spapr.c       | 2 +-
>>   target/ppc/kvm.c     | 2 +-
>>   3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
>> index 98bd7d5da6..f22daabf51 100644
>> --- a/target/ppc/kvm_ppc.h
>> +++ b/target/ppc/kvm_ppc.h
>> @@ -28,7 +28,7 @@ void kvmppc_set_papr(PowerPCCPU *cpu);
>>   int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
>>   void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
>>   int kvmppc_smt_threads(void);
>> -void kvmppc_hint_smt_possible(Error **errp);
>> +void kvmppc_error_append_smt_possible_hint(Error *const *errp);
>>   int kvmppc_set_smt_threads(int smt);
>>   int kvmppc_clear_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
>>   int kvmppc_or_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
>> @@ -164,7 +164,7 @@ static inline int kvmppc_smt_threads(void)
>>       return 1;
>>   }
>>   
>> -static inline void kvmppc_hint_smt_possible(Error **errp)
>> +static inline void kvmppc_error_append_smt_possible_hint(Error *const *errp)
>>   {
>>       return;
>>   }
>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>> index e076f6023c..1b87eb0ffd 100644
>> --- a/hw/ppc/spapr.c
>> +++ b/hw/ppc/spapr.c
>> @@ -2564,7 +2564,7 @@ static void spapr_set_vsmt_mode(SpaprMachineState *spapr, Error **errp)
>>                                         " requires the use of VSMT mode %d.\n",
>>                                         smp_threads, kvm_smt, spapr->vsmt);
>>                   }
>> -                kvmppc_hint_smt_possible(&local_err);
>> +                kvmppc_error_append_smt_possible_hint(&local_err);
>>                   goto out;
>>               }
>>           }
>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index c77f9848ec..27ea3ce535 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -2076,7 +2076,7 @@ int kvmppc_set_smt_threads(int smt)
>>       return ret;
>>   }
>>   
>> -void kvmppc_hint_smt_possible(Error **errp)
>> +void kvmppc_error_append_smt_possible_hint(Error *const *errp)
>>   {
>>       int i;
>>       GString *g;
> 


-- 
Best regards,
Vladimir

[PATCH v7 08/21] 9pfs: well form error hint helpers

[Vladimir Sementsov-Ogievskiy]

Make error_append_security_model_hint and
error_append_socket_sockfd_hint hint append helpers well formed:
rename errp to errp_in, as it is IN-parameter here (which is unusual
for errp).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/9pfs/9p-local.c | 2 +-
 hw/9pfs/9p-proxy.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
index 4708c0bd89..ca641390fb 100644
--- a/hw/9pfs/9p-local.c
+++ b/hw/9pfs/9p-local.c
@@ -1473,7 +1473,7 @@ static void local_cleanup(FsContext *ctx)
     g_free(data);
 }
 
-static void error_append_security_model_hint(Error **errp)
+static void error_append_security_model_hint(Error *const *errp)
 {
     error_append_hint(errp, "Valid options are: security_model="
                       "[passthrough|mapped-xattr|mapped-file|none]\n");
diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
index 97ab9c58a5..8136e1342d 100644
--- a/hw/9pfs/9p-proxy.c
+++ b/hw/9pfs/9p-proxy.c
@@ -1114,7 +1114,7 @@ static int connect_namedsocket(const char *path, Error **errp)
     return sockfd;
 }
 
-static void error_append_socket_sockfd_hint(Error **errp)
+static void error_append_socket_sockfd_hint(Error *const *errp)
 {
     error_append_hint(errp, "Either specify socket=/some/path where /some/path"
                       " points to a listening AF_UNIX socket or sock_fd=fd"
-- 
2.21.0

Re: [PATCH v7 08/21] 9pfs: well form error hint helpers

[Greg Kurz]

On Thu,  5 Dec 2019 18:20:06 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> Make error_append_security_model_hint and
> error_append_socket_sockfd_hint hint append helpers well formed:
> rename errp to errp_in, as it is IN-parameter here (which is unusual
> for errp).
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---

Acked-by: Greg Kurz <groug@kaod.org>

>  hw/9pfs/9p-local.c | 2 +-
>  hw/9pfs/9p-proxy.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> index 4708c0bd89..ca641390fb 100644
> --- a/hw/9pfs/9p-local.c
> +++ b/hw/9pfs/9p-local.c
> @@ -1473,7 +1473,7 @@ static void local_cleanup(FsContext *ctx)
>      g_free(data);
>  }
>  
> -static void error_append_security_model_hint(Error **errp)
> +static void error_append_security_model_hint(Error *const *errp)
>  {
>      error_append_hint(errp, "Valid options are: security_model="
>                        "[passthrough|mapped-xattr|mapped-file|none]\n");
> diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> index 97ab9c58a5..8136e1342d 100644
> --- a/hw/9pfs/9p-proxy.c
> +++ b/hw/9pfs/9p-proxy.c
> @@ -1114,7 +1114,7 @@ static int connect_namedsocket(const char *path, Error **errp)
>      return sockfd;
>  }
>  
> -static void error_append_socket_sockfd_hint(Error **errp)
> +static void error_append_socket_sockfd_hint(Error *const *errp)
>  {
>      error_append_hint(errp, "Either specify socket=/some/path where /some/path"
>                        " points to a listening AF_UNIX socket or sock_fd=fd"

Re: [PATCH v7 08/21] 9pfs: well form error hint helpers

[Greg Kurz]

On Thu, 5 Dec 2019 18:08:56 +0100
Greg Kurz <groug@kaod.org> wrote:

> On Thu,  5 Dec 2019 18:20:06 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
> > Make error_append_security_model_hint and
> > error_append_socket_sockfd_hint hint append helpers well formed:
> > rename errp to errp_in, as it is IN-parameter here (which is unusual
> > for errp).
> > 
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> 
> Acked-by: Greg Kurz <groug@kaod.org>
> 

With an updated changelog that no longer mentions errp_in :)

> >  hw/9pfs/9p-local.c | 2 +-
> >  hw/9pfs/9p-proxy.c | 2 +-
> >  2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c
> > index 4708c0bd89..ca641390fb 100644
> > --- a/hw/9pfs/9p-local.c
> > +++ b/hw/9pfs/9p-local.c
> > @@ -1473,7 +1473,7 @@ static void local_cleanup(FsContext *ctx)
> >      g_free(data);
> >  }
> >  
> > -static void error_append_security_model_hint(Error **errp)
> > +static void error_append_security_model_hint(Error *const *errp)
> >  {
> >      error_append_hint(errp, "Valid options are: security_model="
> >                        "[passthrough|mapped-xattr|mapped-file|none]\n");
> > diff --git a/hw/9pfs/9p-proxy.c b/hw/9pfs/9p-proxy.c
> > index 97ab9c58a5..8136e1342d 100644
> > --- a/hw/9pfs/9p-proxy.c
> > +++ b/hw/9pfs/9p-proxy.c
> > @@ -1114,7 +1114,7 @@ static int connect_namedsocket(const char *path, Error **errp)
> >      return sockfd;
> >  }
> >  
> > -static void error_append_socket_sockfd_hint(Error **errp)
> > +static void error_append_socket_sockfd_hint(Error *const *errp)
> >  {
> >      error_append_hint(errp, "Either specify socket=/some/path where /some/path"
> >                        " points to a listening AF_UNIX socket or sock_fd=fd"
> 

[PATCH v7 09/21] hw/core/qdev: cleanup Error ** variables

[Vladimir Sementsov-Ogievskiy]

Rename Error ** parameter in check_only_migratable to common errp.

In device_set_realized:

 - Move "if (local_err != NULL)" closer to error setters.

 - Drop 'Error **local_errp': it doesn't save any LoCs, but it's very
   unusual.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/core/qdev.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index cf1ba28fe3..82d3ee590a 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -820,12 +820,12 @@ static bool device_get_realized(Object *obj, Error **errp)
     return dev->realized;
 }
 
-static bool check_only_migratable(Object *obj, Error **err)
+static bool check_only_migratable(Object *obj, Error **errp)
 {
     DeviceClass *dc = DEVICE_GET_CLASS(obj);
 
     if (!vmstate_check_only_migratable(dc->vmsd)) {
-        error_setg(err, "Device %s is not migratable, but "
+        error_setg(errp, "Device %s is not migratable, but "
                    "--only-migratable was specified",
                    object_get_typename(obj));
         return false;
@@ -874,10 +874,9 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
 
         if (dc->realize) {
             dc->realize(dev, &local_err);
-        }
-
-        if (local_err != NULL) {
-            goto fail;
+            if (local_err != NULL) {
+                goto fail;
+            }
         }
 
         DEVICE_LISTENER_CALL(realize, Forward, dev);
@@ -918,27 +917,26 @@ static void device_set_realized(Object *obj, bool value, Error **errp)
        }
 
     } else if (!value && dev->realized) {
-        Error **local_errp = NULL;
+        /* We want local_err to track only the first error */
         QLIST_FOREACH(bus, &dev->child_bus, sibling) {
-            local_errp = local_err ? NULL : &local_err;
             object_property_set_bool(OBJECT(bus), false, "realized",
-                                     local_errp);
+                                     local_err ? NULL : &local_err);
         }
         if (qdev_get_vmsd(dev)) {
             vmstate_unregister(dev, qdev_get_vmsd(dev), dev);
         }
         if (dc->unrealize) {
-            local_errp = local_err ? NULL : &local_err;
-            dc->unrealize(dev, local_errp);
+            dc->unrealize(dev, local_err ? NULL : &local_err);
         }
         dev->pending_deleted_event = true;
         DEVICE_LISTENER_CALL(unrealize, Reverse, dev);
-    }
 
-    if (local_err != NULL) {
-        goto fail;
+        if (local_err != NULL) {
+            goto fail;
+        }
     }
 
+    assert(local_err == NULL);
     dev->realized = value;
     return;
 
@@ -976,7 +974,7 @@ static bool device_get_hotpluggable(Object *obj, Error **errp)
                                 qbus_is_hotpluggable(dev->parent_bus));
 }
 
-static bool device_get_hotplugged(Object *obj, Error **err)
+static bool device_get_hotplugged(Object *obj, Error **errp)
 {
     DeviceState *dev = DEVICE(obj);
 
-- 
2.21.0

[PATCH v7 10/21] block/snapshot: rename Error ** parameter to more common errp

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Eric Blake <eblake@Redhat.com>
---
 include/block/snapshot.h | 2 +-
 block/snapshot.c         | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/block/snapshot.h b/include/block/snapshot.h
index b5d5084a12..2bfcd57578 100644
--- a/include/block/snapshot.h
+++ b/include/block/snapshot.h
@@ -78,7 +78,7 @@ int bdrv_snapshot_load_tmp_by_id_or_name(BlockDriverState *bs,
 
 bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
 int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
-                             Error **err);
+                             Error **errp);
 int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs,
                            Error **errp);
 int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
diff --git a/block/snapshot.c b/block/snapshot.c
index 8081616ae9..bd9fb01817 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -426,7 +426,7 @@ fail:
 }
 
 int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
-                             Error **err)
+                             Error **errp)
 {
     int ret = 0;
     BlockDriverState *bs;
@@ -441,7 +441,7 @@ int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bad_bs,
             bdrv_snapshot_find(bs, snapshot, name) >= 0)
         {
             ret = bdrv_snapshot_delete(bs, snapshot->id_str,
-                                       snapshot->name, err);
+                                       snapshot->name, errp);
         }
         aio_context_release(ctx);
         if (ret < 0) {
-- 
2.21.0

[PATCH v7 11/21] hw/i386/amd_iommu: rename Error ** parameter to more common errp

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/i386/amd_iommu.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/i386/amd_iommu.c b/hw/i386/amd_iommu.c
index d55dbf07fc..b1175e52c7 100644
--- a/hw/i386/amd_iommu.c
+++ b/hw/i386/amd_iommu.c
@@ -1533,7 +1533,7 @@ static void amdvi_reset(DeviceState *dev)
     amdvi_init(s);
 }
 
-static void amdvi_realize(DeviceState *dev, Error **err)
+static void amdvi_realize(DeviceState *dev, Error **errp)
 {
     int ret = 0;
     AMDVIState *s = AMD_IOMMU_DEVICE(dev);
@@ -1549,21 +1549,21 @@ static void amdvi_realize(DeviceState *dev, Error **err)
     /* This device should take care of IOMMU PCI properties */
     x86_iommu->type = TYPE_AMD;
     qdev_set_parent_bus(DEVICE(&s->pci), &bus->qbus);
-    object_property_set_bool(OBJECT(&s->pci), true, "realized", err);
+    object_property_set_bool(OBJECT(&s->pci), true, "realized", errp);
     ret = pci_add_capability(&s->pci.dev, AMDVI_CAPAB_ID_SEC, 0,
-                                         AMDVI_CAPAB_SIZE, err);
+                                         AMDVI_CAPAB_SIZE, errp);
     if (ret < 0) {
         return;
     }
     s->capab_offset = ret;
 
     ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_MSI, 0,
-                             AMDVI_CAPAB_REG_SIZE, err);
+                             AMDVI_CAPAB_REG_SIZE, errp);
     if (ret < 0) {
         return;
     }
     ret = pci_add_capability(&s->pci.dev, PCI_CAP_ID_HT, 0,
-                             AMDVI_CAPAB_REG_SIZE, err);
+                             AMDVI_CAPAB_REG_SIZE, errp);
     if (ret < 0) {
         return;
     }
@@ -1578,8 +1578,8 @@ static void amdvi_realize(DeviceState *dev, Error **err)
     sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->mmio);
     sysbus_mmio_map(SYS_BUS_DEVICE(s), 0, AMDVI_BASE_ADDR);
     pci_setup_iommu(bus, amdvi_host_dma_iommu, s);
-    s->devid = object_property_get_int(OBJECT(&s->pci), "addr", err);
-    msi_init(&s->pci.dev, 0, 1, true, false, err);
+    s->devid = object_property_get_int(OBJECT(&s->pci), "addr", errp);
+    msi_init(&s->pci.dev, 0, 1, true, false, errp);
     amdvi_init(s);
 }
 
-- 
2.21.0

[PATCH v7 12/21] qga: rename Error ** parameter to more common errp

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qga/commands-posix.c |  2 +-
 qga/commands-win32.c |  2 +-
 qga/commands.c       | 12 ++++++------
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 1c1a165dae..3bd7b54c08 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -2781,7 +2781,7 @@ static double ga_get_login_time(struct utmpx *user_info)
     return seconds + useconds;
 }
 
-GuestUserList *qmp_guest_get_users(Error **err)
+GuestUserList *qmp_guest_get_users(Error **errp)
 {
     GHashTable *cache = NULL;
     GuestUserList *head = NULL, *cur_item = NULL;
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 55ba5b263a..2461fd19bf 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -1946,7 +1946,7 @@ typedef struct _GA_WTSINFOA {
 
 } GA_WTSINFOA;
 
-GuestUserList *qmp_guest_get_users(Error **err)
+GuestUserList *qmp_guest_get_users(Error **errp)
 {
 #define QGA_NANOSECONDS 10000000
 
diff --git a/qga/commands.c b/qga/commands.c
index 0c7d1385c2..43c323cead 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -143,7 +143,7 @@ static GuestExecInfo *guest_exec_info_find(int64_t pid_numeric)
     return NULL;
 }
 
-GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **err)
+GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **errp)
 {
     GuestExecInfo *gei;
     GuestExecStatus *ges;
@@ -152,7 +152,7 @@ GuestExecStatus *qmp_guest_exec_status(int64_t pid, Error **err)
 
     gei = guest_exec_info_find(pid);
     if (gei == NULL) {
-        error_setg(err, QERR_INVALID_PARAMETER, "pid");
+        error_setg(errp, QERR_INVALID_PARAMETER, "pid");
         return NULL;
     }
 
@@ -385,7 +385,7 @@ GuestExec *qmp_guest_exec(const char *path,
                        bool has_env, strList *env,
                        bool has_input_data, const char *input_data,
                        bool has_capture_output, bool capture_output,
-                       Error **err)
+                       Error **errp)
 {
     GPid pid;
     GuestExec *ge = NULL;
@@ -405,7 +405,7 @@ GuestExec *qmp_guest_exec(const char *path,
     arglist.next = has_arg ? arg : NULL;
 
     if (has_input_data) {
-        input = qbase64_decode(input_data, -1, &ninput, err);
+        input = qbase64_decode(input_data, -1, &ninput, errp);
         if (!input) {
             return NULL;
         }
@@ -424,7 +424,7 @@ GuestExec *qmp_guest_exec(const char *path,
             guest_exec_task_setup, NULL, &pid, has_input_data ? &in_fd : NULL,
             has_output ? &out_fd : NULL, has_output ? &err_fd : NULL, &gerr);
     if (!ret) {
-        error_setg(err, QERR_QGA_COMMAND_FAILED, gerr->message);
+        error_setg(errp, QERR_QGA_COMMAND_FAILED, gerr->message);
         g_error_free(gerr);
         goto done;
     }
@@ -499,7 +499,7 @@ int ga_parse_whence(GuestFileWhence *whence, Error **errp)
     return -1;
 }
 
-GuestHostName *qmp_guest_get_host_name(Error **err)
+GuestHostName *qmp_guest_get_host_name(Error **errp)
 {
     GuestHostName *result = NULL;
     gchar const *hostname = g_get_host_name();
-- 
2.21.0

[PATCH v7 13/21] monitor/qmp-cmds: rename Error ** parameter to more common errp

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 monitor/qmp-cmds.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
index 0880341a2d..c6faa3eaf0 100644
--- a/monitor/qmp-cmds.c
+++ b/monitor/qmp-cmds.c
@@ -109,7 +109,7 @@ void qmp_system_reset(Error **errp)
     qemu_system_reset_request(SHUTDOWN_CAUSE_HOST_QMP_SYSTEM_RESET);
 }
 
-void qmp_system_powerdown(Error **erp)
+void qmp_system_powerdown(Error **errp)
 {
     qemu_system_powerdown_request();
 }
-- 
2.21.0

[PATCH v7 14/21] hw/s390x: rename Error ** parameter to more common errp

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/s390x/event-facility.c | 2 +-
 hw/s390x/s390-stattrib.c  | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 66205697ae..dc733ee2af 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -439,7 +439,7 @@ static void sclp_event_set_allow_all_mask_sizes(Object *obj, bool value,
     ef->allow_all_mask_sizes = value;
 }
 
-static bool sclp_event_get_allow_all_mask_sizes(Object *obj, Error **e)
+static bool sclp_event_get_allow_all_mask_sizes(Object *obj, Error **errp)
 {
     SCLPEventFacility *ef = (SCLPEventFacility *)obj;
 
diff --git a/hw/s390x/s390-stattrib.c b/hw/s390x/s390-stattrib.c
index bf5ac014c4..58121b9f68 100644
--- a/hw/s390x/s390-stattrib.c
+++ b/hw/s390x/s390-stattrib.c
@@ -352,7 +352,8 @@ static void s390_stattrib_class_init(ObjectClass *oc, void *data)
     dc->realize = s390_stattrib_realize;
 }
 
-static inline bool s390_stattrib_get_migration_enabled(Object *obj, Error **e)
+static inline bool s390_stattrib_get_migration_enabled(Object *obj,
+                                                       Error **errp)
 {
     S390StAttribState *s = S390_STATTRIB(obj);
 
-- 
2.21.0

[PATCH v7 15/21] hw/sd: drop extra whitespace in sdhci_sysbus_realize() header

[Vladimir Sementsov-Ogievskiy]

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

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 88404d0e9d..18c0c052ce 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1459,7 +1459,7 @@ static void sdhci_sysbus_finalize(Object *obj)
     sdhci_uninitfn(s);
 }
 
-static void sdhci_sysbus_realize(DeviceState *dev, Error ** errp)
+static void sdhci_sysbus_realize(DeviceState *dev, Error **errp)
 {
     SDHCIState *s = SYSBUS_SDHCI(dev);
     SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
-- 
2.21.0

[PATCH v7 16/21] hw/tpm: rename Error ** parameter to more common errp

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 hw/tpm/tpm_emulator.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/tpm/tpm_emulator.c b/hw/tpm/tpm_emulator.c
index 22f9113432..10d587ed40 100644
--- a/hw/tpm/tpm_emulator.c
+++ b/hw/tpm/tpm_emulator.c
@@ -155,7 +155,7 @@ static int tpm_emulator_unix_tx_bufs(TPMEmulator *tpm_emu,
                                      const uint8_t *in, uint32_t in_len,
                                      uint8_t *out, uint32_t out_len,
                                      bool *selftest_done,
-                                     Error **err)
+                                     Error **errp)
 {
     ssize_t ret;
     bool is_selftest = false;
@@ -165,20 +165,20 @@ static int tpm_emulator_unix_tx_bufs(TPMEmulator *tpm_emu,
         is_selftest = tpm_util_is_selftest(in, in_len);
     }
 
-    ret = qio_channel_write_all(tpm_emu->data_ioc, (char *)in, in_len, err);
+    ret = qio_channel_write_all(tpm_emu->data_ioc, (char *)in, in_len, errp);
     if (ret != 0) {
         return -1;
     }
 
     ret = qio_channel_read_all(tpm_emu->data_ioc, (char *)out,
-              sizeof(struct tpm_resp_hdr), err);
+              sizeof(struct tpm_resp_hdr), errp);
     if (ret != 0) {
         return -1;
     }
 
     ret = qio_channel_read_all(tpm_emu->data_ioc,
               (char *)out + sizeof(struct tpm_resp_hdr),
-              tpm_cmd_get_size(out) - sizeof(struct tpm_resp_hdr), err);
+              tpm_cmd_get_size(out) - sizeof(struct tpm_resp_hdr), errp);
     if (ret != 0) {
         return -1;
     }
-- 
2.21.0

[PATCH v7 17/21] hw/usb: rename Error ** parameter to more common errp

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 hw/usb/dev-network.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/dev-network.c b/hw/usb/dev-network.c
index 5de6213cc4..b81a8abe83 100644
--- a/hw/usb/dev-network.c
+++ b/hw/usb/dev-network.c
@@ -1342,7 +1342,7 @@ static NetClientInfo net_usbnet_info = {
     .cleanup = usbnet_cleanup,
 };
 
-static void usb_net_realize(USBDevice *dev, Error **errrp)
+static void usb_net_realize(USBDevice *dev, Error **errp)
 {
     USBNetState *s = USB_NET(dev);
 
-- 
2.21.0

[PATCH v7 18/21] include/qom/object.h: rename Error ** parameter to more common errp

[Vladimir Sementsov-Ogievskiy]

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/qom/object.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/qom/object.h b/include/qom/object.h
index 128d00c77f..716f6f655d 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -1635,9 +1635,9 @@ void object_class_property_add_uint32_ptr(ObjectClass *klass, const char *name,
  * property of type 'uint64'.
  */
 void object_property_add_uint64_ptr(Object *obj, const char *name,
-                                    const uint64_t *v, Error **Errp);
+                                    const uint64_t *v, Error **errp);
 void object_class_property_add_uint64_ptr(ObjectClass *klass, const char *name,
-                                          const uint64_t *v, Error **Errp);
+                                          const uint64_t *v, Error **errp);
 
 /**
  * object_property_add_alias:
-- 
2.21.0

[PATCH v7 19/21] backends/cryptodev: drop local_err from cryptodev_backend_complete()

[Vladimir Sementsov-Ogievskiy]

No reason for local_err here, use errp directly instead.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Gonglei <arei.gonglei@huawei.com>
---
 backends/cryptodev.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/backends/cryptodev.c b/backends/cryptodev.c
index 3c071eab95..5a9735684e 100644
--- a/backends/cryptodev.c
+++ b/backends/cryptodev.c
@@ -176,19 +176,10 @@ cryptodev_backend_complete(UserCreatable *uc, Error **errp)
 {
     CryptoDevBackend *backend = CRYPTODEV_BACKEND(uc);
     CryptoDevBackendClass *bc = CRYPTODEV_BACKEND_GET_CLASS(uc);
-    Error *local_err = NULL;
 
     if (bc->init) {
-        bc->init(backend, &local_err);
-        if (local_err) {
-            goto out;
-        }
+        bc->init(backend, errp);
     }
-
-    return;
-
-out:
-    error_propagate(errp, local_err);
 }
 
 void cryptodev_backend_set_used(CryptoDevBackend *backend, bool used)
-- 
2.21.0

[PATCH v7 20/21] hw/vfio/ap: drop local_err from vfio_ap_realize

[Vladimir Sementsov-Ogievskiy]

No reason for local_err here, use errp directly instead.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 hw/vfio/ap.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index da6a20669d..8fbaa724c2 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -89,14 +89,13 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
 {
     int ret;
     char *mdevid;
-    Error *local_err = NULL;
     VFIOGroup *vfio_group;
     APDevice *apdev = AP_DEVICE(dev);
     VFIOAPDevice *vapdev = VFIO_AP_DEVICE(apdev);
 
-    vfio_group = vfio_ap_get_group(vapdev, &local_err);
+    vfio_group = vfio_ap_get_group(vapdev, errp);
     if (!vfio_group) {
-        goto out_err;
+        return;
     }
 
     vapdev->vdev.ops = &vfio_ap_ops;
@@ -113,7 +112,7 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
      */
     vapdev->vdev.balloon_allowed = true;
 
-    ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, &local_err);
+    ret = vfio_get_device(vfio_group, mdevid, &vapdev->vdev, errp);
     if (ret) {
         goto out_get_dev_err;
     }
@@ -123,8 +122,6 @@ static void vfio_ap_realize(DeviceState *dev, Error **errp)
 out_get_dev_err:
     vfio_ap_put_device(vapdev);
     vfio_put_group(vfio_group);
-out_err:
-    error_propagate(errp, local_err);
 }
 
 static void vfio_ap_unrealize(DeviceState *dev, Error **errp)
-- 
2.21.0

Re: [PATCH v7 20/21] hw/vfio/ap: drop local_err from vfio_ap_realize

[Cornelia Huck]

On Thu,  5 Dec 2019 18:20:18 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> No reason for local_err here, use errp directly instead.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/vfio/ap.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

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

[PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error

[Vladimir Sementsov-Ogievskiy]

The local_err parameter is not here to return information about
nbd_iter_channel_error failure. Instead it's assumed to be filled when
passed to the function. This is already stressed by its name
(local_err, instead of classic errp). Stress it additionally by
assertion.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/nbd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/nbd.c b/block/nbd.c
index 5f18f78a94..d085554f21 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -866,6 +866,7 @@ typedef struct NBDReplyChunkIter {
 static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
                                    int ret, Error **local_err)
 {
+    assert(local_err && *local_err);
     assert(ret < 0);
 
     if (!iter->ret) {
-- 
2.21.0

Re: [PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error

[Eric Blake]

On 12/5/19 9:20 AM, Vladimir Sementsov-Ogievskiy wrote:
> The local_err parameter is not here to return information about
> nbd_iter_channel_error failure. Instead it's assumed to be filled when
> passed to the function. This is already stressed by its name
> (local_err, instead of classic errp). Stress it additionally by
> assertion.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/nbd.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 5f18f78a94..d085554f21 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -866,6 +866,7 @@ typedef struct NBDReplyChunkIter {
>   static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
>                                      int ret, Error **local_err)
>   {
> +    assert(local_err && *local_err);

Why are we forbidding grandparent callers from passing NULL when they 
want to ignore an error?  We are called by several parent functions that 
get an errp from the grandparent, and use this function to do some 
common grunt work.  Let's look at the possibilities:

1. grandparent passes address of a local error: we want to append to the 
error message, parent will then deal with the error how it wants (report 
it, ignore it, propagate it, whatever)

2. grandparent passes &error_fatal: we want to append a hint, but before 
ERRP_AUTO_PROPAGATE, the parent has already exited.  After 
ERRP_AUTO_PROPAGATE, this looks like case 1.

3. grandparent passes &error_abort: we should never be reached (failure 
earlier in the parent should have already aborted) - true whether or not 
ERRP_AUTO_PROPAGATE is applied

4. grandparent passes NULL to ignore the error. Does not currently 
happen in any of the grandparent callers, because if it did, your 
assertion in this patch would now fire.  After ERRP_AUTO_PROPAGATE, this 
would look like case 1.

Would it be better to assert(!local_err || *local_err)?  The assertion 
as written is too strict without ERRP_AUTO_PROPAGATE, but you get away 
with it because none of the grandparents pass NULL; but is appropriate 
as written for after after the macro conversion so then we wonder if 
churn on the macro is worth it.

>       assert(ret < 0);
>   
>       if (!iter->ret) {
> 

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

Re: [PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error

[Vladimir Sementsov-Ogievskiy]

05.12.2019 20:14, Eric Blake wrote:
> On 12/5/19 9:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>> The local_err parameter is not here to return information about
>> nbd_iter_channel_error failure. Instead it's assumed to be filled when
>> passed to the function. This is already stressed by its name
>> (local_err, instead of classic errp). Stress it additionally by
>> assertion.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/nbd.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 5f18f78a94..d085554f21 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -866,6 +866,7 @@ typedef struct NBDReplyChunkIter {
>>   static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
>>                                      int ret, Error **local_err)
>>   {
>> +    assert(local_err && *local_err);
> 
> Why are we forbidding grandparent callers from passing NULL when they want to ignore an error?  We are called by several parent functions that get an errp from the grandparent, and use this function to do some common grunt work.  Let's look at the possibilities:
> 
> 1. grandparent passes address of a local error: we want to append to the error message, parent will then deal with the error how it wants (report it, ignore it, propagate it, whatever)
> 
> 2. grandparent passes &error_fatal: we want to append a hint, but before ERRP_AUTO_PROPAGATE, the parent has already exited.  After ERRP_AUTO_PROPAGATE, this looks like case 1.
> 
> 3. grandparent passes &error_abort: we should never be reached (failure earlier in the parent should have already aborted) - true whether or not ERRP_AUTO_PROPAGATE is applied
> 
> 4. grandparent passes NULL to ignore the error. Does not currently happen in any of the grandparent callers, because if it did, your assertion in this patch would now fire.  After ERRP_AUTO_PROPAGATE, this would look like case 1.
> 
> Would it be better to assert(!local_err || *local_err)?  The assertion as written is too strict without ERRP_AUTO_PROPAGATE, but you get away with it because none of the grandparents pass NULL; but is appropriate as written for after after the macro conversion so then we wonder if churn on the macro is worth it.

We don't have any grandparents, this function is always called on local_err. And it's argument named local_err to stress it.
The function is an API to report error, and it wants filled local_err object.

It will crash anyway if local_err is NULL, as it dereferences it.

I just want to place an assertion at start of functions like this,
which will be easily recognizable by coccinelle.

---

We can improve the API, to support local_err==NULL, for the case when original request was called with
errp==NULL, but for this we'll need more changes, like, pass errp to NBD_FOREACH_REPLY_CHUNK and save
it into iter object...

But how to detect it in code? Something like


--- a/block/nbd.c
+++ b/block/nbd.c
@@ -1059,8 +1059,10 @@ static int nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
          case NBD_REPLY_TYPE_BLOCK_STATUS:
              if (received) {
                  nbd_channel_error(s, -EINVAL);
-                error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
-                nbd_iter_channel_error(&iter, -EINVAL, &local_err);
+                if (errp) {
+                    error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
+                }
+                nbd_iter_channel_error(&iter, -EINVAL, errp ? &local_err : NULL);
              }
              received = true;


is ugly..


So, to support original errp=NULL the whole thing should be refactored.. Not worth it, I think.


> 
>>       assert(ret < 0);
>>       if (!iter->ret) {
>>
> 


-- 
Best regards,
Vladimir

Re: [PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error

[Eric Blake]

On 12/5/19 11:39 AM, Vladimir Sementsov-Ogievskiy wrote:
> 05.12.2019 20:14, Eric Blake wrote:
>> On 12/5/19 9:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> The local_err parameter is not here to return information about
>>> nbd_iter_channel_error failure. Instead it's assumed to be filled when
>>> passed to the function. This is already stressed by its name
>>> (local_err, instead of classic errp). Stress it additionally by
>>> assertion.
>>>

>>
>> Would it be better to assert(!local_err || *local_err)?  The assertion as written is too strict without ERRP_AUTO_PROPAGATE, but you get away with it because none of the grandparents pass NULL; but is appropriate as written for after after the macro conversion so then we wonder if churn on the macro is worth it.
> 
> We don't have any grandparents, this function is always called on local_err. And it's argument named local_err to stress it.

Then the commit message should state that. How about:

All callers of nbd_iter_channel_error() pass the address of a local_err 
variable, and only call this function if an error has already occurred, 
using this function to append details to that error.  This is already 
implied by its name (local_err instead of the classic errp), but it is 
worth additionally stressing this by adding an assertion to make it part 
of the function contract.

> The function is an API to report error, and it wants filled local_err object.
> 
> It will crash anyway if local_err is NULL, as it dereferences it.
> 
> I just want to place an assertion at start of functions like this,
> which will be easily recognizable by coccinelle.

With an improved commit message, the assertion makes sense, so

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

> 
> ---
> 
> We can improve the API, to support local_err==NULL, for the case when original request was called with
> errp==NULL, but for this we'll need more changes, like, pass errp to NBD_FOREACH_REPLY_CHUNK and save
> it into iter object...
> 
> But how to detect it in code? Something like
> 
> 
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1059,8 +1059,10 @@ static int nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>            case NBD_REPLY_TYPE_BLOCK_STATUS:
>                if (received) {
>                    nbd_channel_error(s, -EINVAL);
> -                error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
> -                nbd_iter_channel_error(&iter, -EINVAL, &local_err);
> +                if (errp) {
> +                    error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
> +                }
> +                nbd_iter_channel_error(&iter, -EINVAL, errp ? &local_err : NULL);

No, that's not worth it.

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

Re: [PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error

[Vladimir Sementsov-Ogievskiy]

05.12.2019 20:49, Eric Blake wrote:
> On 12/5/19 11:39 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 05.12.2019 20:14, Eric Blake wrote:
>>> On 12/5/19 9:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> The local_err parameter is not here to return information about
>>>> nbd_iter_channel_error failure. Instead it's assumed to be filled when
>>>> passed to the function. This is already stressed by its name
>>>> (local_err, instead of classic errp). Stress it additionally by
>>>> assertion.
>>>>
> 
>>>
>>> Would it be better to assert(!local_err || *local_err)?  The assertion as written is too strict without ERRP_AUTO_PROPAGATE, but you get away with it because none of the grandparents pass NULL; but is appropriate as written for after after the macro conversion so then we wonder if churn on the macro is worth it.
>>
>> We don't have any grandparents, this function is always called on local_err. And it's argument named local_err to stress it.
> 
> Then the commit message should state that. How about:
> 
> All callers of nbd_iter_channel_error() pass the address of a local_err variable, and only call this function if an error has already occurred, using this function

> to append details to that error. 

Hmm, not to append details but to report the error to the magical receiving loop, which doesn't yet know about the error

> This is already implied by its name (local_err instead of the classic errp), but it is worth additionally stressing this by adding an assertion to make it part of the function contract.


So, how about simply s/to append details to that error/to report that error/ ?

> 
>> The function is an API to report error, and it wants filled local_err object.
>>
>> It will crash anyway if local_err is NULL, as it dereferences it.
>>
>> I just want to place an assertion at start of functions like this,
>> which will be easily recognizable by coccinelle.
> 
> With an improved commit message, the assertion makes sense, so
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
>>
>> ---
>>
>> We can improve the API, to support local_err==NULL, for the case when original request was called with
>> errp==NULL, but for this we'll need more changes, like, pass errp to NBD_FOREACH_REPLY_CHUNK and save
>> it into iter object...
>>
>> But how to detect it in code? Something like
>>
>>
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -1059,8 +1059,10 @@ static int nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>>            case NBD_REPLY_TYPE_BLOCK_STATUS:
>>                if (received) {
>>                    nbd_channel_error(s, -EINVAL);
>> -                error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
>> -                nbd_iter_channel_error(&iter, -EINVAL, &local_err);
>> +                if (errp) {
>> +                    error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
>> +                }
>> +                nbd_iter_channel_error(&iter, -EINVAL, errp ? &local_err : NULL);
> 
> No, that's not worth it.
> 


-- 
Best regards,
Vladimir

Re: [PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error

[Eric Blake]

On 12/5/19 12:09 PM, Vladimir Sementsov-Ogievskiy wrote:

>>
>> All callers of nbd_iter_channel_error() pass the address of a local_err variable, and only call this function if an error has already occurred, using this function
> 
>> to append details to that error.
> 
> Hmm, not to append details but to report the error to the magical receiving loop, which doesn't yet know about the error
> 
>> This is already implied by its name (local_err instead of the classic errp), but it is worth additionally stressing this by adding an assertion to make it part of the function contract.
> 
> 
> So, how about simply s/to append details to that error/to report that error/ ?

That should be fine.

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

Re: [PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error

[Markus Armbruster]

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

> 05.12.2019 20:14, Eric Blake wrote:
>> On 12/5/19 9:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> The local_err parameter is not here to return information about
>>> nbd_iter_channel_error failure. Instead it's assumed to be filled when
>>> passed to the function. This is already stressed by its name
>>> (local_err, instead of classic errp). Stress it additionally by
>>> assertion.
>>>
>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>   block/nbd.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/block/nbd.c b/block/nbd.c
>>> index 5f18f78a94..d085554f21 100644
>>> --- a/block/nbd.c
>>> +++ b/block/nbd.c
>>> @@ -866,6 +866,7 @@ typedef struct NBDReplyChunkIter {
>>>   static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
>>>                                      int ret, Error **local_err)
>>>   {
>>> +    assert(local_err && *local_err);
>> 
>> Why are we forbidding grandparent callers from passing NULL when they want to ignore an error?  We are called by several parent functions that get an errp from the grandparent, and use this function to do some common grunt work.  Let's look at the possibilities:
>> 
>> 1. grandparent passes address of a local error: we want to append to the error message, parent will then deal with the error how it wants (report it, ignore it, propagate it, whatever)
>> 
>> 2. grandparent passes &error_fatal: we want to append a hint, but before ERRP_AUTO_PROPAGATE, the parent has already exited.  After ERRP_AUTO_PROPAGATE, this looks like case 1.
>> 
>> 3. grandparent passes &error_abort: we should never be reached (failure earlier in the parent should have already aborted) - true whether or not ERRP_AUTO_PROPAGATE is applied
>> 
>> 4. grandparent passes NULL to ignore the error. Does not currently happen in any of the grandparent callers, because if it did, your assertion in this patch would now fire.  After ERRP_AUTO_PROPAGATE, this would look like case 1.
>> 
>> Would it be better to assert(!local_err || *local_err)?  The assertion as written is too strict without ERRP_AUTO_PROPAGATE, but you get away with it because none of the grandparents pass NULL; but is appropriate as written for after after the macro conversion so then we wonder if churn on the macro is worth it.
>
> We don't have any grandparents, this function is always called on local_err. And it's argument named local_err to stress it.
> The function is an API to report error, and it wants filled local_err object.
>
> It will crash anyway if local_err is NULL, as it dereferences it.

Yes.

We already assert ret < 0 explicitly, and we rely on !local_err
implicitly.  Making that explicit is obviously safe.

The code doesn't actually rely on !*local_err.  But when ret < 0, and
!local_err, surely local_err should point to an Error object.  Asserting
that makes sense to me.

> I just want to place an assertion at start of functions like this,
> which will be easily recognizable by coccinelle.

That's not a convincing argument.  Doesn't matter as long as we have
convincing ones :)

>
> ---
>
> We can improve the API, to support local_err==NULL, for the case when original request was called with
> errp==NULL, but for this we'll need more changes, like, pass errp to NBD_FOREACH_REPLY_CHUNK and save
> it into iter object...
>
> But how to detect it in code? Something like
>
>
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -1059,8 +1059,10 @@ static int nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>           case NBD_REPLY_TYPE_BLOCK_STATUS:
>               if (received) {
>                   nbd_channel_error(s, -EINVAL);
> -                error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
> -                nbd_iter_channel_error(&iter, -EINVAL, &local_err);
> +                if (errp) {
> +                    error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
> +                }
> +                nbd_iter_channel_error(&iter, -EINVAL, errp ? &local_err : NULL);
>               }
>               received = true;
>
>
> is ugly..
>
>
> So, to support original errp=NULL the whole thing should be refactored.. Not worth it, I think.

The only change I'd consider in addition to the assertion is this
simplification:

diff --git a/block/nbd.c b/block/nbd.c
index 5f18f78a94..7bbac1e5b8 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -870,11 +870,9 @@ static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
 
     if (!iter->ret) {
         iter->ret = ret;
-        error_propagate(&iter->err, *local_err);
-    } else {
-        error_free(*local_err);
     }
 
+    error_propagate(&iter->err, *local_err);
     *local_err = NULL;
 }
 

Re: [PATCH v7 21/21] nbd: assert that Error** is not NULL in nbd_iter_channel_error

[Vladimir Sementsov-Ogievskiy]

06.12.2019 11:54, Markus Armbruster wrote:
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
> 
>> 05.12.2019 20:14, Eric Blake wrote:
>>> On 12/5/19 9:20 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> The local_err parameter is not here to return information about
>>>> nbd_iter_channel_error failure. Instead it's assumed to be filled when
>>>> passed to the function. This is already stressed by its name
>>>> (local_err, instead of classic errp). Stress it additionally by
>>>> assertion.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    block/nbd.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/block/nbd.c b/block/nbd.c
>>>> index 5f18f78a94..d085554f21 100644
>>>> --- a/block/nbd.c
>>>> +++ b/block/nbd.c
>>>> @@ -866,6 +866,7 @@ typedef struct NBDReplyChunkIter {
>>>>    static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
>>>>                                       int ret, Error **local_err)
>>>>    {
>>>> +    assert(local_err && *local_err);
>>>
>>> Why are we forbidding grandparent callers from passing NULL when they want to ignore an error?  We are called by several parent functions that get an errp from the grandparent, and use this function to do some common grunt work.  Let's look at the possibilities:
>>>
>>> 1. grandparent passes address of a local error: we want to append to the error message, parent will then deal with the error how it wants (report it, ignore it, propagate it, whatever)
>>>
>>> 2. grandparent passes &error_fatal: we want to append a hint, but before ERRP_AUTO_PROPAGATE, the parent has already exited.  After ERRP_AUTO_PROPAGATE, this looks like case 1.
>>>
>>> 3. grandparent passes &error_abort: we should never be reached (failure earlier in the parent should have already aborted) - true whether or not ERRP_AUTO_PROPAGATE is applied
>>>
>>> 4. grandparent passes NULL to ignore the error. Does not currently happen in any of the grandparent callers, because if it did, your assertion in this patch would now fire.  After ERRP_AUTO_PROPAGATE, this would look like case 1.
>>>
>>> Would it be better to assert(!local_err || *local_err)?  The assertion as written is too strict without ERRP_AUTO_PROPAGATE, but you get away with it because none of the grandparents pass NULL; but is appropriate as written for after after the macro conversion so then we wonder if churn on the macro is worth it.
>>
>> We don't have any grandparents, this function is always called on local_err. And it's argument named local_err to stress it.
>> The function is an API to report error, and it wants filled local_err object.
>>
>> It will crash anyway if local_err is NULL, as it dereferences it.
> 
> Yes.
> 
> We already assert ret < 0 explicitly, and we rely on !local_err
> implicitly.  Making that explicit is obviously safe.
> 
> The code doesn't actually rely on !*local_err.  But when ret < 0, and
> !local_err, surely local_err should point to an Error object.  Asserting
> that makes sense to me.
> 
>> I just want to place an assertion at start of functions like this,
>> which will be easily recognizable by coccinelle.
> 
> That's not a convincing argument. 

That's why it is absent in commit message)

> Doesn't matter as long as we have
> convincing ones :)
> 
>>
>> ---
>>
>> We can improve the API, to support local_err==NULL, for the case when original request was called with
>> errp==NULL, but for this we'll need more changes, like, pass errp to NBD_FOREACH_REPLY_CHUNK and save
>> it into iter object...
>>
>> But how to detect it in code? Something like
>>
>>
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -1059,8 +1059,10 @@ static int nbd_co_receive_blockstatus_reply(BDRVNBDState *s,
>>            case NBD_REPLY_TYPE_BLOCK_STATUS:
>>                if (received) {
>>                    nbd_channel_error(s, -EINVAL);
>> -                error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
>> -                nbd_iter_channel_error(&iter, -EINVAL, &local_err);
>> +                if (errp) {
>> +                    error_setg(&local_err, "Several BLOCK_STATUS chunks in reply");
>> +                }
>> +                nbd_iter_channel_error(&iter, -EINVAL, errp ? &local_err : NULL);
>>                }
>>                received = true;
>>
>>
>> is ugly..
>>
>>
>> So, to support original errp=NULL the whole thing should be refactored.. Not worth it, I think.
> 
> The only change I'd consider in addition to the assertion is this
> simplification:
> 
> diff --git a/block/nbd.c b/block/nbd.c
> index 5f18f78a94..7bbac1e5b8 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -870,11 +870,9 @@ static void nbd_iter_channel_error(NBDReplyChunkIter *iter,
>   
>       if (!iter->ret) {
>           iter->ret = ret;
> -        error_propagate(&iter->err, *local_err);
> -    } else {
> -        error_free(*local_err);
>       }
>   
> +    error_propagate(&iter->err, *local_err);

because it will just free the second argument if the first one already set..
OK, will add this.

>       *local_err = NULL;
>   }
>   
> 


-- 
Best regards,
Vladimir

Re: [PATCH v7 00/21] error: prepare for auto propagated local_err

[Cornelia Huck]

On Thu,  5 Dec 2019 18:19:58 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> Hi all!
> 
> This is the first part of the bit series, which contains mostly simple
> cleanups.

What's the plan? Should subsystem maintainers pick up individual
patches, or will they be merged in one go?

Re: [PATCH v7 00/21] error: prepare for auto propagated local_err

[Vladimir Sementsov-Ogievskiy]

05.12.2019 18:26, Cornelia Huck wrote:
> On Thu,  5 Dec 2019 18:19:58 +0300
> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
> 
>> Hi all!
>>
>> This is the first part of the bit series, which contains mostly simple
>> cleanups.
> 
> What's the plan? Should subsystem maintainers pick up individual
> patches, or will they be merged in one go?
> 
The latter. Markus will take them all together.

-- 
Best regards,
Vladimir

Re: [PATCH v7 00/21] error: prepare for auto propagated local_err

[Markus Armbruster]

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

> 05.12.2019 18:26, Cornelia Huck wrote:
>> On Thu,  5 Dec 2019 18:19:58 +0300
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:
>> 
>>> Hi all!
>>>
>>> This is the first part of the bit series, which contains mostly simple
>>> cleanups.
>> 
>> What's the plan? Should subsystem maintainers pick up individual
>> patches, or will they be merged in one go?
>> 
> The latter. Markus will take them all together.

Subsystem maintainers are welcome to take truly independent patches
through their trees if that's more convenient for them, say because it
avoids or reduces the likelihood of conflicts.

Whatever is left I will take through my tree.  Me taking everything is
slightly easier for me, and results in a more coherent git-log.  Neither
is serious enough to justify inconveniencing subsystem maintainers.