All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/16] Various error handling fixes and cleanups
@ 2021-07-20 12:53 Markus Armbruster
  2021-07-20 12:53 ` [PATCH 01/16] error: Use error_fatal to simplify obvious fatal errors (again) Markus Armbruster
                   ` (17 more replies)
  0 siblings, 18 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Michael S . Tsirkin, Peter Xu,
	Kirti Wankhede, Gerd Hoffmann, Reinoud Zandijk,
	Jagannathan Raman, Juan Quintela, Kamil Rytarowski,
	Ganesh Goudar, Marc-André Lureau, Aravinda Prasad,
	Thomas Huth, Eduardo Habkost, Sergio Lopez,
	Dr . David Alan Gilbert, Alex Williamson, Stefan Hajnoczi,
	Cornelia Huck, Sunil Muthuswamy, David Gibson, Kevin Wolf,
	Daniel P . Berrangé,
	Marcelo Tosatti, Paolo Bonzini

I doubt the fixes are 6.1 material at this late stage.  If you
disagree, let me know.

Cc: Alex Williamson <alex.williamson@redhat.com>
Cc: Aravinda Prasad <arawinda.p@gmail.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Cc: Ganesh Goudar <ganeshgr@linux.ibm.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Jagannathan Raman <jag.raman@oracle.com>
Cc: John G Johnson <john.g.johnson@oracle.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Kamil Rytarowski <kamil@netbsd.org>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Reinoud Zandijk <reinoud@netbsd.org>
Cc: Sergio Lopez <slp@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Sunil Muthuswamy <sunilmut@microsoft.com>
Cc: Thomas Huth <thuth@redhat.com>

Markus Armbruster (16):
  error: Use error_fatal to simplify obvious fatal errors (again)
  spapr: Plug memory leak when we can't add a migration blocker
  spapr: Explain purpose of ->fwnmi_migration_blocker more clearly
  multi-process: Fix pci_proxy_dev_realize() error handling
  vhost-scsi: Plug memory leak on migrate_add_blocker() failure
  i386: Never free migration blocker objects instead of sometimes
  vfio: Avoid error_propagate() after migrate_add_blocker()
  whpx nvmm: Drop useless migrate_del_blocker()
  migration: Unify failure check for migrate_add_blocker()
  migration: Handle migration_incoming_setup() errors consistently
  microvm: Drop dead error handling in microvm_machine_state_init()
  vhost: Clean up how VhostOpts method vhost_get_config() fails
  vhost: Clean up how VhostOpts method vhost_backend_init() fails
  Remove superfluous ERRP_GUARD()
  vl: Clean up -smp error handling
  vl: Don't continue after -smp help.

 backends/tpm/tpm_emulator.c |  3 +--
 hw/display/qxl.c            |  8 ++++----
 hw/i386/microvm.c           |  5 -----
 hw/ppc/spapr_events.c       | 20 ++++++++++----------
 hw/remote/mpqemu-link.c     |  3 ---
 hw/remote/proxy.c           | 10 +++++++++-
 hw/s390x/ipl.c              |  6 +-----
 hw/scsi/vhost-scsi.c        |  4 ++--
 hw/vfio/migration.c         |  6 ++----
 hw/virtio/vhost-user.c      |  8 ++++++++
 hw/virtio/vhost.c           | 16 +++-------------
 migration/migration.c       | 34 ++++++++++------------------------
 qemu-img.c                  |  6 +-----
 qemu-io.c                   |  6 +-----
 qemu-nbd.c                  |  5 +----
 qga/commands-posix-ssh.c    | 17 -----------------
 qga/commands-win32.c        |  1 -
 scsi/qemu-pr-helper.c       | 11 +++--------
 softmmu/vl.c                | 19 +++++++------------
 target/i386/kvm/kvm.c       |  9 +++------
 target/i386/nvmm/nvmm-all.c |  4 +---
 target/i386/sev.c           |  8 +-------
 target/i386/whpx/whpx-all.c |  4 +---
 ui/console.c                |  7 ++-----
 ui/spice-core.c             |  7 +------
 25 files changed, 72 insertions(+), 155 deletions(-)

-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 01/16] error: Use error_fatal to simplify obvious fatal errors (again)
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
@ 2021-07-20 12:53 ` Markus Armbruster
  2021-07-20 13:42   ` Eric Blake
                     ` (2 more replies)
  2021-07-20 12:53 ` [PATCH 02/16] spapr: Plug memory leak when we can't add a migration blocker Markus Armbruster
                   ` (16 subsequent siblings)
  17 siblings, 3 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Thomas Huth, Daniel P . Berrangé,
	Juan Quintela, Peter Xu, Gerd Hoffmann, Stefan Hajnoczi,
	Marc-André Lureau, Cornelia Huck, Paolo Bonzini

We did this with scripts/coccinelle/use-error_fatal.cocci before, in
commit 50beeb68094 and 007b06578ab.  This commit cleans up rarer
variations that don't seem worth matching with Coccinelle.

Cc: Thomas Huth <thuth@redhat.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Peter Xu <peterx@redhat.com>
Cc: Juan Quintela <quintela@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/s390x/ipl.c        |  6 +-----
 migration/migration.c |  7 +------
 qemu-img.c            |  6 +-----
 qemu-io.c             |  6 +-----
 qemu-nbd.c            |  5 +----
 scsi/qemu-pr-helper.c | 11 +++--------
 softmmu/vl.c          |  7 +------
 target/i386/sev.c     |  8 +-------
 ui/console.c          |  6 ++----
 ui/spice-core.c       |  7 +------
 10 files changed, 13 insertions(+), 56 deletions(-)

diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
index 8c863cf386..1821c6faee 100644
--- a/hw/s390x/ipl.c
+++ b/hw/s390x/ipl.c
@@ -711,7 +711,6 @@ int s390_ipl_pv_unpack(void)
 void s390_ipl_prepare_cpu(S390CPU *cpu)
 {
     S390IPLState *ipl = get_ipl_device();
-    Error *err = NULL;
 
     cpu->env.psw.addr = ipl->start_addr;
     cpu->env.psw.mask = IPL_PSW_MASK;
@@ -723,10 +722,7 @@ void s390_ipl_prepare_cpu(S390CPU *cpu)
         }
     }
     if (ipl->netboot) {
-        if (load_netboot_image(&err) < 0) {
-            error_report_err(err);
-            exit(1);
-        }
+        load_netboot_image(&error_fatal);
         ipl->qipl.netboot_start_addr = cpu_to_be64(ipl->start_addr);
     }
     s390_ipl_set_boot_menu(ipl);
diff --git a/migration/migration.c b/migration/migration.c
index 2d306582eb..231dc24414 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -187,8 +187,6 @@ static gint page_request_addr_cmp(gconstpointer ap, gconstpointer bp)
 
 void migration_object_init(void)
 {
-    Error *err = NULL;
-
     /* This can only be called once. */
     assert(!current_migration);
     current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
@@ -209,10 +207,7 @@ void migration_object_init(void)
     qemu_mutex_init(&current_incoming->page_request_mutex);
     current_incoming->page_requested = g_tree_new(page_request_addr_cmp);
 
-    if (!migration_object_check(current_migration, &err)) {
-        error_report_err(err);
-        exit(1);
-    }
+    migration_object_check(current_migration, &error_fatal);
 
     blk_mig_init();
     ram_mig_init();
diff --git a/qemu-img.c b/qemu-img.c
index 797742a443..e0b438182e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -5310,7 +5310,6 @@ int main(int argc, char **argv)
 {
     const img_cmd_t *cmd;
     const char *cmdname;
-    Error *local_error = NULL;
     int c;
     static const struct option long_options[] = {
         {"help", no_argument, 0, 'h'},
@@ -5328,10 +5327,7 @@ int main(int argc, char **argv)
     module_call_init(MODULE_INIT_TRACE);
     qemu_init_exec_dir(argv[0]);
 
-    if (qemu_init_main_loop(&local_error)) {
-        error_report_err(local_error);
-        exit(EXIT_FAILURE);
-    }
+    qemu_init_main_loop(&error_fatal);
 
     qcrypto_init(&error_fatal);
 
diff --git a/qemu-io.c b/qemu-io.c
index 57f07501df..3924639b92 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -529,7 +529,6 @@ int main(int argc, char **argv)
     int flags = BDRV_O_UNMAP;
     int ret;
     bool writethrough = true;
-    Error *local_error = NULL;
     QDict *opts = NULL;
     const char *format = NULL;
     bool force_share = false;
@@ -629,10 +628,7 @@ int main(int argc, char **argv)
         exit(1);
     }
 
-    if (qemu_init_main_loop(&local_error)) {
-        error_report_err(local_error);
-        exit(1);
-    }
+    qemu_init_main_loop(&error_fatal);
 
     if (!trace_init_backends()) {
         exit(1);
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 26ffbf15af..65ebec598f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -963,10 +963,7 @@ int main(int argc, char **argv)
         }
     }
 
-    if (qemu_init_main_loop(&local_err)) {
-        error_report_err(local_err);
-        exit(EXIT_FAILURE);
-    }
+    qemu_init_main_loop(&error_fatal);
     bdrv_init();
     atexit(qemu_nbd_shutdown);
 
diff --git a/scsi/qemu-pr-helper.c b/scsi/qemu-pr-helper.c
index 7b9389b47b..f281daeced 100644
--- a/scsi/qemu-pr-helper.c
+++ b/scsi/qemu-pr-helper.c
@@ -1044,10 +1044,7 @@ int main(int argc, char **argv)
         }
     }
 
-    if (qemu_init_main_loop(&local_err)) {
-        error_report_err(local_err);
-        exit(EXIT_FAILURE);
-    }
+    qemu_init_main_loop(&error_fatal);
 
     server_watch = qio_channel_add_watch(QIO_CHANNEL(server_ioc),
                                          G_IO_IN,
@@ -1061,10 +1058,8 @@ int main(int argc, char **argv)
         }
     }
 
-    if ((daemonize || pidfile_specified) &&
-        !qemu_write_pidfile(pidfile, &local_err)) {
-        error_report_err(local_err);
-        exit(EXIT_FAILURE);
+    if (daemonize || pidfile_specified) {
+        qemu_write_pidfile(pidfile, &error_fatal);
     }
 
 #ifdef CONFIG_LIBCAP_NG
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 4df1496101..0d2db1abc3 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -2673,12 +2673,7 @@ void qmp_x_exit_preconfig(Error **errp)
     qemu_machine_creation_done();
 
     if (loadvm) {
-        Error *local_err = NULL;
-        if (!load_snapshot(loadvm, NULL, false, NULL, &local_err)) {
-            error_report_err(local_err);
-            autostart = 0;
-            exit(1);
-        }
+        load_snapshot(loadvm, NULL, false, NULL, &error_fatal);
     }
     if (replay_mode != REPLAY_MODE_NONE) {
         replay_vmstate_init();
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 83df8c09f6..0b2c8f594a 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -737,7 +737,6 @@ static void
 sev_launch_finish(SevGuestState *sev)
 {
     int ret, error;
-    Error *local_err = NULL;
 
     trace_kvm_sev_launch_finish();
     ret = sev_ioctl(sev->sev_fd, KVM_SEV_LAUNCH_FINISH, 0, &error);
@@ -752,12 +751,7 @@ sev_launch_finish(SevGuestState *sev)
     /* add migration blocker */
     error_setg(&sev_mig_blocker,
                "SEV: Migration is not implemented");
-    ret = migrate_add_blocker(sev_mig_blocker, &local_err);
-    if (local_err) {
-        error_report_err(local_err);
-        error_free(sev_mig_blocker);
-        exit(1);
-    }
+    migrate_add_blocker(sev_mig_blocker, &error_fatal);
 }
 
 static void
diff --git a/ui/console.c b/ui/console.c
index 1103b65314..5d2e6178ff 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1508,7 +1508,6 @@ void register_displaychangelistener(DisplayChangeListener *dcl)
         "This VM has no graphic display device.";
     static DisplaySurface *dummy;
     QemuConsole *con;
-    Error *err = NULL;
 
     assert(!dcl->ds);
 
@@ -1523,9 +1522,8 @@ void register_displaychangelistener(DisplayChangeListener *dcl)
         dcl->con->gl = dcl;
     }
 
-    if (dcl->con && !dpy_compatible_with(dcl->con, dcl, &err)) {
-        error_report_err(err);
-        exit(1);
+    if (dcl->con) {
+        dpy_compatible_with(dcl->con, dcl, &error_fatal);
     }
 
     trace_displaychangelistener_register(dcl, dcl->ops->dpy_name);
diff --git a/ui/spice-core.c b/ui/spice-core.c
index 86d43783ac..bbd7ad070b 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -671,18 +671,13 @@ static void qemu_spice_init(void)
     }
     passwordSecret = qemu_opt_get(opts, "password-secret");
     if (passwordSecret) {
-        Error *local_err = NULL;
         if (qemu_opt_get(opts, "password")) {
             error_report("'password' option is mutually exclusive with "
                          "'password-secret'");
             exit(1);
         }
         password = qcrypto_secret_lookup_as_utf8(passwordSecret,
-                                                 &local_err);
-        if (!password) {
-            error_report_err(local_err);
-            exit(1);
-        }
+                                                 &error_fatal);
     } else {
         str = qemu_opt_get(opts, "password");
         if (str) {
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 02/16] spapr: Plug memory leak when we can't add a migration blocker
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
  2021-07-20 12:53 ` [PATCH 01/16] error: Use error_fatal to simplify obvious fatal errors (again) Markus Armbruster
@ 2021-07-20 12:53 ` Markus Armbruster
  2021-07-20 13:12   ` David Gibson
  2021-08-02 18:54   ` Philippe Mathieu-Daudé
  2021-07-20 12:53 ` [PATCH 03/16] spapr: Explain purpose of ->fwnmi_migration_blocker more clearly Markus Armbruster
                   ` (15 subsequent siblings)
  17 siblings, 2 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:53 UTC (permalink / raw)
  To: qemu-devel; +Cc: Aravinda Prasad, Ganesh Goudar, David Gibson

Fixes: 2500fb423adb17995485de0b4d507cf2f09e3a7f
Cc: Aravinda Prasad <arawinda.p@gmail.com>
Cc: Ganesh Goudar <ganeshgr@linux.ibm.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ppc/spapr_events.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index 0cfc19be19..a8f2cc6bdc 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -872,7 +872,6 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
     SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     CPUState *cs = CPU(cpu);
     int ret;
-    Error *local_err = NULL;
 
     if (spapr->fwnmi_machine_check_addr == -1) {
         /* Non-FWNMI case, deliver it like an architected CPU interrupt. */
@@ -912,7 +911,7 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
         }
     }
 
-    ret = migrate_add_blocker(spapr->fwnmi_migration_blocker, &local_err);
+    ret = migrate_add_blocker(spapr->fwnmi_migration_blocker, NULL);
     if (ret == -EBUSY) {
         /*
          * We don't want to abort so we let the migration to continue.
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 03/16] spapr: Explain purpose of ->fwnmi_migration_blocker more clearly
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
  2021-07-20 12:53 ` [PATCH 01/16] error: Use error_fatal to simplify obvious fatal errors (again) Markus Armbruster
  2021-07-20 12:53 ` [PATCH 02/16] spapr: Plug memory leak when we can't add a migration blocker Markus Armbruster
@ 2021-07-20 12:53 ` Markus Armbruster
  2021-07-21  6:08   ` David Gibson
  2021-07-20 12:53 ` [PATCH 04/16] multi-process: Fix pci_proxy_dev_realize() error handling Markus Armbruster
                   ` (14 subsequent siblings)
  17 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Aravinda Prasad, Ganesh Goudar, Dr . David Alan Gilbert, David Gibson

spapr_mce_req_event() makes an effort to prevent migration from
degrading the reporting of FWNMIs.  It adds a migration blocker when
it receives one, and deletes it when it's done handling it.  This is a
best effort.

Commit 2500fb423a "migration: Include migration support for machine
check handling" tried to explain this in a comment.  Rewrite the
comment for clarity, and reposition it to make it clear it applies to
all failure modes, not just "migration already in progress".

Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: Aravinda Prasad <arawinda.p@gmail.com>
Cc: Ganesh Goudar <ganeshgr@linux.ibm.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/ppc/spapr_events.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
index a8f2cc6bdc..7d6876f12d 100644
--- a/hw/ppc/spapr_events.c
+++ b/hw/ppc/spapr_events.c
@@ -911,16 +911,17 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
         }
     }
 
+    /*
+     * Try to block migration while FWNMI is being handled, so the
+     * machine check handler runs where the information passed to it
+     * actually makes sense.  This shouldn't actually block migration,
+     * only delay it slightly, assuming migration is retried.  If the
+     * attempt to block fails, carry on.  Unfortunately, it always
+     * fails when running with -only-migrate.  A proper interface to
+     * delay migration completion for a bit could avoid that.
+     */
     ret = migrate_add_blocker(spapr->fwnmi_migration_blocker, NULL);
     if (ret == -EBUSY) {
-        /*
-         * We don't want to abort so we let the migration to continue.
-         * In a rare case, the machine check handler will run on the target.
-         * Though this is not preferable, it is better than aborting
-         * the migration or killing the VM. It is okay to call
-         * migrate_del_blocker on a blocker that was not added (which the
-         * nmi-interlock handler would do when it's called after this).
-         */
         warn_report("Received a fwnmi while migration was in progress");
     }
 
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 04/16] multi-process: Fix pci_proxy_dev_realize() error handling
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (2 preceding siblings ...)
  2021-07-20 12:53 ` [PATCH 03/16] spapr: Explain purpose of ->fwnmi_migration_blocker more clearly Markus Armbruster
@ 2021-07-20 12:53 ` Markus Armbruster
  2021-07-20 18:02   ` Philippe Mathieu-Daudé
  2021-07-20 20:02   ` Jag Raman
  2021-07-20 12:53 ` [PATCH 05/16] vhost-scsi: Plug memory leak on migrate_add_blocker() failure Markus Armbruster
                   ` (13 subsequent siblings)
  17 siblings, 2 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Jagannathan Raman, Stefan Hajnoczi

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

pci_proxy_dev_realize() is wrong that way: it passes @errp to
qio_channel_new_fd() without checking for failure.  If it runs into
another failure, it trips error_setv()'s assertion.

Fix it to check for failure properly.

Fixes: 9f8112073aad8e485ac012ee18809457ab7f23a6
Cc: Elena Ufimtseva <elena.ufimtseva@oracle.com>
Cc: Jagannathan Raman <jag.raman@oracle.com>
Cc: John G Johnson <john.g.johnson@oracle.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/remote/proxy.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/hw/remote/proxy.c b/hw/remote/proxy.c
index 6dda705fc2..499f540c94 100644
--- a/hw/remote/proxy.c
+++ b/hw/remote/proxy.c
@@ -102,10 +102,18 @@ static void pci_proxy_dev_realize(PCIDevice *device, Error **errp)
     }
 
     dev->ioc = qio_channel_new_fd(fd, errp);
+    if (!dev->ioc) {
+        close(fd);
+        return;
+    }
 
     error_setg(&dev->migration_blocker, "%s does not support migration",
                TYPE_PCI_PROXY_DEV);
-    migrate_add_blocker(dev->migration_blocker, errp);
+    if (migrate_add_blocker(dev->migration_blocker, errp) < 0) {
+        error_free(dev->migration_blocker);
+        object_unref(dev->ioc);
+        return;
+    }
 
     qemu_mutex_init(&dev->io_mutex);
     qio_channel_set_blocking(dev->ioc, true, NULL);
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 05/16] vhost-scsi: Plug memory leak on migrate_add_blocker() failure
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (3 preceding siblings ...)
  2021-07-20 12:53 ` [PATCH 04/16] multi-process: Fix pci_proxy_dev_realize() error handling Markus Armbruster
@ 2021-07-20 12:53 ` Markus Armbruster
  2021-07-20 12:53 ` [PATCH 06/16] i386: Never free migration blocker objects instead of sometimes Markus Armbruster
                   ` (12 subsequent siblings)
  17 siblings, 0 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:53 UTC (permalink / raw)
  To: qemu-devel; +Cc: Michael S . Tsirkin

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/scsi/vhost-scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c
index 8c611bfd2d..039caf2614 100644
--- a/hw/scsi/vhost-scsi.c
+++ b/hw/scsi/vhost-scsi.c
@@ -208,7 +208,6 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
                 "target SCSI device state or use shared storage over network), "
                 "set 'migratable' property to true to enable migration.");
         if (migrate_add_blocker(vsc->migration_blocker, errp) < 0) {
-            error_free(vsc->migration_blocker);
             goto free_virtio;
         }
     }
@@ -233,11 +232,12 @@ static void vhost_scsi_realize(DeviceState *dev, Error **errp)
     return;
 
  free_vqs:
+    g_free(vsc->dev.vqs);
     if (!vsc->migratable) {
         migrate_del_blocker(vsc->migration_blocker);
     }
-    g_free(vsc->dev.vqs);
  free_virtio:
+    error_free(vsc->migration_blocker);
     virtio_scsi_common_unrealize(dev);
  close_fd:
     close(vhostfd);
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 06/16] i386: Never free migration blocker objects instead of sometimes
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (4 preceding siblings ...)
  2021-07-20 12:53 ` [PATCH 05/16] vhost-scsi: Plug memory leak on migrate_add_blocker() failure Markus Armbruster
@ 2021-07-20 12:53 ` Markus Armbruster
  2021-07-20 12:53 ` [PATCH 07/16] vfio: Avoid error_propagate() after migrate_add_blocker() Markus Armbruster
                   ` (11 subsequent siblings)
  17 siblings, 0 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:53 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini, Marcelo Tosatti, Eduardo Habkost

invtsc_mig_blocker has static storage duration.  When a CPU with
certain features is initialized, and invtsc_mig_blocker is still null,
we add a migration blocker and store it in invtsc_mig_blocker.

The object is freed when migrate_add_blocker() fails, leaving
invtsc_mig_blocker dangling.  It is not freed on later failures.

Same for hv_passthrough_mig_blocker and hv_no_nonarch_cs_mig_blocker.

All failures are actually fatal, so whether we free or not doesn't
really matter, except as bad examples to be copied / imitated.

Clean this up in a minimal way: never free these blocker objects.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 target/i386/kvm/kvm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 59ed8327ac..8e1bb905ca 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1423,7 +1423,6 @@ static int hyperv_init_vcpu(X86CPU *cpu)
         ret = migrate_add_blocker(hv_passthrough_mig_blocker, &local_err);
         if (local_err) {
             error_report_err(local_err);
-            error_free(hv_passthrough_mig_blocker);
             return ret;
         }
     }
@@ -1438,7 +1437,6 @@ static int hyperv_init_vcpu(X86CPU *cpu)
         ret = migrate_add_blocker(hv_no_nonarch_cs_mig_blocker, &local_err);
         if (local_err) {
             error_report_err(local_err);
-            error_free(hv_no_nonarch_cs_mig_blocker);
             return ret;
         }
     }
@@ -1878,7 +1876,6 @@ int kvm_arch_init_vcpu(CPUState *cs)
             r = migrate_add_blocker(invtsc_mig_blocker, &local_err);
             if (local_err) {
                 error_report_err(local_err);
-                error_free(invtsc_mig_blocker);
                 return r;
             }
         }
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 07/16] vfio: Avoid error_propagate() after migrate_add_blocker()
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (5 preceding siblings ...)
  2021-07-20 12:53 ` [PATCH 06/16] i386: Never free migration blocker objects instead of sometimes Markus Armbruster
@ 2021-07-20 12:53 ` Markus Armbruster
  2021-07-20 18:03   ` Philippe Mathieu-Daudé
                     ` (2 more replies)
  2021-07-20 12:54 ` [PATCH 08/16] whpx nvmm: Drop useless migrate_del_blocker() Markus Armbruster
                   ` (10 subsequent siblings)
  17 siblings, 3 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:53 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kirti Wankhede, Alex Williamson

When migrate_add_blocker(blocker, &errp) is followed by
error_propagate(errp, err), we can often just as well do
migrate_add_blocker(..., errp).  This is the case in
vfio_migration_probe().

Prior art: commit 386f6c07d2 "error: Avoid error_propagate() after
migrate_add_blocker()".

Cc: Kirti Wankhede <kwankhede@nvidia.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/vfio/migration.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 82f654afb6..ff6b45de6b 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -858,7 +858,6 @@ int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
 {
     VFIOContainer *container = vbasedev->group->container;
     struct vfio_region_info *info = NULL;
-    Error *local_err = NULL;
     int ret = -ENOTSUP;
 
     if (!vbasedev->enable_migration || !container->dirty_pages_supported) {
@@ -885,9 +884,8 @@ add_blocker:
                "VFIO device doesn't support migration");
     g_free(info);
 
-    ret = migrate_add_blocker(vbasedev->migration_blocker, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
+    ret = migrate_add_blocker(vbasedev->migration_blocker, errp);
+    if (ret < 0) {
         error_free(vbasedev->migration_blocker);
         vbasedev->migration_blocker = NULL;
     }
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 08/16] whpx nvmm: Drop useless migrate_del_blocker()
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (6 preceding siblings ...)
  2021-07-20 12:53 ` [PATCH 07/16] vfio: Avoid error_propagate() after migrate_add_blocker() Markus Armbruster
@ 2021-07-20 12:54 ` Markus Armbruster
  2021-07-22  8:04   ` Reinoud Zandijk
  2021-07-20 12:54 ` [PATCH 09/16] migration: Unify failure check for migrate_add_blocker() Markus Armbruster
                   ` (9 subsequent siblings)
  17 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:54 UTC (permalink / raw)
  To: qemu-devel; +Cc: Sunil Muthuswamy, Reinoud Zandijk, Kamil Rytarowski

There is nothing to delete after migrate_add_blocker() failed.  Trying
anyway is safe, but useless.  Don't.

Cc: Sunil Muthuswamy <sunilmut@microsoft.com>
Cc: Kamil Rytarowski <kamil@netbsd.org>
Cc: Reinoud Zandijk <reinoud@netbsd.org>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 target/i386/nvmm/nvmm-all.c | 1 -
 target/i386/whpx/whpx-all.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
index dfa690d65d..7bb0d9e30e 100644
--- a/target/i386/nvmm/nvmm-all.c
+++ b/target/i386/nvmm/nvmm-all.c
@@ -929,7 +929,6 @@ nvmm_init_vcpu(CPUState *cpu)
         (void)migrate_add_blocker(nvmm_migration_blocker, &local_error);
         if (local_error) {
             error_report_err(local_error);
-            migrate_del_blocker(nvmm_migration_blocker);
             error_free(nvmm_migration_blocker);
             return -EINVAL;
         }
diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
index f832f286ac..cc8c0b984b 100644
--- a/target/i386/whpx/whpx-all.c
+++ b/target/i386/whpx/whpx-all.c
@@ -1349,7 +1349,6 @@ int whpx_init_vcpu(CPUState *cpu)
         (void)migrate_add_blocker(whpx_migration_blocker, &local_error);
         if (local_error) {
             error_report_err(local_error);
-            migrate_del_blocker(whpx_migration_blocker);
             error_free(whpx_migration_blocker);
             ret = -EINVAL;
             goto error;
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 09/16] migration: Unify failure check for migrate_add_blocker()
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (7 preceding siblings ...)
  2021-07-20 12:54 ` [PATCH 08/16] whpx nvmm: Drop useless migrate_del_blocker() Markus Armbruster
@ 2021-07-20 12:54 ` Markus Armbruster
  2021-07-20 18:05   ` Philippe Mathieu-Daudé
  2021-07-20 12:54 ` [PATCH 10/16] migration: Handle migration_incoming_setup() errors consistently Markus Armbruster
                   ` (8 subsequent siblings)
  17 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:54 UTC (permalink / raw)
  To: qemu-devel

Most callers check the return value.  Some check whether it set an
error.  Functionally equivalent, but the former tends to be easier on
the eyes, so do that everywhere.

Prior art: commit c6ecec43b2 "qemu-option: Check return value instead
of @err where convenient".

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 backends/tpm/tpm_emulator.c | 3 +--
 hw/display/qxl.c            | 8 ++++----
 hw/virtio/vhost.c           | 2 +-
 target/i386/kvm/kvm.c       | 6 +++---
 target/i386/nvmm/nvmm-all.c | 3 +--
 target/i386/whpx/whpx-all.c | 3 +--
 6 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/backends/tpm/tpm_emulator.c b/backends/tpm/tpm_emulator.c
index e5f1063ab6..f8095d23d5 100644
--- a/backends/tpm/tpm_emulator.c
+++ b/backends/tpm/tpm_emulator.c
@@ -492,8 +492,7 @@ static int tpm_emulator_block_migration(TPMEmulator *tpm_emu)
         error_setg(&tpm_emu->migration_blocker,
                    "Migration disabled: TPM emulator does not support "
                    "migration");
-        migrate_add_blocker(tpm_emu->migration_blocker, &err);
-        if (err) {
+        if (migrate_add_blocker(tpm_emu->migration_blocker, &err) < 0) {
             error_report_err(err);
             error_free(tpm_emu->migration_blocker);
             tpm_emu->migration_blocker = NULL;
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 84f99088e0..436399db76 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -680,12 +680,12 @@ static int interface_get_command(QXLInstance *sin, struct QXLCommandExt *ext)
                     msg < (void *)qxl->vga.vram_ptr ||
                     msg > ((void *)qxl->vga.vram_ptr + qxl->vga.vram_size))) {
                 if (!qxl->migration_blocker) {
-                    Error *local_err = NULL;
+                    Error *err = NULL;
+
                     error_setg(&qxl->migration_blocker,
                                "qxl: guest bug: command not in ram bar");
-                    migrate_add_blocker(qxl->migration_blocker, &local_err);
-                    if (local_err) {
-                        error_report_err(local_err);
+                    if (migrate_add_blocker(qxl->migration_blocker, &err) < 0) {
+                        error_report_err(err);
                     }
                 }
             }
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index e8f85a5d2d..dbbc6b6915 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1372,7 +1372,7 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
 
     if (hdev->migration_blocker != NULL) {
         r = migrate_add_blocker(hdev->migration_blocker, errp);
-        if (*errp) {
+        if (r < 0) {
             error_free(hdev->migration_blocker);
             goto fail_busyloop;
         }
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 8e1bb905ca..bb72c233cc 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1421,7 +1421,7 @@ static int hyperv_init_vcpu(X86CPU *cpu)
                    "'hv-passthrough' CPU flag prevents migration, use explicit"
                    " set of hv-* flags instead");
         ret = migrate_add_blocker(hv_passthrough_mig_blocker, &local_err);
-        if (local_err) {
+        if (ret < 0) {
             error_report_err(local_err);
             return ret;
         }
@@ -1435,7 +1435,7 @@ static int hyperv_init_vcpu(X86CPU *cpu)
                    " make sure SMT is disabled and/or that vCPUs are properly"
                    " pinned)");
         ret = migrate_add_blocker(hv_no_nonarch_cs_mig_blocker, &local_err);
-        if (local_err) {
+        if (ret < 0) {
             error_report_err(local_err);
             return ret;
         }
@@ -1874,7 +1874,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
                        "State blocked by non-migratable CPU device"
                        " (invtsc flag)");
             r = migrate_add_blocker(invtsc_mig_blocker, &local_err);
-            if (local_err) {
+            if (r < 0) {
                 error_report_err(local_err);
                 return r;
             }
diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
index 7bb0d9e30e..28dee4c5ee 100644
--- a/target/i386/nvmm/nvmm-all.c
+++ b/target/i386/nvmm/nvmm-all.c
@@ -926,8 +926,7 @@ nvmm_init_vcpu(CPUState *cpu)
         error_setg(&nvmm_migration_blocker,
             "NVMM: Migration not supported");
 
-        (void)migrate_add_blocker(nvmm_migration_blocker, &local_error);
-        if (local_error) {
+        if (migrate_add_blocker(nvmm_migration_blocker, &local_error) < 0) {
             error_report_err(local_error);
             error_free(nvmm_migration_blocker);
             return -EINVAL;
diff --git a/target/i386/whpx/whpx-all.c b/target/i386/whpx/whpx-all.c
index cc8c0b984b..3e925b9da7 100644
--- a/target/i386/whpx/whpx-all.c
+++ b/target/i386/whpx/whpx-all.c
@@ -1346,8 +1346,7 @@ int whpx_init_vcpu(CPUState *cpu)
                "State blocked due to non-migratable CPUID feature support,"
                "dirty memory tracking support, and XSAVE/XRSTOR support");
 
-        (void)migrate_add_blocker(whpx_migration_blocker, &local_error);
-        if (local_error) {
+        if (migrate_add_blocker(whpx_migration_blocker, &local_error) < 0) {
             error_report_err(local_error);
             error_free(whpx_migration_blocker);
             ret = -EINVAL;
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 10/16] migration: Handle migration_incoming_setup() errors consistently
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (8 preceding siblings ...)
  2021-07-20 12:54 ` [PATCH 09/16] migration: Unify failure check for migrate_add_blocker() Markus Armbruster
@ 2021-07-20 12:54 ` Markus Armbruster
  2021-07-20 18:53   ` Eric Blake
                     ` (2 more replies)
  2021-07-20 12:54 ` [PATCH 11/16] microvm: Drop dead error handling in microvm_machine_state_init() Markus Armbruster
                   ` (7 subsequent siblings)
  17 siblings, 3 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:54 UTC (permalink / raw)
  To: qemu-devel; +Cc: Dr . David Alan Gilbert, Juan Quintela

Commit b673eab4e2 "multifd: Make multifd_load_setup() get an Error
parameter" changed migration_incoming_setup() to take an Error **
argument, and adjusted the callers accordingly.  It neglected to
change adjust multifd_load_setup(): it still exit()s on error.  Clean
that up.

The error now gets propagated up two call chains: via
migration_fd_process_incoming() to rdma_accept_incoming_migration(),
and via migration_ioc_process_incoming() to
migration_channel_process_incoming().  Both chain ends report the
error with error_report_err(), but otherwise ignore it.  Behavioral
change: we no longer exit() on this error.

This is consistent with how we handle other errors here, e.g. from
multifd_recv_new_channel() via migration_ioc_process_incoming() to
migration_channel_process_incoming().  Wether it's consistently right
or consistently wrong I can't tell.

Also clean up the return value from the unusual 0 on success, 1 on
error to the more common true on success, false on error.

Cc: Juan Quintela <quintela@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 migration/migration.c | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 231dc24414..c1c0a48647 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -609,30 +609,25 @@ fail:
 }
 
 /**
- * @migration_incoming_setup: Setup incoming migration
- *
- * Returns 0 for no error or 1 for error
- *
+ * migration_incoming_setup: Setup incoming migration
  * @f: file for main migration channel
  * @errp: where to put errors
+ *
+ * Returns: %true on success, %false on error.
  */
-static int migration_incoming_setup(QEMUFile *f, Error **errp)
+static bool migration_incoming_setup(QEMUFile *f, Error **errp)
 {
     MigrationIncomingState *mis = migration_incoming_get_current();
-    Error *local_err = NULL;
 
-    if (multifd_load_setup(&local_err) != 0) {
-        /* We haven't been able to create multifd threads
-           nothing better to do */
-        error_report_err(local_err);
-        exit(EXIT_FAILURE);
+    if (multifd_load_setup(errp) != 0) {
+        return false;
     }
 
     if (!mis->from_src_file) {
         mis->from_src_file = f;
     }
     qemu_file_set_blocking(f, false);
-    return 0;
+    return true;
 }
 
 void migration_incoming_process(void)
@@ -675,14 +670,11 @@ static bool postcopy_try_recover(QEMUFile *f)
 
 void migration_fd_process_incoming(QEMUFile *f, Error **errp)
 {
-    Error *local_err = NULL;
-
     if (postcopy_try_recover(f)) {
         return;
     }
 
-    if (migration_incoming_setup(f, &local_err)) {
-        error_propagate(errp, local_err);
+    if (!migration_incoming_setup(f, errp)) {
         return;
     }
     migration_incoming_process();
@@ -703,8 +695,7 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
             return;
         }
 
-        if (migration_incoming_setup(f, &local_err)) {
-            error_propagate(errp, local_err);
+        if (!migration_incoming_setup(f, errp)) {
             return;
         }
 
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 11/16] microvm: Drop dead error handling in microvm_machine_state_init()
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (9 preceding siblings ...)
  2021-07-20 12:54 ` [PATCH 10/16] migration: Handle migration_incoming_setup() errors consistently Markus Armbruster
@ 2021-07-20 12:54 ` Markus Armbruster
  2021-07-20 13:36   ` Sergio Lopez
                     ` (2 more replies)
  2021-07-20 12:54 ` [PATCH 12/16] vhost: Clean up how VhostOpts method vhost_get_config() fails Markus Armbruster
                   ` (6 subsequent siblings)
  17 siblings, 3 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:54 UTC (permalink / raw)
  To: qemu-devel; +Cc: Sergio Lopez

Stillborn in commit 0ebf007dda "hw/i386: Introduce the microvm machine
type".

Cc: Sergio Lopez <slp@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/i386/microvm.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index aba0c83219..f257ec5a0b 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -458,15 +458,10 @@ static void microvm_machine_state_init(MachineState *machine)
 {
     MicrovmMachineState *mms = MICROVM_MACHINE(machine);
     X86MachineState *x86ms = X86_MACHINE(machine);
-    Error *local_err = NULL;
 
     microvm_memory_init(mms);
 
     x86_cpus_init(x86ms, CPU_VERSION_LATEST);
-    if (local_err) {
-        error_report_err(local_err);
-        exit(1);
-    }
 
     microvm_devices_init(mms);
 }
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 12/16] vhost: Clean up how VhostOpts method vhost_get_config() fails
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (10 preceding siblings ...)
  2021-07-20 12:54 ` [PATCH 11/16] microvm: Drop dead error handling in microvm_machine_state_init() Markus Armbruster
@ 2021-07-20 12:54 ` Markus Armbruster
  2021-08-02 18:56   ` Philippe Mathieu-Daudé
  2021-08-02 18:58   ` Philippe Mathieu-Daudé
  2021-07-20 12:54 ` [PATCH 13/16] vhost: Clean up how VhostOpts method vhost_backend_init() fails Markus Armbruster
                   ` (5 subsequent siblings)
  17 siblings, 2 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:54 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Michael S . Tsirkin

vhost_user_get_config() can fail without setting an error.  Unclean.
Its caller vhost_dev_get_config() compensates by substituting a
generic error then.  Goes back to commit 50de51387f "vhost:
Distinguish errors in vhost_dev_get_config()".

Clean up by moving the generic error from vhost_dev_get_config() to
all the failure paths that neglect to set an error.

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/virtio/vhost-user.c |  2 ++
 hw/virtio/vhost.c      | 10 ++--------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 29ea2b4fce..dbbd6fbc25 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -2139,10 +2139,12 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
     msg.payload.config.offset = 0;
     msg.payload.config.size = config_len;
     if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
+        error_setg_errno(errp, -EPROTO, "vhost_get_config failed");
         return -EPROTO;
     }
 
     if (vhost_user_read(dev, &msg) < 0) {
+        error_setg_errno(errp, -EPROTO, "vhost_get_config failed");
         return -EPROTO;
     }
 
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index dbbc6b6915..88f8a397dc 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1564,17 +1564,11 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
 int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
                          uint32_t config_len, Error **errp)
 {
-    ERRP_GUARD();
-    int ret;
-
     assert(hdev->vhost_ops);
 
     if (hdev->vhost_ops->vhost_get_config) {
-        ret = hdev->vhost_ops->vhost_get_config(hdev, config, config_len, errp);
-        if (ret < 0 && !*errp) {
-            error_setg_errno(errp, -ret, "vhost_get_config failed");
-        }
-        return ret;
+        return hdev->vhost_ops->vhost_get_config(hdev, config, config_len,
+                                                 errp);
     }
 
     error_setg(errp, "vhost_get_config not implemented");
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 13/16] vhost: Clean up how VhostOpts method vhost_backend_init() fails
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (11 preceding siblings ...)
  2021-07-20 12:54 ` [PATCH 12/16] vhost: Clean up how VhostOpts method vhost_get_config() fails Markus Armbruster
@ 2021-07-20 12:54 ` Markus Armbruster
  2021-08-02 18:59   ` Philippe Mathieu-Daudé
  2021-07-20 12:54 ` [PATCH 14/16] Remove superfluous ERRP_GUARD() Markus Armbruster
                   ` (4 subsequent siblings)
  17 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:54 UTC (permalink / raw)
  To: qemu-devel; +Cc: Kevin Wolf, Michael S . Tsirkin

vhost_user_backend_init() can fail without setting an error.  Unclean.
Its caller vhost_dev_init() compensates by substituting a generic
error then.  Goes back to commit 28770ff935 "vhost: Distinguish errors
in vhost_backend_init()".

Clean up by moving the generic error from vhost_dev_init() to all the
failure paths that neglect to set an error.

Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/virtio/vhost-user.c | 6 ++++++
 hw/virtio/vhost.c      | 4 ----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index dbbd6fbc25..b4a4998088 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1876,6 +1876,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
 
     err = vhost_user_get_features(dev, &features);
     if (err < 0) {
+        error_setg_errno(errp, -err, "vhost_backend_init failed");
         return err;
     }
 
@@ -1885,6 +1886,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
         err = vhost_user_get_u64(dev, VHOST_USER_GET_PROTOCOL_FEATURES,
                                  &protocol_features);
         if (err < 0) {
+            error_setg_errno(errp, EPROTO, "vhost_backend_init failed");
             return -EPROTO;
         }
 
@@ -1903,6 +1905,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
 
         err = vhost_user_set_protocol_features(dev, dev->protocol_features);
         if (err < 0) {
+            error_setg_errno(errp, EPROTO, "vhost_backend_init failed");
             return -EPROTO;
         }
 
@@ -1911,6 +1914,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
             err = vhost_user_get_u64(dev, VHOST_USER_GET_QUEUE_NUM,
                                      &dev->max_queues);
             if (err < 0) {
+                error_setg_errno(errp, EPROTO, "vhost_backend_init failed");
                 return -EPROTO;
             }
         } else {
@@ -1940,6 +1944,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
         } else {
             err = vhost_user_get_max_memslots(dev, &ram_slots);
             if (err < 0) {
+                error_setg_errno(errp, EPROTO, "vhost_backend_init failed");
                 return -EPROTO;
             }
 
@@ -1966,6 +1971,7 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,
     if (dev->vq_index == 0) {
         err = vhost_setup_slave_channel(dev);
         if (err < 0) {
+            error_setg_errno(errp, EPROTO, "vhost_backend_init failed");
             return -EPROTO;
         }
     }
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 88f8a397dc..3c0b537f89 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1289,7 +1289,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
                    VhostBackendType backend_type, uint32_t busyloop_timeout,
                    Error **errp)
 {
-    ERRP_GUARD();
     uint64_t features;
     int i, r, n_initialized_vqs = 0;
 
@@ -1301,9 +1300,6 @@ int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
 
     r = hdev->vhost_ops->vhost_backend_init(hdev, opaque, errp);
     if (r < 0) {
-        if (!*errp) {
-            error_setg_errno(errp, -r, "vhost_backend_init failed");
-        }
         goto fail;
     }
 
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 14/16] Remove superfluous ERRP_GUARD()
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (12 preceding siblings ...)
  2021-07-20 12:54 ` [PATCH 13/16] vhost: Clean up how VhostOpts method vhost_backend_init() fails Markus Armbruster
@ 2021-07-20 12:54 ` Markus Armbruster
  2021-08-02 19:00   ` Philippe Mathieu-Daudé
  2021-07-20 12:54 ` [PATCH 15/16] vl: Clean up -smp error handling Markus Armbruster
                   ` (3 subsequent siblings)
  17 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:54 UTC (permalink / raw)
  To: qemu-devel

Macro ERRP_GUARD() is only needed when we want to dereference @errp or
pass it to error_prepend() or error_append_hint().  Delete superfluous
ones.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/remote/mpqemu-link.c  |  3 ---
 qga/commands-posix-ssh.c | 17 -----------------
 qga/commands-win32.c     |  1 -
 softmmu/vl.c             |  1 -
 ui/console.c             |  1 -
 5 files changed, 23 deletions(-)

diff --git a/hw/remote/mpqemu-link.c b/hw/remote/mpqemu-link.c
index e67a5de72c..7e841820e5 100644
--- a/hw/remote/mpqemu-link.c
+++ b/hw/remote/mpqemu-link.c
@@ -34,7 +34,6 @@
  */
 bool mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp)
 {
-    ERRP_GUARD();
     bool iolock = qemu_mutex_iothread_locked();
     bool iothread = qemu_in_iothread();
     struct iovec send[2] = {};
@@ -97,7 +96,6 @@ bool mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp)
 static ssize_t mpqemu_read(QIOChannel *ioc, void *buf, size_t len, int **fds,
                            size_t *nfds, Error **errp)
 {
-    ERRP_GUARD();
     struct iovec iov = { .iov_base = buf, .iov_len = len };
     bool iolock = qemu_mutex_iothread_locked();
     bool iothread = qemu_in_iothread();
@@ -192,7 +190,6 @@ fail:
 uint64_t mpqemu_msg_send_and_await_reply(MPQemuMsg *msg, PCIProxyDev *pdev,
                                          Error **errp)
 {
-    ERRP_GUARD();
     MPQemuMsg msg_reply = {0};
     uint64_t ret = UINT64_MAX;
 
diff --git a/qga/commands-posix-ssh.c b/qga/commands-posix-ssh.c
index 2dda136d64..f3a580b8cc 100644
--- a/qga/commands-posix-ssh.c
+++ b/qga/commands-posix-ssh.c
@@ -45,8 +45,6 @@ get_passwd_entry(const char *username, Error **errp)
     g_autoptr(GError) err = NULL;
     struct passwd *p;
 
-    ERRP_GUARD();
-
     p = g_unix_get_passwd_entry_qemu(username, &err);
     if (p == NULL) {
         error_setg(errp, "failed to lookup user '%s': %s",
@@ -61,8 +59,6 @@ static bool
 mkdir_for_user(const char *path, const struct passwd *p,
                mode_t mode, Error **errp)
 {
-    ERRP_GUARD();
-
     if (g_mkdir(path, mode) == -1) {
         error_setg(errp, "failed to create directory '%s': %s",
                    path, g_strerror(errno));
@@ -87,8 +83,6 @@ mkdir_for_user(const char *path, const struct passwd *p,
 static bool
 check_openssh_pub_key(const char *key, Error **errp)
 {
-    ERRP_GUARD();
-
     /* simple sanity-check, we may want more? */
     if (!key || key[0] == '#' || strchr(key, '\n')) {
         error_setg(errp, "invalid OpenSSH public key: '%s'", key);
@@ -104,8 +98,6 @@ check_openssh_pub_keys(strList *keys, size_t *nkeys, Error **errp)
     size_t n = 0;
     strList *k;
 
-    ERRP_GUARD();
-
     for (k = keys; k != NULL; k = k->next) {
         if (!check_openssh_pub_key(k->value, errp)) {
             return false;
@@ -126,8 +118,6 @@ write_authkeys(const char *path, const GStrv keys,
     g_autofree char *contents = NULL;
     g_autoptr(GError) err = NULL;
 
-    ERRP_GUARD();
-
     contents = g_strjoinv("\n", keys);
     if (!g_file_set_contents(path, contents, -1, &err)) {
         error_setg(errp, "failed to write to '%s': %s", path, err->message);
@@ -155,8 +145,6 @@ read_authkeys(const char *path, Error **errp)
     g_autoptr(GError) err = NULL;
     g_autofree char *contents = NULL;
 
-    ERRP_GUARD();
-
     if (!g_file_get_contents(path, &contents, NULL, &err)) {
         error_setg(errp, "failed to read '%s': %s", path, err->message);
         return NULL;
@@ -178,7 +166,6 @@ qmp_guest_ssh_add_authorized_keys(const char *username, strList *keys,
     strList *k;
     size_t nkeys, nauthkeys;
 
-    ERRP_GUARD();
     reset = has_reset && reset;
 
     if (!check_openssh_pub_keys(keys, &nkeys, errp)) {
@@ -228,8 +215,6 @@ qmp_guest_ssh_remove_authorized_keys(const char *username, strList *keys,
     GStrv a;
     size_t nkeys = 0;
 
-    ERRP_GUARD();
-
     if (!check_openssh_pub_keys(keys, NULL, errp)) {
         return;
     }
@@ -277,8 +262,6 @@ qmp_guest_ssh_get_authorized_keys(const char *username, Error **errp)
     g_autoptr(GuestAuthorizedKeys) ret = NULL;
     int i;
 
-    ERRP_GUARD();
-
     p = get_passwd_entry(username, errp);
     if (p == NULL) {
         return NULL;
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index a099acb34d..a6d2c0d88e 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -976,7 +976,6 @@ out:
 
 GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
 {
-    ERRP_GUARD();
     GuestDiskInfoList *ret = NULL;
     HDEVINFO dev_info;
     SP_DEVICE_INTERFACE_DATA dev_iface_data;
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 0d2db1abc3..f9ffeb8d4d 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1539,7 +1539,6 @@ machine_parse_property_opt(QemuOptsList *opts_list, const char *propname,
 {
     QDict *opts, *prop;
     bool help = false;
-    ERRP_GUARD();
 
     prop = keyval_parse(arg, opts_list->implied_opt_name, &help, errp);
     if (help) {
diff --git a/ui/console.c b/ui/console.c
index 5d2e6178ff..eabbbc951c 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -1481,7 +1481,6 @@ static bool displaychangelistener_has_dmabuf(DisplayChangeListener *dcl)
 static bool dpy_compatible_with(QemuConsole *con,
                                 DisplayChangeListener *dcl, Error **errp)
 {
-    ERRP_GUARD();
     int flags;
 
     flags = con->hw_ops->get_flags ? con->hw_ops->get_flags(con->hw) : 0;
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 15/16] vl: Clean up -smp error handling
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (13 preceding siblings ...)
  2021-07-20 12:54 ` [PATCH 14/16] Remove superfluous ERRP_GUARD() Markus Armbruster
@ 2021-07-20 12:54 ` Markus Armbruster
  2021-07-20 18:07   ` Philippe Mathieu-Daudé
  2021-07-20 12:54 ` [PATCH 16/16] vl: Don't continue after -smp help Markus Armbruster
                   ` (2 subsequent siblings)
  17 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:54 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini

The Error ** argument must be NULL, &error_abort, &error_fatal, or a
pointer to a variable containing NULL.  Passing an argument of the
latter kind twice without clearing it in between is wrong: if the
first call sets an error, it no longer points to NULL for the second
call.

machine_parse_property_opt() is wrong that way: it passes @errp to
keyval_parse() without checking for failure, then passes it to
keyval_merge().  Harmless, since the only caller passes &error_fatal.

Clean up: drop the parameter, and use &error_fatal directly.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 softmmu/vl.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index f9ffeb8d4d..ce0ecc736b 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1535,19 +1535,19 @@ static void machine_help_func(const QDict *qdict)
 
 static void
 machine_parse_property_opt(QemuOptsList *opts_list, const char *propname,
-                           const char *arg, Error **errp)
+                           const char *arg)
 {
     QDict *opts, *prop;
     bool help = false;
 
-    prop = keyval_parse(arg, opts_list->implied_opt_name, &help, errp);
+    prop = keyval_parse(arg, opts_list->implied_opt_name, &help, &error_fatal);
     if (help) {
         qemu_opts_print_help(opts_list, true);
         return;
     }
     opts = qdict_new();
     qdict_put(opts, propname, prop);
-    keyval_merge(machine_opts_dict, opts, errp);
+    keyval_merge(machine_opts_dict, opts, &error_fatal);
     qobject_unref(opts);
 }
 
@@ -3321,7 +3321,8 @@ void qemu_init(int argc, char **argv, char **envp)
                 }
                 break;
             case QEMU_OPTION_smp:
-                machine_parse_property_opt(qemu_find_opts("smp-opts"), "smp", optarg, &error_fatal);
+                machine_parse_property_opt(qemu_find_opts("smp-opts"),
+                                           "smp", optarg);
                 break;
             case QEMU_OPTION_vnc:
                 vnc_parse(optarg);
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* [PATCH 16/16] vl: Don't continue after -smp help.
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (14 preceding siblings ...)
  2021-07-20 12:54 ` [PATCH 15/16] vl: Clean up -smp error handling Markus Armbruster
@ 2021-07-20 12:54 ` Markus Armbruster
  2021-07-20 18:08   ` [PATCH-for-6.1? " Philippe Mathieu-Daudé
  2021-07-20 21:57   ` [PATCH " Pankaj Gupta
  2021-07-24  0:30 ` [PATCH 00/16] Various error handling fixes and cleanups Michael S. Tsirkin
  2021-07-29 13:23 ` Markus Armbruster
  17 siblings, 2 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-07-20 12:54 UTC (permalink / raw)
  To: qemu-devel; +Cc: Paolo Bonzini

We continue after -smp help:

    $ qemu-system-x86_64 -smp help -display none -monitor stdio
    smp-opts options:
      cores=<num>
      cpus=<num>
      dies=<num>
      maxcpus=<num>
      sockets=<num>
      threads=<num>
    QEMU 6.0.50 monitor - type 'help' for more information
    (qemu)

Other options, such as -object help and -device help, don't.

Adjust -smp not to continue either.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 softmmu/vl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index ce0ecc736b..8f9d97635a 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1543,7 +1543,7 @@ machine_parse_property_opt(QemuOptsList *opts_list, const char *propname,
     prop = keyval_parse(arg, opts_list->implied_opt_name, &help, &error_fatal);
     if (help) {
         qemu_opts_print_help(opts_list, true);
-        return;
+        exit(0);
     }
     opts = qdict_new();
     qdict_put(opts, propname, prop);
-- 
2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 02/16] spapr: Plug memory leak when we can't add a migration blocker
  2021-07-20 12:53 ` [PATCH 02/16] spapr: Plug memory leak when we can't add a migration blocker Markus Armbruster
@ 2021-07-20 13:12   ` David Gibson
  2021-08-02 18:54   ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 51+ messages in thread
From: David Gibson @ 2021-07-20 13:12 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Aravinda Prasad, Ganesh Goudar, qemu-devel

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

On Tue, Jul 20, 2021 at 02:53:54PM +0200, Markus Armbruster wrote:
> Fixes: 2500fb423adb17995485de0b4d507cf2f09e3a7f
> Cc: Aravinda Prasad <arawinda.p@gmail.com>
> Cc: Ganesh Goudar <ganeshgr@linux.ibm.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

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

> ---
>  hw/ppc/spapr_events.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 0cfc19be19..a8f2cc6bdc 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -872,7 +872,6 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
>      SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      CPUState *cs = CPU(cpu);
>      int ret;
> -    Error *local_err = NULL;
>  
>      if (spapr->fwnmi_machine_check_addr == -1) {
>          /* Non-FWNMI case, deliver it like an architected CPU interrupt. */
> @@ -912,7 +911,7 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
>          }
>      }
>  
> -    ret = migrate_add_blocker(spapr->fwnmi_migration_blocker, &local_err);
> +    ret = migrate_add_blocker(spapr->fwnmi_migration_blocker, NULL);
>      if (ret == -EBUSY) {
>          /*
>           * We don't want to abort so we let the migration to continue.

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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 11/16] microvm: Drop dead error handling in microvm_machine_state_init()
  2021-07-20 12:54 ` [PATCH 11/16] microvm: Drop dead error handling in microvm_machine_state_init() Markus Armbruster
@ 2021-07-20 13:36   ` Sergio Lopez
  2021-07-20 18:06   ` Philippe Mathieu-Daudé
  2021-07-20 21:59   ` Pankaj Gupta
  2 siblings, 0 replies; 51+ messages in thread
From: Sergio Lopez @ 2021-07-20 13:36 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel

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

On Tue, Jul 20, 2021 at 02:54:03PM +0200, Markus Armbruster wrote:
> Stillborn in commit 0ebf007dda "hw/i386: Introduce the microvm machine
> type".
> 
> Cc: Sergio Lopez <slp@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/i386/microvm.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> index aba0c83219..f257ec5a0b 100644
> --- a/hw/i386/microvm.c
> +++ b/hw/i386/microvm.c
> @@ -458,15 +458,10 @@ static void microvm_machine_state_init(MachineState *machine)
>  {
>      MicrovmMachineState *mms = MICROVM_MACHINE(machine);
>      X86MachineState *x86ms = X86_MACHINE(machine);
> -    Error *local_err = NULL;
>  
>      microvm_memory_init(mms);
>  
>      x86_cpus_init(x86ms, CPU_VERSION_LATEST);
> -    if (local_err) {
> -        error_report_err(local_err);
> -        exit(1);
> -    }
>  
>      microvm_devices_init(mms);
>  }
> -- 
> 2.31.1
> 

Reviewed-by: Sergio Lopez <slp@redhat.com>

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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 01/16] error: Use error_fatal to simplify obvious fatal errors (again)
  2021-07-20 12:53 ` [PATCH 01/16] error: Use error_fatal to simplify obvious fatal errors (again) Markus Armbruster
@ 2021-07-20 13:42   ` Eric Blake
  2021-07-20 17:55   ` Peter Xu
  2021-08-02 19:02   ` Philippe Mathieu-Daudé
  2 siblings, 0 replies; 51+ messages in thread
From: Eric Blake @ 2021-07-20 13:42 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Thomas Huth, Daniel P . Berrangé,
	Juan Quintela, qemu-devel, Peter Xu, Gerd Hoffmann,
	Stefan Hajnoczi, Paolo Bonzini, Cornelia Huck,
	Marc-André Lureau

On Tue, Jul 20, 2021 at 02:53:53PM +0200, Markus Armbruster wrote:
> We did this with scripts/coccinelle/use-error_fatal.cocci before, in
> commit 50beeb68094 and 007b06578ab.  This commit cleans up rarer
> variations that don't seem worth matching with Coccinelle.
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

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

I agree with your assessment that this is not bug-fix material needed
in 6.1.

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



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 01/16] error: Use error_fatal to simplify obvious fatal errors (again)
  2021-07-20 12:53 ` [PATCH 01/16] error: Use error_fatal to simplify obvious fatal errors (again) Markus Armbruster
  2021-07-20 13:42   ` Eric Blake
@ 2021-07-20 17:55   ` Peter Xu
  2021-08-02 19:02   ` Philippe Mathieu-Daudé
  2 siblings, 0 replies; 51+ messages in thread
From: Peter Xu @ 2021-07-20 17:55 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Thomas Huth, Daniel P . Berrangé,
	Juan Quintela, qemu-devel, Gerd Hoffmann, Stefan Hajnoczi,
	Marc-André Lureau, Cornelia Huck, Paolo Bonzini

On Tue, Jul 20, 2021 at 02:53:53PM +0200, Markus Armbruster wrote:
> We did this with scripts/coccinelle/use-error_fatal.cocci before, in
> commit 50beeb68094 and 007b06578ab.  This commit cleans up rarer
> variations that don't seem worth matching with Coccinelle.
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

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

-- 
Peter Xu



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 04/16] multi-process: Fix pci_proxy_dev_realize() error handling
  2021-07-20 12:53 ` [PATCH 04/16] multi-process: Fix pci_proxy_dev_realize() error handling Markus Armbruster
@ 2021-07-20 18:02   ` Philippe Mathieu-Daudé
  2021-07-20 20:02   ` Jag Raman
  1 sibling, 0 replies; 51+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-20 18:02 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Jagannathan Raman, Stefan Hajnoczi

On 7/20/21 2:53 PM, Markus Armbruster wrote:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
> 
> pci_proxy_dev_realize() is wrong that way: it passes @errp to
> qio_channel_new_fd() without checking for failure.  If it runs into
> another failure, it trips error_setv()'s assertion.
> 
> Fix it to check for failure properly.
> 
> Fixes: 9f8112073aad8e485ac012ee18809457ab7f23a6
> Cc: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Cc: Jagannathan Raman <jag.raman@oracle.com>
> Cc: John G Johnson <john.g.johnson@oracle.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/remote/proxy.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)

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



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 07/16] vfio: Avoid error_propagate() after migrate_add_blocker()
  2021-07-20 12:53 ` [PATCH 07/16] vfio: Avoid error_propagate() after migrate_add_blocker() Markus Armbruster
@ 2021-07-20 18:03   ` Philippe Mathieu-Daudé
  2021-07-21  6:24   ` Kirti Wankhede
  2021-07-21  9:26   ` Paolo Bonzini
  2 siblings, 0 replies; 51+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-20 18:03 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel; +Cc: Kirti Wankhede, Alex Williamson

On 7/20/21 2:53 PM, Markus Armbruster wrote:
> When migrate_add_blocker(blocker, &errp) is followed by
> error_propagate(errp, err), we can often just as well do
> migrate_add_blocker(..., errp).  This is the case in
> vfio_migration_probe().
> 
> Prior art: commit 386f6c07d2 "error: Avoid error_propagate() after
> migrate_add_blocker()".
> 
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/vfio/migration.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)

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



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 09/16] migration: Unify failure check for migrate_add_blocker()
  2021-07-20 12:54 ` [PATCH 09/16] migration: Unify failure check for migrate_add_blocker() Markus Armbruster
@ 2021-07-20 18:05   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 51+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-20 18:05 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel

On 7/20/21 2:54 PM, Markus Armbruster wrote:
> Most callers check the return value.  Some check whether it set an
> error.  Functionally equivalent, but the former tends to be easier on
> the eyes, so do that everywhere.
> 
> Prior art: commit c6ecec43b2 "qemu-option: Check return value instead
> of @err where convenient".
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  backends/tpm/tpm_emulator.c | 3 +--
>  hw/display/qxl.c            | 8 ++++----
>  hw/virtio/vhost.c           | 2 +-
>  target/i386/kvm/kvm.c       | 6 +++---
>  target/i386/nvmm/nvmm-all.c | 3 +--
>  target/i386/whpx/whpx-all.c | 3 +--
>  6 files changed, 11 insertions(+), 14 deletions(-)

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



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 11/16] microvm: Drop dead error handling in microvm_machine_state_init()
  2021-07-20 12:54 ` [PATCH 11/16] microvm: Drop dead error handling in microvm_machine_state_init() Markus Armbruster
  2021-07-20 13:36   ` Sergio Lopez
@ 2021-07-20 18:06   ` Philippe Mathieu-Daudé
  2021-07-20 21:59   ` Pankaj Gupta
  2 siblings, 0 replies; 51+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-20 18:06 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel; +Cc: Sergio Lopez

On 7/20/21 2:54 PM, Markus Armbruster wrote:
> Stillborn in commit 0ebf007dda "hw/i386: Introduce the microvm machine
> type".
> 
> Cc: Sergio Lopez <slp@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/i386/microvm.c | 5 -----
>  1 file changed, 5 deletions(-)

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



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 15/16] vl: Clean up -smp error handling
  2021-07-20 12:54 ` [PATCH 15/16] vl: Clean up -smp error handling Markus Armbruster
@ 2021-07-20 18:07   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 51+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-20 18:07 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel; +Cc: Paolo Bonzini

On 7/20/21 2:54 PM, Markus Armbruster wrote:
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
> 
> machine_parse_property_opt() is wrong that way: it passes @errp to
> keyval_parse() without checking for failure, then passes it to
> keyval_merge().  Harmless, since the only caller passes &error_fatal.
> 
> Clean up: drop the parameter, and use &error_fatal directly.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  softmmu/vl.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

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



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH-for-6.1? 16/16] vl: Don't continue after -smp help.
  2021-07-20 12:54 ` [PATCH 16/16] vl: Don't continue after -smp help Markus Armbruster
@ 2021-07-20 18:08   ` Philippe Mathieu-Daudé
  2021-07-20 21:57   ` [PATCH " Pankaj Gupta
  1 sibling, 0 replies; 51+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-07-20 18:08 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel; +Cc: Paolo Bonzini

On 7/20/21 2:54 PM, Markus Armbruster wrote:
> We continue after -smp help:
> 
>     $ qemu-system-x86_64 -smp help -display none -monitor stdio
>     smp-opts options:
>       cores=<num>
>       cpus=<num>
>       dies=<num>
>       maxcpus=<num>
>       sockets=<num>
>       threads=<num>
>     QEMU 6.0.50 monitor - type 'help' for more information
>     (qemu)
> 
> Other options, such as -object help and -device help, don't.
> 
> Adjust -smp not to continue either.
> 
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  softmmu/vl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index ce0ecc736b..8f9d97635a 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1543,7 +1543,7 @@ machine_parse_property_opt(QemuOptsList *opts_list, const char *propname,
>      prop = keyval_parse(arg, opts_list->implied_opt_name, &help, &error_fatal);
>      if (help) {
>          qemu_opts_print_help(opts_list, true);
> -        return;
> +        exit(0);
>      }
>      opts = qdict_new();
>      qdict_put(opts, propname, prop);
> 

Easy one for 6.1.

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



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 10/16] migration: Handle migration_incoming_setup() errors consistently
  2021-07-20 12:54 ` [PATCH 10/16] migration: Handle migration_incoming_setup() errors consistently Markus Armbruster
@ 2021-07-20 18:53   ` Eric Blake
  2021-07-21 14:12     ` Markus Armbruster
  2021-07-20 21:55   ` Pankaj Gupta
  2021-08-02 17:53   ` Dr. David Alan Gilbert
  2 siblings, 1 reply; 51+ messages in thread
From: Eric Blake @ 2021-07-20 18:53 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Juan Quintela, qemu-devel, Dr . David Alan Gilbert

On Tue, Jul 20, 2021 at 02:54:02PM +0200, Markus Armbruster wrote:
> Commit b673eab4e2 "multifd: Make multifd_load_setup() get an Error
> parameter" changed migration_incoming_setup() to take an Error **
> argument, and adjusted the callers accordingly.  It neglected to
> change adjust multifd_load_setup(): it still exit()s on error.  Clean
> that up.
> 
> The error now gets propagated up two call chains: via
> migration_fd_process_incoming() to rdma_accept_incoming_migration(),
> and via migration_ioc_process_incoming() to
> migration_channel_process_incoming().  Both chain ends report the
> error with error_report_err(), but otherwise ignore it.  Behavioral
> change: we no longer exit() on this error.
> 
> This is consistent with how we handle other errors here, e.g. from
> multifd_recv_new_channel() via migration_ioc_process_incoming() to
> migration_channel_process_incoming().  Wether it's consistently right

Whether

> or consistently wrong I can't tell.
> 
> Also clean up the return value from the unusual 0 on success, 1 on
> error to the more common true on success, false on error.
> 
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  migration/migration.c | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
>

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

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



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 04/16] multi-process: Fix pci_proxy_dev_realize() error handling
  2021-07-20 12:53 ` [PATCH 04/16] multi-process: Fix pci_proxy_dev_realize() error handling Markus Armbruster
  2021-07-20 18:02   ` Philippe Mathieu-Daudé
@ 2021-07-20 20:02   ` Jag Raman
  1 sibling, 0 replies; 51+ messages in thread
From: Jag Raman @ 2021-07-20 20:02 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Elena Ufimtseva, John Johnson, qemu-devel, Stefan Hajnoczi



> On Jul 20, 2021, at 8:53 AM, Markus Armbruster <armbru@redhat.com> wrote:
> 
> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
> pointer to a variable containing NULL.  Passing an argument of the
> latter kind twice without clearing it in between is wrong: if the
> first call sets an error, it no longer points to NULL for the second
> call.
> 
> pci_proxy_dev_realize() is wrong that way: it passes @errp to
> qio_channel_new_fd() without checking for failure.  If it runs into
> another failure, it trips error_setv()'s assertion.
> 
> Fix it to check for failure properly.
> 
> Fixes: 9f8112073aad8e485ac012ee18809457ab7f23a6
> Cc: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Cc: Jagannathan Raman <jag.raman@oracle.com>
> Cc: John G Johnson <john.g.johnson@oracle.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/remote/proxy.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)

Acked-by: Jagannathan Raman <jag.raman@oracle.com>


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 10/16] migration: Handle migration_incoming_setup() errors consistently
  2021-07-20 12:54 ` [PATCH 10/16] migration: Handle migration_incoming_setup() errors consistently Markus Armbruster
  2021-07-20 18:53   ` Eric Blake
@ 2021-07-20 21:55   ` Pankaj Gupta
  2021-08-02 17:53   ` Dr. David Alan Gilbert
  2 siblings, 0 replies; 51+ messages in thread
From: Pankaj Gupta @ 2021-07-20 21:55 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Juan Quintela, Qemu Developers, Dr . David Alan Gilbert

> Commit b673eab4e2 "multifd: Make multifd_load_setup() get an Error
> parameter" changed migration_incoming_setup() to take an Error **
> argument, and adjusted the callers accordingly.  It neglected to
> change adjust multifd_load_setup(): it still exit()s on error.  Clean
> that up.
>
> The error now gets propagated up two call chains: via
> migration_fd_process_incoming() to rdma_accept_incoming_migration(),
> and via migration_ioc_process_incoming() to
> migration_channel_process_incoming().  Both chain ends report the
> error with error_report_err(), but otherwise ignore it.  Behavioral
> change: we no longer exit() on this error.
>
> This is consistent with how we handle other errors here, e.g. from
> multifd_recv_new_channel() via migration_ioc_process_incoming() to
> migration_channel_process_incoming().  Wether it's consistently right
> or consistently wrong I can't tell.
>
> Also clean up the return value from the unusual 0 on success, 1 on
> error to the more common true on success, false on error.
>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  migration/migration.c | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 231dc24414..c1c0a48647 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -609,30 +609,25 @@ fail:
>  }
>
>  /**
> - * @migration_incoming_setup: Setup incoming migration
> - *
> - * Returns 0 for no error or 1 for error
> - *
> + * migration_incoming_setup: Setup incoming migration
>   * @f: file for main migration channel
>   * @errp: where to put errors
> + *
> + * Returns: %true on success, %false on error.
>   */
> -static int migration_incoming_setup(QEMUFile *f, Error **errp)
> +static bool migration_incoming_setup(QEMUFile *f, Error **errp)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
> -    Error *local_err = NULL;
>
> -    if (multifd_load_setup(&local_err) != 0) {
> -        /* We haven't been able to create multifd threads
> -           nothing better to do */
> -        error_report_err(local_err);
> -        exit(EXIT_FAILURE);
> +    if (multifd_load_setup(errp) != 0) {
> +        return false;
>      }
>
>      if (!mis->from_src_file) {
>          mis->from_src_file = f;
>      }
>      qemu_file_set_blocking(f, false);
> -    return 0;
> +    return true;
>  }
>
>  void migration_incoming_process(void)
> @@ -675,14 +670,11 @@ static bool postcopy_try_recover(QEMUFile *f)
>
>  void migration_fd_process_incoming(QEMUFile *f, Error **errp)
>  {
> -    Error *local_err = NULL;
> -
>      if (postcopy_try_recover(f)) {
>          return;
>      }
>
> -    if (migration_incoming_setup(f, &local_err)) {
> -        error_propagate(errp, local_err);
> +    if (!migration_incoming_setup(f, errp)) {
>          return;
>      }
>      migration_incoming_process();
> @@ -703,8 +695,7 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>              return;
>          }
>
> -        if (migration_incoming_setup(f, &local_err)) {
> -            error_propagate(errp, local_err);
> +        if (!migration_incoming_setup(f, errp)) {
>              return;
>          }
>

Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 16/16] vl: Don't continue after -smp help.
  2021-07-20 12:54 ` [PATCH 16/16] vl: Don't continue after -smp help Markus Armbruster
  2021-07-20 18:08   ` [PATCH-for-6.1? " Philippe Mathieu-Daudé
@ 2021-07-20 21:57   ` Pankaj Gupta
  1 sibling, 0 replies; 51+ messages in thread
From: Pankaj Gupta @ 2021-07-20 21:57 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Paolo Bonzini, Qemu Developers

> We continue after -smp help:
>
>     $ qemu-system-x86_64 -smp help -display none -monitor stdio
>     smp-opts options:
>       cores=<num>
>       cpus=<num>
>       dies=<num>
>       maxcpus=<num>
>       sockets=<num>
>       threads=<num>
>     QEMU 6.0.50 monitor - type 'help' for more information
>     (qemu)
>
> Other options, such as -object help and -device help, don't.
>
> Adjust -smp not to continue either.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  softmmu/vl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index ce0ecc736b..8f9d97635a 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1543,7 +1543,7 @@ machine_parse_property_opt(QemuOptsList *opts_list, const char *propname,
>      prop = keyval_parse(arg, opts_list->implied_opt_name, &help, &error_fatal);
>      if (help) {
>          qemu_opts_print_help(opts_list, true);
> -        return;
> +        exit(0);
>      }
>      opts = qdict_new();
>      qdict_put(opts, propname, prop);
> --

Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 11/16] microvm: Drop dead error handling in microvm_machine_state_init()
  2021-07-20 12:54 ` [PATCH 11/16] microvm: Drop dead error handling in microvm_machine_state_init() Markus Armbruster
  2021-07-20 13:36   ` Sergio Lopez
  2021-07-20 18:06   ` Philippe Mathieu-Daudé
@ 2021-07-20 21:59   ` Pankaj Gupta
  2 siblings, 0 replies; 51+ messages in thread
From: Pankaj Gupta @ 2021-07-20 21:59 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: Qemu Developers, Sergio Lopez

> Stillborn in commit 0ebf007dda "hw/i386: Introduce the microvm machine
> type".
>
> Cc: Sergio Lopez <slp@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/i386/microvm.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
> index aba0c83219..f257ec5a0b 100644
> --- a/hw/i386/microvm.c
> +++ b/hw/i386/microvm.c
> @@ -458,15 +458,10 @@ static void microvm_machine_state_init(MachineState *machine)
>  {
>      MicrovmMachineState *mms = MICROVM_MACHINE(machine);
>      X86MachineState *x86ms = X86_MACHINE(machine);
> -    Error *local_err = NULL;
>
>      microvm_memory_init(mms);
>
>      x86_cpus_init(x86ms, CPU_VERSION_LATEST);
> -    if (local_err) {
> -        error_report_err(local_err);
> -        exit(1);
> -    }
>
>      microvm_devices_init(mms);
>  }
> --
> 2.31.1

Reviewed-by: Pankaj Gupta <pankaj.gupta@ionos.com>


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 03/16] spapr: Explain purpose of ->fwnmi_migration_blocker more clearly
  2021-07-20 12:53 ` [PATCH 03/16] spapr: Explain purpose of ->fwnmi_migration_blocker more clearly Markus Armbruster
@ 2021-07-21  6:08   ` David Gibson
  0 siblings, 0 replies; 51+ messages in thread
From: David Gibson @ 2021-07-21  6:08 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Aravinda Prasad, Ganesh Goudar, qemu-devel, Dr . David Alan Gilbert

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

On Tue, Jul 20, 2021 at 02:53:55PM +0200, Markus Armbruster wrote:
> spapr_mce_req_event() makes an effort to prevent migration from
> degrading the reporting of FWNMIs.  It adds a migration blocker when
> it receives one, and deletes it when it's done handling it.  This is a
> best effort.
> 
> Commit 2500fb423a "migration: Include migration support for machine
> check handling" tried to explain this in a comment.  Rewrite the
> comment for clarity, and reposition it to make it clear it applies to
> all failure modes, not just "migration already in progress".
> 
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Aravinda Prasad <arawinda.p@gmail.com>
> Cc: Ganesh Goudar <ganeshgr@linux.ibm.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

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

> ---
>  hw/ppc/spapr_events.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index a8f2cc6bdc..7d6876f12d 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -911,16 +911,17 @@ void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
>          }
>      }
>  
> +    /*
> +     * Try to block migration while FWNMI is being handled, so the
> +     * machine check handler runs where the information passed to it
> +     * actually makes sense.  This shouldn't actually block migration,
> +     * only delay it slightly, assuming migration is retried.  If the
> +     * attempt to block fails, carry on.  Unfortunately, it always
> +     * fails when running with -only-migrate.  A proper interface to
> +     * delay migration completion for a bit could avoid that.
> +     */
>      ret = migrate_add_blocker(spapr->fwnmi_migration_blocker, NULL);
>      if (ret == -EBUSY) {
> -        /*
> -         * We don't want to abort so we let the migration to continue.
> -         * In a rare case, the machine check handler will run on the target.
> -         * Though this is not preferable, it is better than aborting
> -         * the migration or killing the VM. It is okay to call
> -         * migrate_del_blocker on a blocker that was not added (which the
> -         * nmi-interlock handler would do when it's called after this).
> -         */
>          warn_report("Received a fwnmi while migration was in progress");
>      }
>  

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

^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 07/16] vfio: Avoid error_propagate() after migrate_add_blocker()
  2021-07-20 12:53 ` [PATCH 07/16] vfio: Avoid error_propagate() after migrate_add_blocker() Markus Armbruster
  2021-07-20 18:03   ` Philippe Mathieu-Daudé
@ 2021-07-21  6:24   ` Kirti Wankhede
  2021-07-21  9:26   ` Paolo Bonzini
  2 siblings, 0 replies; 51+ messages in thread
From: Kirti Wankhede @ 2021-07-21  6:24 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel; +Cc: Alex Williamson



On 7/20/2021 6:23 PM, Markus Armbruster wrote:
> When migrate_add_blocker(blocker, &errp) is followed by
> error_propagate(errp, err), we can often just as well do
> migrate_add_blocker(..., errp).  This is the case in
> vfio_migration_probe().
> 
> Prior art: commit 386f6c07d2 "error: Avoid error_propagate() after
> migrate_add_blocker()".
> 
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   hw/vfio/migration.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 82f654afb6..ff6b45de6b 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -858,7 +858,6 @@ int vfio_migration_probe(VFIODevice *vbasedev, Error **errp)
>   {
>       VFIOContainer *container = vbasedev->group->container;
>       struct vfio_region_info *info = NULL;
> -    Error *local_err = NULL;
>       int ret = -ENOTSUP;
>   
>       if (!vbasedev->enable_migration || !container->dirty_pages_supported) {
> @@ -885,9 +884,8 @@ add_blocker:
>                  "VFIO device doesn't support migration");
>       g_free(info);
>   
> -    ret = migrate_add_blocker(vbasedev->migration_blocker, &local_err);
> -    if (local_err) {
> -        error_propagate(errp, local_err);
> +    ret = migrate_add_blocker(vbasedev->migration_blocker, errp);
> +    if (ret < 0) {
>           error_free(vbasedev->migration_blocker);
>           vbasedev->migration_blocker = NULL;
>       }
> 

Reviewed by: Kirti Wankhede <kwankhede@nvidia.com>


^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 07/16] vfio: Avoid error_propagate() after migrate_add_blocker()
  2021-07-20 12:53 ` [PATCH 07/16] vfio: Avoid error_propagate() after migrate_add_blocker() Markus Armbruster
  2021-07-20 18:03   ` Philippe Mathieu-Daudé
  2021-07-21  6:24   ` Kirti Wankhede
@ 2021-07-21  9:26   ` Paolo Bonzini
  2021-07-21 14:12     ` Markus Armbruster
  2 siblings, 1 reply; 51+ messages in thread
From: Paolo Bonzini @ 2021-07-21  9:26 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel; +Cc: Kirti Wankhede, Alex Williamson

On 20/07/21 14:53, Markus Armbruster wrote:
> When migrate_add_blocker(blocker, &errp) is followed by

Just a nit, &err.

Paolo

> error_propagate(errp, err), we can often just as well do
> migrate_add_blocker(..., errp).  This is the case in
> vfio_migration_probe().



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 07/16] vfio: Avoid error_propagate() after migrate_add_blocker()
  2021-07-21  9:26   ` Paolo Bonzini
@ 2021-07-21 14:12     ` Markus Armbruster
  0 siblings, 0 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-07-21 14:12 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Kirti Wankhede, Alex Williamson, qemu-devel

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 20/07/21 14:53, Markus Armbruster wrote:
>> When migrate_add_blocker(blocker, &errp) is followed by
>
> Just a nit, &err.

Wll fix, thanks!

> Paolo
>
>> error_propagate(errp, err), we can often just as well do
>> migrate_add_blocker(..., errp).  This is the case in
>> vfio_migration_probe().



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 10/16] migration: Handle migration_incoming_setup() errors consistently
  2021-07-20 18:53   ` Eric Blake
@ 2021-07-21 14:12     ` Markus Armbruster
  0 siblings, 0 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-07-21 14:12 UTC (permalink / raw)
  To: Eric Blake; +Cc: Juan Quintela, qemu-devel, Dr . David Alan Gilbert

Eric Blake <eblake@redhat.com> writes:

> On Tue, Jul 20, 2021 at 02:54:02PM +0200, Markus Armbruster wrote:
>> Commit b673eab4e2 "multifd: Make multifd_load_setup() get an Error
>> parameter" changed migration_incoming_setup() to take an Error **
>> argument, and adjusted the callers accordingly.  It neglected to
>> change adjust multifd_load_setup(): it still exit()s on error.  Clean
>> that up.
>> 
>> The error now gets propagated up two call chains: via
>> migration_fd_process_incoming() to rdma_accept_incoming_migration(),
>> and via migration_ioc_process_incoming() to
>> migration_channel_process_incoming().  Both chain ends report the
>> error with error_report_err(), but otherwise ignore it.  Behavioral
>> change: we no longer exit() on this error.
>> 
>> This is consistent with how we handle other errors here, e.g. from
>> multifd_recv_new_channel() via migration_ioc_process_incoming() to
>> migration_channel_process_incoming().  Wether it's consistently right
>
> Whether

ACK

>> or consistently wrong I can't tell.
>> 
>> Also clean up the return value from the unusual 0 on success, 1 on
>> error to the more common true on success, false on error.
>> 
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  migration/migration.c | 27 +++++++++------------------
>>  1 file changed, 9 insertions(+), 18 deletions(-)
>>
>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 08/16] whpx nvmm: Drop useless migrate_del_blocker()
  2021-07-20 12:54 ` [PATCH 08/16] whpx nvmm: Drop useless migrate_del_blocker() Markus Armbruster
@ 2021-07-22  8:04   ` Reinoud Zandijk
  0 siblings, 0 replies; 51+ messages in thread
From: Reinoud Zandijk @ 2021-07-22  8:04 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Sunil Muthuswamy, Kamil Rytarowski, Reinoud Zandijk, qemu-devel

On Tue, Jul 20, 2021 at 02:54:00PM +0200, Markus Armbruster wrote:
> There is nothing to delete after migrate_add_blocker() failed.  Trying
> anyway is safe, but useless.  Don't.
> 
> Cc: Sunil Muthuswamy <sunilmut@microsoft.com>
> Cc: Kamil Rytarowski <kamil@netbsd.org>
> Cc: Reinoud Zandijk <reinoud@netbsd.org>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  target/i386/nvmm/nvmm-all.c | 1 -
>  target/i386/whpx/whpx-all.c | 1 -
>  2 files changed, 2 deletions(-)
> 
> diff --git a/target/i386/nvmm/nvmm-all.c b/target/i386/nvmm/nvmm-all.c
> index dfa690d65d..7bb0d9e30e 100644
> --- a/target/i386/nvmm/nvmm-all.c
> +++ b/target/i386/nvmm/nvmm-all.c
> @@ -929,7 +929,6 @@ nvmm_init_vcpu(CPUState *cpu)
>          (void)migrate_add_blocker(nvmm_migration_blocker, &local_error);
>          if (local_error) {
>              error_report_err(local_error);
> -            migrate_del_blocker(nvmm_migration_blocker);
>              error_free(nvmm_migration_blocker);
>              return -EINVAL;
>          }

Reviewed-by: Reinoud Zandijk <reinoud@NetBSD.org>        

No problem with it



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 00/16] Various error handling fixes and cleanups
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (15 preceding siblings ...)
  2021-07-20 12:54 ` [PATCH 16/16] vl: Don't continue after -smp help Markus Armbruster
@ 2021-07-24  0:30 ` Michael S. Tsirkin
  2021-07-29 13:23 ` Markus Armbruster
  17 siblings, 0 replies; 51+ messages in thread
From: Michael S. Tsirkin @ 2021-07-24  0:30 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: Elena Ufimtseva, John G Johnson, qemu-devel, Peter Xu,
	Kirti Wankhede, Gerd Hoffmann, Reinoud Zandijk,
	Jagannathan Raman, Juan Quintela, Kamil Rytarowski,
	Ganesh Goudar, Marc-André Lureau, Aravinda Prasad,
	Thomas Huth, Eduardo Habkost, Sergio Lopez,
	Dr . David Alan Gilbert, Alex Williamson, Stefan Hajnoczi,
	Cornelia Huck, Sunil Muthuswamy, David Gibson, Kevin Wolf,
	Daniel P . Berrangé,
	Marcelo Tosatti, Paolo Bonzini

On Tue, Jul 20, 2021 at 02:53:52PM +0200, Markus Armbruster wrote:
> I doubt the fixes are 6.1 material at this late stage.  If you
> disagree, let me know.
> 
> Cc: Alex Williamson <alex.williamson@redhat.com>
> Cc: Aravinda Prasad <arawinda.p@gmail.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: Elena Ufimtseva <elena.ufimtseva@oracle.com>
> Cc: Ganesh Goudar <ganeshgr@linux.ibm.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Jagannathan Raman <jag.raman@oracle.com>
> Cc: John G Johnson <john.g.johnson@oracle.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Kamil Rytarowski <kamil@netbsd.org>
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Marcelo Tosatti <mtosatti@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Reinoud Zandijk <reinoud@netbsd.org>
> Cc: Sergio Lopez <slp@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Sunil Muthuswamy <sunilmut@microsoft.com>
> Cc: Thomas Huth <thuth@redhat.com>


Acked-by: Michael S. Tsirkin <mst@redhat.com>

agree it's not 6.1 material

> Markus Armbruster (16):
>   error: Use error_fatal to simplify obvious fatal errors (again)
>   spapr: Plug memory leak when we can't add a migration blocker
>   spapr: Explain purpose of ->fwnmi_migration_blocker more clearly
>   multi-process: Fix pci_proxy_dev_realize() error handling
>   vhost-scsi: Plug memory leak on migrate_add_blocker() failure
>   i386: Never free migration blocker objects instead of sometimes
>   vfio: Avoid error_propagate() after migrate_add_blocker()
>   whpx nvmm: Drop useless migrate_del_blocker()
>   migration: Unify failure check for migrate_add_blocker()
>   migration: Handle migration_incoming_setup() errors consistently
>   microvm: Drop dead error handling in microvm_machine_state_init()
>   vhost: Clean up how VhostOpts method vhost_get_config() fails
>   vhost: Clean up how VhostOpts method vhost_backend_init() fails
>   Remove superfluous ERRP_GUARD()
>   vl: Clean up -smp error handling
>   vl: Don't continue after -smp help.
> 
>  backends/tpm/tpm_emulator.c |  3 +--
>  hw/display/qxl.c            |  8 ++++----
>  hw/i386/microvm.c           |  5 -----
>  hw/ppc/spapr_events.c       | 20 ++++++++++----------
>  hw/remote/mpqemu-link.c     |  3 ---
>  hw/remote/proxy.c           | 10 +++++++++-
>  hw/s390x/ipl.c              |  6 +-----
>  hw/scsi/vhost-scsi.c        |  4 ++--
>  hw/vfio/migration.c         |  6 ++----
>  hw/virtio/vhost-user.c      |  8 ++++++++
>  hw/virtio/vhost.c           | 16 +++-------------
>  migration/migration.c       | 34 ++++++++++------------------------
>  qemu-img.c                  |  6 +-----
>  qemu-io.c                   |  6 +-----
>  qemu-nbd.c                  |  5 +----
>  qga/commands-posix-ssh.c    | 17 -----------------
>  qga/commands-win32.c        |  1 -
>  scsi/qemu-pr-helper.c       | 11 +++--------
>  softmmu/vl.c                | 19 +++++++------------
>  target/i386/kvm/kvm.c       |  9 +++------
>  target/i386/nvmm/nvmm-all.c |  4 +---
>  target/i386/sev.c           |  8 +-------
>  target/i386/whpx/whpx-all.c |  4 +---
>  ui/console.c                |  7 ++-----
>  ui/spice-core.c             |  7 +------
>  25 files changed, 72 insertions(+), 155 deletions(-)
> 
> -- 
> 2.31.1



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 00/16] Various error handling fixes and cleanups
  2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
                   ` (16 preceding siblings ...)
  2021-07-24  0:30 ` [PATCH 00/16] Various error handling fixes and cleanups Michael S. Tsirkin
@ 2021-07-29 13:23 ` Markus Armbruster
  17 siblings, 0 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-07-29 13:23 UTC (permalink / raw)
  To: qemu-devel
  Cc: Elena Ufimtseva, John G Johnson, Michael S . Tsirkin, Peter Xu,
	Kirti Wankhede, Gerd Hoffmann, Reinoud Zandijk,
	Jagannathan Raman, Juan Quintela, Kamil Rytarowski,
	Ganesh Goudar, Marc-André Lureau, Aravinda Prasad,
	Thomas Huth, Eduardo Habkost, Sergio Lopez,
	Dr . David Alan Gilbert, Alex Williamson, Stefan Hajnoczi,
	Cornelia Huck, Sunil Muthuswamy, David Gibson, Kevin Wolf,
	Daniel P . Berrangé,
	Marcelo Tosatti, Paolo Bonzini

Markus Armbruster <armbru@redhat.com> writes:

> I doubt the fixes are 6.1 material at this late stage.  If you
> disagree, let me know.

PATCH 16 has become commit 3e61a13af3.  Remainder queued for 6.2;
additional review is welcome all the same.

Thanks, guys!



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 10/16] migration: Handle migration_incoming_setup() errors consistently
  2021-07-20 12:54 ` [PATCH 10/16] migration: Handle migration_incoming_setup() errors consistently Markus Armbruster
  2021-07-20 18:53   ` Eric Blake
  2021-07-20 21:55   ` Pankaj Gupta
@ 2021-08-02 17:53   ` Dr. David Alan Gilbert
  2021-08-03  5:58     ` Markus Armbruster
  2 siblings, 1 reply; 51+ messages in thread
From: Dr. David Alan Gilbert @ 2021-08-02 17:53 UTC (permalink / raw)
  To: Markus Armbruster; +Cc: qemu-devel, Juan Quintela

* Markus Armbruster (armbru@redhat.com) wrote:
> Commit b673eab4e2 "multifd: Make multifd_load_setup() get an Error
> parameter" changed migration_incoming_setup() to take an Error **
> argument, and adjusted the callers accordingly.  It neglected to
> change adjust multifd_load_setup(): it still exit()s on error.  Clean
> that up.
> 
> The error now gets propagated up two call chains: via
> migration_fd_process_incoming() to rdma_accept_incoming_migration(),
> and via migration_ioc_process_incoming() to
> migration_channel_process_incoming().  Both chain ends report the
> error with error_report_err(), but otherwise ignore it.  Behavioral
> change: we no longer exit() on this error.
> 
> This is consistent with how we handle other errors here, e.g. from
> multifd_recv_new_channel() via migration_ioc_process_incoming() to
> migration_channel_process_incoming().  Wether it's consistently right
> or consistently wrong I can't tell.
> 
> Also clean up the return value from the unusual 0 on success, 1 on
> error to the more common true on success, false on error.
> 
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  migration/migration.c | 27 +++++++++------------------
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 231dc24414..c1c0a48647 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -609,30 +609,25 @@ fail:
>  }
>  
>  /**
> - * @migration_incoming_setup: Setup incoming migration
> - *
> - * Returns 0 for no error or 1 for error
> - *
> + * migration_incoming_setup: Setup incoming migration
>   * @f: file for main migration channel
>   * @errp: where to put errors
> + *
> + * Returns: %true on success, %false on error.
>   */
> -static int migration_incoming_setup(QEMUFile *f, Error **errp)
> +static bool migration_incoming_setup(QEMUFile *f, Error **errp)
>  {
>      MigrationIncomingState *mis = migration_incoming_get_current();
> -    Error *local_err = NULL;
>  
> -    if (multifd_load_setup(&local_err) != 0) {
> -        /* We haven't been able to create multifd threads
> -           nothing better to do */
> -        error_report_err(local_err);
> -        exit(EXIT_FAILURE);
> +    if (multifd_load_setup(errp) != 0) {
> +        return false;
>      }

What I don't know is how well that will survive; for example in
multifd_load_setup it creates multiple threads and calls the recv_setup
member on each thread; now if one of those fails what happens - if we
don't exit, is it cleaned up enough so you can try another
migrate_incoming or is it just going to fall over?

Dave

>  
>      if (!mis->from_src_file) {
>          mis->from_src_file = f;
>      }
>      qemu_file_set_blocking(f, false);
> -    return 0;
> +    return true;
>  }
>  
>  void migration_incoming_process(void)
> @@ -675,14 +670,11 @@ static bool postcopy_try_recover(QEMUFile *f)
>  
>  void migration_fd_process_incoming(QEMUFile *f, Error **errp)
>  {
> -    Error *local_err = NULL;
> -
>      if (postcopy_try_recover(f)) {
>          return;
>      }
>  
> -    if (migration_incoming_setup(f, &local_err)) {
> -        error_propagate(errp, local_err);
> +    if (!migration_incoming_setup(f, errp)) {
>          return;
>      }
>      migration_incoming_process();
> @@ -703,8 +695,7 @@ void migration_ioc_process_incoming(QIOChannel *ioc, Error **errp)
>              return;
>          }
>  
> -        if (migration_incoming_setup(f, &local_err)) {
> -            error_propagate(errp, local_err);
> +        if (!migration_incoming_setup(f, errp)) {
>              return;
>          }
>  
> -- 
> 2.31.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 02/16] spapr: Plug memory leak when we can't add a migration blocker
  2021-07-20 12:53 ` [PATCH 02/16] spapr: Plug memory leak when we can't add a migration blocker Markus Armbruster
  2021-07-20 13:12   ` David Gibson
@ 2021-08-02 18:54   ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 51+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-02 18:54 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel
  Cc: Aravinda Prasad, Ganesh Goudar, David Gibson

On 7/20/21 2:53 PM, Markus Armbruster wrote:
> Fixes: 2500fb423adb17995485de0b4d507cf2f09e3a7f
> Cc: Aravinda Prasad <arawinda.p@gmail.com>
> Cc: Ganesh Goudar <ganeshgr@linux.ibm.com>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/ppc/spapr_events.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

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



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 12/16] vhost: Clean up how VhostOpts method vhost_get_config() fails
  2021-07-20 12:54 ` [PATCH 12/16] vhost: Clean up how VhostOpts method vhost_get_config() fails Markus Armbruster
@ 2021-08-02 18:56   ` Philippe Mathieu-Daudé
  2021-08-02 18:58   ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 51+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-02 18:56 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel; +Cc: Kevin Wolf, Michael S . Tsirkin

On 7/20/21 2:54 PM, Markus Armbruster wrote:
> vhost_user_get_config() can fail without setting an error.  Unclean.
> Its caller vhost_dev_get_config() compensates by substituting a
> generic error then.  Goes back to commit 50de51387f "vhost:
> Distinguish errors in vhost_dev_get_config()".
> 
> Clean up by moving the generic error from vhost_dev_get_config() to
> all the failure paths that neglect to set an error.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/virtio/vhost-user.c |  2 ++
>  hw/virtio/vhost.c      | 10 ++--------
>  2 files changed, 4 insertions(+), 8 deletions(-)

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



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 12/16] vhost: Clean up how VhostOpts method vhost_get_config() fails
  2021-07-20 12:54 ` [PATCH 12/16] vhost: Clean up how VhostOpts method vhost_get_config() fails Markus Armbruster
  2021-08-02 18:56   ` Philippe Mathieu-Daudé
@ 2021-08-02 18:58   ` Philippe Mathieu-Daudé
  2021-08-03  5:48     ` Markus Armbruster
  1 sibling, 1 reply; 51+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-02 18:58 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel; +Cc: Kevin Wolf, Michael S . Tsirkin

On 7/20/21 2:54 PM, Markus Armbruster wrote:
> vhost_user_get_config() can fail without setting an error.  Unclean.
> Its caller vhost_dev_get_config() compensates by substituting a
> generic error then.  Goes back to commit 50de51387f "vhost:
> Distinguish errors in vhost_dev_get_config()".
> 
> Clean up by moving the generic error from vhost_dev_get_config() to
> all the failure paths that neglect to set an error.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/virtio/vhost-user.c |  2 ++
>  hw/virtio/vhost.c      | 10 ++--------
>  2 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 29ea2b4fce..dbbd6fbc25 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -2139,10 +2139,12 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
>      msg.payload.config.offset = 0;
>      msg.payload.config.size = config_len;
>      if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> +        error_setg_errno(errp, -EPROTO, "vhost_get_config failed");
>          return -EPROTO;
>      }
>  
>      if (vhost_user_read(dev, &msg) < 0) {
> +        error_setg_errno(errp, -EPROTO, "vhost_get_config failed");
>          return -EPROTO;
>      }

Oops, should be error_setg_errno(EPROTO)!



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 13/16] vhost: Clean up how VhostOpts method vhost_backend_init() fails
  2021-07-20 12:54 ` [PATCH 13/16] vhost: Clean up how VhostOpts method vhost_backend_init() fails Markus Armbruster
@ 2021-08-02 18:59   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 51+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-02 18:59 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel; +Cc: Kevin Wolf, Michael S . Tsirkin

On 7/20/21 2:54 PM, Markus Armbruster wrote:
> vhost_user_backend_init() can fail without setting an error.  Unclean.
> Its caller vhost_dev_init() compensates by substituting a generic
> error then.  Goes back to commit 28770ff935 "vhost: Distinguish errors
> in vhost_backend_init()".
> 
> Clean up by moving the generic error from vhost_dev_init() to all the
> failure paths that neglect to set an error.
> 
> Cc: Kevin Wolf <kwolf@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/virtio/vhost-user.c | 6 ++++++
>  hw/virtio/vhost.c      | 4 ----
>  2 files changed, 6 insertions(+), 4 deletions(-)

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



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 14/16] Remove superfluous ERRP_GUARD()
  2021-07-20 12:54 ` [PATCH 14/16] Remove superfluous ERRP_GUARD() Markus Armbruster
@ 2021-08-02 19:00   ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 51+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-02 19:00 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel; +Cc: Marc-André Lureau

Cc'ing Marc-André as he recently disagreed with this.

On 7/20/21 2:54 PM, Markus Armbruster wrote:
> Macro ERRP_GUARD() is only needed when we want to dereference @errp or
> pass it to error_prepend() or error_append_hint().  Delete superfluous
> ones.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/remote/mpqemu-link.c  |  3 ---
>  qga/commands-posix-ssh.c | 17 -----------------
>  qga/commands-win32.c     |  1 -
>  softmmu/vl.c             |  1 -
>  ui/console.c             |  1 -
>  5 files changed, 23 deletions(-)
> 
> diff --git a/hw/remote/mpqemu-link.c b/hw/remote/mpqemu-link.c
> index e67a5de72c..7e841820e5 100644
> --- a/hw/remote/mpqemu-link.c
> +++ b/hw/remote/mpqemu-link.c
> @@ -34,7 +34,6 @@
>   */
>  bool mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp)
>  {
> -    ERRP_GUARD();
>      bool iolock = qemu_mutex_iothread_locked();
>      bool iothread = qemu_in_iothread();
>      struct iovec send[2] = {};
> @@ -97,7 +96,6 @@ bool mpqemu_msg_send(MPQemuMsg *msg, QIOChannel *ioc, Error **errp)
>  static ssize_t mpqemu_read(QIOChannel *ioc, void *buf, size_t len, int **fds,
>                             size_t *nfds, Error **errp)
>  {
> -    ERRP_GUARD();
>      struct iovec iov = { .iov_base = buf, .iov_len = len };
>      bool iolock = qemu_mutex_iothread_locked();
>      bool iothread = qemu_in_iothread();
> @@ -192,7 +190,6 @@ fail:
>  uint64_t mpqemu_msg_send_and_await_reply(MPQemuMsg *msg, PCIProxyDev *pdev,
>                                           Error **errp)
>  {
> -    ERRP_GUARD();
>      MPQemuMsg msg_reply = {0};
>      uint64_t ret = UINT64_MAX;
>  
> diff --git a/qga/commands-posix-ssh.c b/qga/commands-posix-ssh.c
> index 2dda136d64..f3a580b8cc 100644
> --- a/qga/commands-posix-ssh.c
> +++ b/qga/commands-posix-ssh.c
> @@ -45,8 +45,6 @@ get_passwd_entry(const char *username, Error **errp)
>      g_autoptr(GError) err = NULL;
>      struct passwd *p;
>  
> -    ERRP_GUARD();
> -
>      p = g_unix_get_passwd_entry_qemu(username, &err);
>      if (p == NULL) {
>          error_setg(errp, "failed to lookup user '%s': %s",
> @@ -61,8 +59,6 @@ static bool
>  mkdir_for_user(const char *path, const struct passwd *p,
>                 mode_t mode, Error **errp)
>  {
> -    ERRP_GUARD();
> -
>      if (g_mkdir(path, mode) == -1) {
>          error_setg(errp, "failed to create directory '%s': %s",
>                     path, g_strerror(errno));
> @@ -87,8 +83,6 @@ mkdir_for_user(const char *path, const struct passwd *p,
>  static bool
>  check_openssh_pub_key(const char *key, Error **errp)
>  {
> -    ERRP_GUARD();
> -
>      /* simple sanity-check, we may want more? */
>      if (!key || key[0] == '#' || strchr(key, '\n')) {
>          error_setg(errp, "invalid OpenSSH public key: '%s'", key);
> @@ -104,8 +98,6 @@ check_openssh_pub_keys(strList *keys, size_t *nkeys, Error **errp)
>      size_t n = 0;
>      strList *k;
>  
> -    ERRP_GUARD();
> -
>      for (k = keys; k != NULL; k = k->next) {
>          if (!check_openssh_pub_key(k->value, errp)) {
>              return false;
> @@ -126,8 +118,6 @@ write_authkeys(const char *path, const GStrv keys,
>      g_autofree char *contents = NULL;
>      g_autoptr(GError) err = NULL;
>  
> -    ERRP_GUARD();
> -
>      contents = g_strjoinv("\n", keys);
>      if (!g_file_set_contents(path, contents, -1, &err)) {
>          error_setg(errp, "failed to write to '%s': %s", path, err->message);
> @@ -155,8 +145,6 @@ read_authkeys(const char *path, Error **errp)
>      g_autoptr(GError) err = NULL;
>      g_autofree char *contents = NULL;
>  
> -    ERRP_GUARD();
> -
>      if (!g_file_get_contents(path, &contents, NULL, &err)) {
>          error_setg(errp, "failed to read '%s': %s", path, err->message);
>          return NULL;
> @@ -178,7 +166,6 @@ qmp_guest_ssh_add_authorized_keys(const char *username, strList *keys,
>      strList *k;
>      size_t nkeys, nauthkeys;
>  
> -    ERRP_GUARD();
>      reset = has_reset && reset;
>  
>      if (!check_openssh_pub_keys(keys, &nkeys, errp)) {
> @@ -228,8 +215,6 @@ qmp_guest_ssh_remove_authorized_keys(const char *username, strList *keys,
>      GStrv a;
>      size_t nkeys = 0;
>  
> -    ERRP_GUARD();
> -
>      if (!check_openssh_pub_keys(keys, NULL, errp)) {
>          return;
>      }
> @@ -277,8 +262,6 @@ qmp_guest_ssh_get_authorized_keys(const char *username, Error **errp)
>      g_autoptr(GuestAuthorizedKeys) ret = NULL;
>      int i;
>  
> -    ERRP_GUARD();
> -
>      p = get_passwd_entry(username, errp);
>      if (p == NULL) {
>          return NULL;
> diff --git a/qga/commands-win32.c b/qga/commands-win32.c
> index a099acb34d..a6d2c0d88e 100644
> --- a/qga/commands-win32.c
> +++ b/qga/commands-win32.c
> @@ -976,7 +976,6 @@ out:
>  
>  GuestDiskInfoList *qmp_guest_get_disks(Error **errp)
>  {
> -    ERRP_GUARD();
>      GuestDiskInfoList *ret = NULL;
>      HDEVINFO dev_info;
>      SP_DEVICE_INTERFACE_DATA dev_iface_data;
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 0d2db1abc3..f9ffeb8d4d 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1539,7 +1539,6 @@ machine_parse_property_opt(QemuOptsList *opts_list, const char *propname,
>  {
>      QDict *opts, *prop;
>      bool help = false;
> -    ERRP_GUARD();
>  
>      prop = keyval_parse(arg, opts_list->implied_opt_name, &help, errp);
>      if (help) {
> diff --git a/ui/console.c b/ui/console.c
> index 5d2e6178ff..eabbbc951c 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -1481,7 +1481,6 @@ static bool displaychangelistener_has_dmabuf(DisplayChangeListener *dcl)
>  static bool dpy_compatible_with(QemuConsole *con,
>                                  DisplayChangeListener *dcl, Error **errp)
>  {
> -    ERRP_GUARD();
>      int flags;
>  
>      flags = con->hw_ops->get_flags ? con->hw_ops->get_flags(con->hw) : 0;
> 



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 01/16] error: Use error_fatal to simplify obvious fatal errors (again)
  2021-07-20 12:53 ` [PATCH 01/16] error: Use error_fatal to simplify obvious fatal errors (again) Markus Armbruster
  2021-07-20 13:42   ` Eric Blake
  2021-07-20 17:55   ` Peter Xu
@ 2021-08-02 19:02   ` Philippe Mathieu-Daudé
  2021-08-03  5:44     ` Markus Armbruster
  2 siblings, 1 reply; 51+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-08-02 19:02 UTC (permalink / raw)
  To: Markus Armbruster, qemu-devel
  Cc: Thomas Huth, Daniel P . Berrangé,
	Juan Quintela, Peter Xu, Gerd Hoffmann, Stefan Hajnoczi,
	Paolo Bonzini, Cornelia Huck, Marc-André Lureau

On 7/20/21 2:53 PM, Markus Armbruster wrote:
> We did this with scripts/coccinelle/use-error_fatal.cocci before, in
> commit 50beeb68094 and 007b06578ab.  This commit cleans up rarer
> variations that don't seem worth matching with Coccinelle.
> 
> Cc: Thomas Huth <thuth@redhat.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Juan Quintela <quintela@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/s390x/ipl.c        |  6 +-----
>  migration/migration.c |  7 +------
>  qemu-img.c            |  6 +-----
>  qemu-io.c             |  6 +-----
>  qemu-nbd.c            |  5 +----
>  scsi/qemu-pr-helper.c | 11 +++--------
>  softmmu/vl.c          |  7 +------
>  target/i386/sev.c     |  8 +-------
>  ui/console.c          |  6 ++----
>  ui/spice-core.c       |  7 +------
>  10 files changed, 13 insertions(+), 56 deletions(-)

> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 4df1496101..0d2db1abc3 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -2673,12 +2673,7 @@ void qmp_x_exit_preconfig(Error **errp)
>      qemu_machine_creation_done();
>  
>      if (loadvm) {
> -        Error *local_err = NULL;
> -        if (!load_snapshot(loadvm, NULL, false, NULL, &local_err)) {
> -            error_report_err(local_err);
> -            autostart = 0;

Uh, odd assignment...

> -            exit(1);
> -        }
> +        load_snapshot(loadvm, NULL, false, NULL, &error_fatal);
>      }
>      if (replay_mode != REPLAY_MODE_NONE) {
>          replay_vmstate_init();

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



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 01/16] error: Use error_fatal to simplify obvious fatal errors (again)
  2021-08-02 19:02   ` Philippe Mathieu-Daudé
@ 2021-08-03  5:44     ` Markus Armbruster
  0 siblings, 0 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-08-03  5:44 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Thomas Huth, Daniel P . Berrangé,
	Juan Quintela, qemu-devel, Peter Xu, Gerd Hoffmann,
	Stefan Hajnoczi, Paolo Bonzini, Cornelia Huck,
	Marc-André Lureau

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

> On 7/20/21 2:53 PM, Markus Armbruster wrote:
>> We did this with scripts/coccinelle/use-error_fatal.cocci before, in
>> commit 50beeb68094 and 007b06578ab.  This commit cleans up rarer
>> variations that don't seem worth matching with Coccinelle.
>> 
>> Cc: Thomas Huth <thuth@redhat.com>
>> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
>> Cc: Peter Xu <peterx@redhat.com>
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Marc-André Lureau <marcandre.lureau@redhat.com>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/s390x/ipl.c        |  6 +-----
>>  migration/migration.c |  7 +------
>>  qemu-img.c            |  6 +-----
>>  qemu-io.c             |  6 +-----
>>  qemu-nbd.c            |  5 +----
>>  scsi/qemu-pr-helper.c | 11 +++--------
>>  softmmu/vl.c          |  7 +------
>>  target/i386/sev.c     |  8 +-------
>>  ui/console.c          |  6 ++----
>>  ui/spice-core.c       |  7 +------
>>  10 files changed, 13 insertions(+), 56 deletions(-)
>
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index 4df1496101..0d2db1abc3 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -2673,12 +2673,7 @@ void qmp_x_exit_preconfig(Error **errp)
>>      qemu_machine_creation_done();
>>  
>>      if (loadvm) {
>> -        Error *local_err = NULL;
>> -        if (!load_snapshot(loadvm, NULL, false, NULL, &local_err)) {
>> -            error_report_err(local_err);
>> -            autostart = 0;
>
> Uh, odd assignment...

Yup.

Commit 05f2401eb2 "make load_vmstate() return errors" added the
assignment:

    @@ -6030,8 +6030,11 @@ int main(int argc, char **argv, char **envp)
             exit(1);
         }

    -    if (loadvm)
    -        load_vmstate(cur_mon, loadvm);
    +    if (loadvm) {
    +        if (load_vmstate(cur_mon, loadvm) < 0) {
    +            autostart = 0;
    +        }
    +    }

         if (incoming) {
             qemu_start_incoming_migration(incoming);


827beacb47 "Add a hint message to loadvm and exits on failure" added the
exit(1) without deleting the now useless assignment:

    @@ -4530,6 +4530,7 @@ int main(int argc, char **argv, char **envp)
             if (load_snapshot(loadvm, &local_err) < 0) {
                 error_report_err(local_err);
                 autostart = 0;
    +            exit(1);
             }
         }

>> -            exit(1);
>> -        }
>> +        load_snapshot(loadvm, NULL, false, NULL, &error_fatal);
>>      }
>>      if (replay_mode != REPLAY_MODE_NONE) {
>>          replay_vmstate_init();
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 12/16] vhost: Clean up how VhostOpts method vhost_get_config() fails
  2021-08-02 18:58   ` Philippe Mathieu-Daudé
@ 2021-08-03  5:48     ` Markus Armbruster
  0 siblings, 0 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-08-03  5:48 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: Kevin Wolf, qemu-devel, Michael S . Tsirkin

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

> On 7/20/21 2:54 PM, Markus Armbruster wrote:
>> vhost_user_get_config() can fail without setting an error.  Unclean.
>> Its caller vhost_dev_get_config() compensates by substituting a
>> generic error then.  Goes back to commit 50de51387f "vhost:
>> Distinguish errors in vhost_dev_get_config()".
>> 
>> Clean up by moving the generic error from vhost_dev_get_config() to
>> all the failure paths that neglect to set an error.
>> 
>> Cc: Kevin Wolf <kwolf@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/virtio/vhost-user.c |  2 ++
>>  hw/virtio/vhost.c      | 10 ++--------
>>  2 files changed, 4 insertions(+), 8 deletions(-)
>> 
>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> index 29ea2b4fce..dbbd6fbc25 100644
>> --- a/hw/virtio/vhost-user.c
>> +++ b/hw/virtio/vhost-user.c
>> @@ -2139,10 +2139,12 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
>>      msg.payload.config.offset = 0;
>>      msg.payload.config.size = config_len;
>>      if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>> +        error_setg_errno(errp, -EPROTO, "vhost_get_config failed");
>>          return -EPROTO;
>>      }
>>  
>>      if (vhost_user_read(dev, &msg) < 0) {
>> +        error_setg_errno(errp, -EPROTO, "vhost_get_config failed");
>>          return -EPROTO;
>>      }
>
> Oops, should be error_setg_errno(EPROTO)!

D'oh!  Thanks!



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 10/16] migration: Handle migration_incoming_setup() errors consistently
  2021-08-02 17:53   ` Dr. David Alan Gilbert
@ 2021-08-03  5:58     ` Markus Armbruster
  2021-08-31 12:47       ` Markus Armbruster
  0 siblings, 1 reply; 51+ messages in thread
From: Markus Armbruster @ 2021-08-03  5:58 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: qemu-devel, Juan Quintela

"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:

> * Markus Armbruster (armbru@redhat.com) wrote:
>> Commit b673eab4e2 "multifd: Make multifd_load_setup() get an Error
>> parameter" changed migration_incoming_setup() to take an Error **
>> argument, and adjusted the callers accordingly.  It neglected to
>> change adjust multifd_load_setup(): it still exit()s on error.  Clean
>> that up.
>> 
>> The error now gets propagated up two call chains: via
>> migration_fd_process_incoming() to rdma_accept_incoming_migration(),
>> and via migration_ioc_process_incoming() to
>> migration_channel_process_incoming().  Both chain ends report the
>> error with error_report_err(), but otherwise ignore it.  Behavioral
>> change: we no longer exit() on this error.
>> 
>> This is consistent with how we handle other errors here, e.g. from
>> multifd_recv_new_channel() via migration_ioc_process_incoming() to
>> migration_channel_process_incoming().  Wether it's consistently right
>> or consistently wrong I can't tell.
>> 
>> Also clean up the return value from the unusual 0 on success, 1 on
>> error to the more common true on success, false on error.
>> 
>> Cc: Juan Quintela <quintela@redhat.com>
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  migration/migration.c | 27 +++++++++------------------
>>  1 file changed, 9 insertions(+), 18 deletions(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 231dc24414..c1c0a48647 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -609,30 +609,25 @@ fail:
>>  }
>>  
>>  /**
>> - * @migration_incoming_setup: Setup incoming migration
>> - *
>> - * Returns 0 for no error or 1 for error
>> - *
>> + * migration_incoming_setup: Setup incoming migration
>>   * @f: file for main migration channel
>>   * @errp: where to put errors
>> + *
>> + * Returns: %true on success, %false on error.
>>   */
>> -static int migration_incoming_setup(QEMUFile *f, Error **errp)
>> +static bool migration_incoming_setup(QEMUFile *f, Error **errp)
>>  {
>>      MigrationIncomingState *mis = migration_incoming_get_current();
>> -    Error *local_err = NULL;
>>  
>> -    if (multifd_load_setup(&local_err) != 0) {
>> -        /* We haven't been able to create multifd threads
>> -           nothing better to do */
>> -        error_report_err(local_err);
>> -        exit(EXIT_FAILURE);
>> +    if (multifd_load_setup(errp) != 0) {
>> +        return false;
>>      }
>
> What I don't know is how well that will survive; for example in
> multifd_load_setup it creates multiple threads and calls the recv_setup
> member on each thread; now if one of those fails what happens - if we
> don't exit, is it cleaned up enough so you can try another
> migrate_incoming or is it just going to fall over?

I don't know, either.

The inconsistent error handling we have is not good.  More consistent
error handling that unmasks bad error handling could be worse, unless we
fix the unmasked badness.

How can we move forward with this patch?

One way *I* could move forward is to tack a FIXME comment to the
problematic exit(1) instead of removing it.

[...]



^ permalink raw reply	[flat|nested] 51+ messages in thread

* Re: [PATCH 10/16] migration: Handle migration_incoming_setup() errors consistently
  2021-08-03  5:58     ` Markus Armbruster
@ 2021-08-31 12:47       ` Markus Armbruster
  0 siblings, 0 replies; 51+ messages in thread
From: Markus Armbruster @ 2021-08-31 12:47 UTC (permalink / raw)
  To: Dr. David Alan Gilbert; +Cc: qemu-devel, Juan Quintela

Markus Armbruster <armbru@redhat.com> writes:

> "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes:
>
>> * Markus Armbruster (armbru@redhat.com) wrote:
>>> Commit b673eab4e2 "multifd: Make multifd_load_setup() get an Error
>>> parameter" changed migration_incoming_setup() to take an Error **
>>> argument, and adjusted the callers accordingly.  It neglected to
>>> change adjust multifd_load_setup(): it still exit()s on error.  Clean
>>> that up.
>>> 
>>> The error now gets propagated up two call chains: via
>>> migration_fd_process_incoming() to rdma_accept_incoming_migration(),
>>> and via migration_ioc_process_incoming() to
>>> migration_channel_process_incoming().  Both chain ends report the
>>> error with error_report_err(), but otherwise ignore it.  Behavioral
>>> change: we no longer exit() on this error.
>>> 
>>> This is consistent with how we handle other errors here, e.g. from
>>> multifd_recv_new_channel() via migration_ioc_process_incoming() to
>>> migration_channel_process_incoming().  Wether it's consistently right
>>> or consistently wrong I can't tell.
>>> 
>>> Also clean up the return value from the unusual 0 on success, 1 on
>>> error to the more common true on success, false on error.
>>> 
>>> Cc: Juan Quintela <quintela@redhat.com>
>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>  migration/migration.c | 27 +++++++++------------------
>>>  1 file changed, 9 insertions(+), 18 deletions(-)
>>> 
>>> diff --git a/migration/migration.c b/migration/migration.c
>>> index 231dc24414..c1c0a48647 100644
>>> --- a/migration/migration.c
>>> +++ b/migration/migration.c
>>> @@ -609,30 +609,25 @@ fail:
>>>  }
>>>  
>>>  /**
>>> - * @migration_incoming_setup: Setup incoming migration
>>> - *
>>> - * Returns 0 for no error or 1 for error
>>> - *
>>> + * migration_incoming_setup: Setup incoming migration
>>>   * @f: file for main migration channel
>>>   * @errp: where to put errors
>>> + *
>>> + * Returns: %true on success, %false on error.
>>>   */
>>> -static int migration_incoming_setup(QEMUFile *f, Error **errp)
>>> +static bool migration_incoming_setup(QEMUFile *f, Error **errp)
>>>  {
>>>      MigrationIncomingState *mis = migration_incoming_get_current();
>>> -    Error *local_err = NULL;
>>>  
>>> -    if (multifd_load_setup(&local_err) != 0) {
>>> -        /* We haven't been able to create multifd threads
>>> -           nothing better to do */
>>> -        error_report_err(local_err);
>>> -        exit(EXIT_FAILURE);
>>> +    if (multifd_load_setup(errp) != 0) {
>>> +        return false;
>>>      }
>>
>> What I don't know is how well that will survive; for example in
>> multifd_load_setup it creates multiple threads and calls the recv_setup
>> member on each thread; now if one of those fails what happens - if we
>> don't exit, is it cleaned up enough so you can try another
>> migrate_incoming or is it just going to fall over?
>
> I don't know, either.
>
> The inconsistent error handling we have is not good.  More consistent
> error handling that unmasks bad error handling could be worse, unless we
> fix the unmasked badness.
>
> How can we move forward with this patch?
>
> One way *I* could move forward is to tack a FIXME comment to the
> problematic exit(1) instead of removing it.
>
> [...]

I forgot this was still undecided, and included the patch in my pull
request.  It has become commit 7d6f6933aa7.  Feel free to revert it, or
to ask me to restore the exit() on failure.



^ permalink raw reply	[flat|nested] 51+ messages in thread

end of thread, other threads:[~2021-08-31 13:30 UTC | newest]

Thread overview: 51+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-20 12:53 [PATCH 00/16] Various error handling fixes and cleanups Markus Armbruster
2021-07-20 12:53 ` [PATCH 01/16] error: Use error_fatal to simplify obvious fatal errors (again) Markus Armbruster
2021-07-20 13:42   ` Eric Blake
2021-07-20 17:55   ` Peter Xu
2021-08-02 19:02   ` Philippe Mathieu-Daudé
2021-08-03  5:44     ` Markus Armbruster
2021-07-20 12:53 ` [PATCH 02/16] spapr: Plug memory leak when we can't add a migration blocker Markus Armbruster
2021-07-20 13:12   ` David Gibson
2021-08-02 18:54   ` Philippe Mathieu-Daudé
2021-07-20 12:53 ` [PATCH 03/16] spapr: Explain purpose of ->fwnmi_migration_blocker more clearly Markus Armbruster
2021-07-21  6:08   ` David Gibson
2021-07-20 12:53 ` [PATCH 04/16] multi-process: Fix pci_proxy_dev_realize() error handling Markus Armbruster
2021-07-20 18:02   ` Philippe Mathieu-Daudé
2021-07-20 20:02   ` Jag Raman
2021-07-20 12:53 ` [PATCH 05/16] vhost-scsi: Plug memory leak on migrate_add_blocker() failure Markus Armbruster
2021-07-20 12:53 ` [PATCH 06/16] i386: Never free migration blocker objects instead of sometimes Markus Armbruster
2021-07-20 12:53 ` [PATCH 07/16] vfio: Avoid error_propagate() after migrate_add_blocker() Markus Armbruster
2021-07-20 18:03   ` Philippe Mathieu-Daudé
2021-07-21  6:24   ` Kirti Wankhede
2021-07-21  9:26   ` Paolo Bonzini
2021-07-21 14:12     ` Markus Armbruster
2021-07-20 12:54 ` [PATCH 08/16] whpx nvmm: Drop useless migrate_del_blocker() Markus Armbruster
2021-07-22  8:04   ` Reinoud Zandijk
2021-07-20 12:54 ` [PATCH 09/16] migration: Unify failure check for migrate_add_blocker() Markus Armbruster
2021-07-20 18:05   ` Philippe Mathieu-Daudé
2021-07-20 12:54 ` [PATCH 10/16] migration: Handle migration_incoming_setup() errors consistently Markus Armbruster
2021-07-20 18:53   ` Eric Blake
2021-07-21 14:12     ` Markus Armbruster
2021-07-20 21:55   ` Pankaj Gupta
2021-08-02 17:53   ` Dr. David Alan Gilbert
2021-08-03  5:58     ` Markus Armbruster
2021-08-31 12:47       ` Markus Armbruster
2021-07-20 12:54 ` [PATCH 11/16] microvm: Drop dead error handling in microvm_machine_state_init() Markus Armbruster
2021-07-20 13:36   ` Sergio Lopez
2021-07-20 18:06   ` Philippe Mathieu-Daudé
2021-07-20 21:59   ` Pankaj Gupta
2021-07-20 12:54 ` [PATCH 12/16] vhost: Clean up how VhostOpts method vhost_get_config() fails Markus Armbruster
2021-08-02 18:56   ` Philippe Mathieu-Daudé
2021-08-02 18:58   ` Philippe Mathieu-Daudé
2021-08-03  5:48     ` Markus Armbruster
2021-07-20 12:54 ` [PATCH 13/16] vhost: Clean up how VhostOpts method vhost_backend_init() fails Markus Armbruster
2021-08-02 18:59   ` Philippe Mathieu-Daudé
2021-07-20 12:54 ` [PATCH 14/16] Remove superfluous ERRP_GUARD() Markus Armbruster
2021-08-02 19:00   ` Philippe Mathieu-Daudé
2021-07-20 12:54 ` [PATCH 15/16] vl: Clean up -smp error handling Markus Armbruster
2021-07-20 18:07   ` Philippe Mathieu-Daudé
2021-07-20 12:54 ` [PATCH 16/16] vl: Don't continue after -smp help Markus Armbruster
2021-07-20 18:08   ` [PATCH-for-6.1? " Philippe Mathieu-Daudé
2021-07-20 21:57   ` [PATCH " Pankaj Gupta
2021-07-24  0:30 ` [PATCH 00/16] Various error handling fixes and cleanups Michael S. Tsirkin
2021-07-29 13:23 ` Markus Armbruster

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.