diff mbox series

[V2,05/11] migration: MigrationEvent for notifiers

Message ID 1705071910-174321-6-git-send-email-steven.sistare@oracle.com (mailing list archive)
State New, archived
Headers show
Series allow cpr-reboot for vfio | expand

Commit Message

Steven Sistare Jan. 12, 2024, 3:05 p.m. UTC 
Passing MigrationState to notifiers is unsound because they could access
unstable migration state internals or even modify the state.  Instead, pass
the minimal info needed in a new MigrationEvent struct, which could be
extended in the future if needed.

Suggested-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
---
 hw/net/virtio-net.c      | 12 +++++++-----
 hw/vfio/migration.c      |  8 +++++---
 include/migration/misc.h |  5 +++++
 migration/migration.c    |  5 ++++-
 net/vhost-vdpa.c         |  7 ++++---
 ui/spice-core.c          |  9 +++++----
 6 files changed, 30 insertions(+), 16 deletions(-)

Comments

Steven Sistare Jan. 12, 2024, 3:18 p.m. UTC | #1 
On 1/12/2024 10:05 AM, Steve Sistare wrote:
> Passing MigrationState to notifiers is unsound because they could access
> unstable migration state internals or even modify the state.  Instead, pass
> the minimal info needed in a new MigrationEvent struct, which could be
> extended in the future if needed.
> 
> Suggested-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> ---
>  [...]
> diff --git a/include/migration/misc.h b/include/migration/misc.h
> index dcc98bb..0b4ce0f 100644
> --- a/include/migration/misc.h
> +++ b/include/migration/misc.h
> @@ -60,6 +60,11 @@ void migration_object_init(void);
>  void migration_shutdown(void);
>  bool migration_is_idle(void);
>  bool migration_is_active(MigrationState *);
> +
> +typedef struct MigrationEvent {
> +    MigrationStatus state;
> +} MigrationEvent;

Hi Peter, I chose to pass MigrationStatus rather than define a new enum and map
MigrationStatus to it.  IMO a new enum adds little value, yet it is more code and
another layer of abstraction for coders to grok.  

- Steve
diff mbox series

Patch

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 9570353..71e1133 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -3499,7 +3499,7 @@  out:
     return !err;
 }
 
-static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
+static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationEvent *e)
 {
     bool should_be_hidden;
     Error *err = NULL;
@@ -3511,7 +3511,7 @@  static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
 
     should_be_hidden = qatomic_read(&n->failover_primary_hidden);
 
-    if (migration_in_setup(s) && !should_be_hidden) {
+    if (e->state == MIGRATION_STATUS_SETUP && !should_be_hidden) {
         if (failover_unplug_primary(n, dev)) {
             vmstate_unregister(VMSTATE_IF(dev), qdev_get_vmsd(dev), dev);
             qapi_event_send_unplug_primary(dev->id);
@@ -3519,7 +3519,8 @@  static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
         } else {
             warn_report("couldn't unplug primary device");
         }
-    } else if (migration_has_failed(s)) {
+    } else if (e->state == MIGRATION_STATUS_FAILED ||
+               e->state == MIGRATION_STATUS_CANCELLED) {
         /* We already unplugged the device let's plug it back */
         if (!failover_replug_primary(n, dev, &err)) {
             if (err) {
@@ -3532,9 +3533,10 @@  static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s)
 static int virtio_net_migration_state_notifier(NotifierWithReturn *notifier,
                                                void *data, Error **errp)
 {
-    MigrationState *s = data;
+    MigrationEvent *e = data;
+
     VirtIONet *n = container_of(notifier, VirtIONet, migration_state);
-    virtio_net_handle_migration_primary(n, s);
+    virtio_net_handle_migration_primary(n, e);
     return 0;
 }
 
diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
index 6b6acc4..746ec08 100644
--- a/hw/vfio/migration.c
+++ b/hw/vfio/migration.c
@@ -757,19 +757,21 @@  static void vfio_vmstate_change(void *opaque, bool running, RunState state)
 static int vfio_migration_state_notifier(NotifierWithReturn *notifier,
                                          void *data, Error **errp)
 {
-    MigrationState *s = data;
+    MigrationEvent *e = data;
     VFIOMigration *migration = container_of(notifier, VFIOMigration,
                                             migration_state);
     VFIODevice *vbasedev = migration->vbasedev;
 
     trace_vfio_migration_state_notifier(vbasedev->name,
-                                        MigrationStatus_str(s->state));
+                                        MigrationStatus_str(e->state));
 
-    switch (s->state) {
+    switch (e->state) {
     case MIGRATION_STATUS_CANCELLING:
     case MIGRATION_STATUS_CANCELLED:
     case MIGRATION_STATUS_FAILED:
         vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING);
+    default:
+        break;
     }
     return 0;
 }
diff --git a/include/migration/misc.h b/include/migration/misc.h
index dcc98bb..0b4ce0f 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -60,6 +60,11 @@  void migration_object_init(void);
 void migration_shutdown(void);
 bool migration_is_idle(void);
 bool migration_is_active(MigrationState *);
+
+typedef struct MigrationEvent {
+    MigrationStatus state;
+} MigrationEvent;
+
 void migration_add_notifier(NotifierWithReturn *notify,
                             NotifierWithReturnFunc func);
 void migration_remove_notifier(NotifierWithReturn *notify);
diff --git a/migration/migration.c b/migration/migration.c
index ad3acd1..4c5180c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1441,7 +1441,10 @@  void migration_remove_notifier(NotifierWithReturn *notify)
 
 void migration_call_notifiers(MigrationState *s)
 {
-    notifier_with_return_list_notify(&migration_state_notifiers, s, 0);
+    MigrationEvent e;
+
+    e.state = s->state;
+    notifier_with_return_list_notify(&migration_state_notifiers, &e, 0);
 }
 
 bool migration_in_setup(MigrationState *s)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 1c00519..f96ac75 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -325,13 +325,14 @@  static void vhost_vdpa_net_log_global_enable(VhostVDPAState *s, bool enable)
 static int vdpa_net_migration_state_notifier(NotifierWithReturn *notifier,
                                              void *data, Error **errp)
 {
-    MigrationState *migration = data;
+    MigrationEvent *e = data;
     VhostVDPAState *s = container_of(notifier, VhostVDPAState,
                                      migration_state);
 
-    if (migration_in_setup(migration)) {
+    if (e->state == MIGRATION_STATUS_SETUP) {
         vhost_vdpa_net_log_global_enable(s, true);
-    } else if (migration_has_failed(migration)) {
+    } else if (e->state == MIGRATION_STATUS_FAILED ||
+               e->state == MIGRATION_STATUS_CANCELLED) {
         vhost_vdpa_net_log_global_enable(s, false);
     }
     return 0;
diff --git a/ui/spice-core.c b/ui/spice-core.c
index e43a93f..74aa3bc 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -571,19 +571,20 @@  static SpiceInfo *qmp_query_spice_real(Error **errp)
 static int migration_state_notifier(NotifierWithReturn *notifier,
                                     void *data, Error **errp)
 {
-    MigrationState *s = data;
+    MigrationEvent *e = data;
 
     if (!spice_have_target_host) {
         return 0;
     }
 
-    if (migration_in_setup(s)) {
+    if (e->state == MIGRATION_STATUS_SETUP) {
         spice_server_migrate_start(spice_server);
-    } else if (migration_has_finished(s) ||
+    } else if (e->state == MIGRATION_STATUS_COMPLETED ||
                migration_in_postcopy_after_devices()) {
         spice_server_migrate_end(spice_server, true);
         spice_have_target_host = false;
-    } else if (migration_has_failed(s)) {
+    } else if (e->state == MIGRATION_STATUS_FAILED ||
+               e->state == MIGRATION_STATUS_CANCELLED) {
         spice_server_migrate_end(spice_server, false);
         spice_have_target_host = false;
     }