| Message ID | 1705071910-174321-4-git-send-email-steven.sistare@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | allow cpr-reboot for vfio | expand |
On Fri, Jan 12, 2024 at 07:05:02AM -0800, Steve Sistare wrote: > Change all migration notifiers to type NotifierWithReturn, so notifiers > can return an error status in a future patch. For now, pass NULL for the > notifier error parameter, and do not check the return value. > > Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > --- > hw/net/virtio-net.c | 4 +++- > hw/vfio/migration.c | 4 +++- > include/hw/vfio/vfio-common.h | 2 +- > include/hw/virtio/virtio-net.h | 2 +- > include/migration/misc.h | 6 +++--- > migration/migration.c | 16 ++++++++-------- > net/vhost-vdpa.c | 6 ++++-- > ui/spice-core.c | 8 +++++--- > 8 files changed, 28 insertions(+), 20 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 7a2846f..9570353 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -3529,11 +3529,13 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s) > } > } > > -static void virtio_net_migration_state_notifier(Notifier *notifier, void *data) > +static int virtio_net_migration_state_notifier(NotifierWithReturn *notifier, > + void *data, Error **errp) > { > MigrationState *s = data; > VirtIONet *n = container_of(notifier, VirtIONet, migration_state); > virtio_net_handle_migration_primary(n, s); > + return 0; > } I should have mentioned this earlier.. we have a trend recently to modify retval to boolean when Error** existed, e.g.: https://lore.kernel.org/all/20231017202633.296756-5-peterx@redhat.com/ Let's start using boolean too here? Previous patches may need touch-ups too for this. Other than that it all looks good here. Thanks,
On 1/15/2024 1:44 AM, Peter Xu wrote: > On Fri, Jan 12, 2024 at 07:05:02AM -0800, Steve Sistare wrote: >> Change all migration notifiers to type NotifierWithReturn, so notifiers >> can return an error status in a future patch. For now, pass NULL for the >> notifier error parameter, and do not check the return value. >> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> >> --- >> hw/net/virtio-net.c | 4 +++- >> hw/vfio/migration.c | 4 +++- >> include/hw/vfio/vfio-common.h | 2 +- >> include/hw/virtio/virtio-net.h | 2 +- >> include/migration/misc.h | 6 +++--- >> migration/migration.c | 16 ++++++++-------- >> net/vhost-vdpa.c | 6 ++++-- >> ui/spice-core.c | 8 +++++--- >> 8 files changed, 28 insertions(+), 20 deletions(-) >> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >> index 7a2846f..9570353 100644 >> --- a/hw/net/virtio-net.c >> +++ b/hw/net/virtio-net.c >> @@ -3529,11 +3529,13 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s) >> } >> } >> >> -static void virtio_net_migration_state_notifier(Notifier *notifier, void *data) >> +static int virtio_net_migration_state_notifier(NotifierWithReturn *notifier, >> + void *data, Error **errp) >> { >> MigrationState *s = data; >> VirtIONet *n = container_of(notifier, VirtIONet, migration_state); >> virtio_net_handle_migration_primary(n, s); >> + return 0; >> } > > I should have mentioned this earlier.. we have a trend recently to modify > retval to boolean when Error** existed, e.g.: > > https://lore.kernel.org/all/20231017202633.296756-5-peterx@redhat.com/ > > Let's start using boolean too here? Previous patches may need touch-ups > too for this. > > Other than that it all looks good here. Thanks, Boolean makes sense when there is only one way to recover from failure. However, when the notifier may fail in different ways, and recovery differs for each, then the function should return an int errno. NotifierWithReturn could have future uses that need multiple return values and multiple recovery paths above the notifier_with_return_list_notify level, so IMO the function should continue to return int for generality. - Steve
On Tue, Jan 16, 2024 at 03:35:53PM -0500, Steven Sistare wrote: > On 1/15/2024 1:44 AM, Peter Xu wrote: > > On Fri, Jan 12, 2024 at 07:05:02AM -0800, Steve Sistare wrote: > >> Change all migration notifiers to type NotifierWithReturn, so notifiers > >> can return an error status in a future patch. For now, pass NULL for the > >> notifier error parameter, and do not check the return value. > >> > >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com> > >> --- > >> hw/net/virtio-net.c | 4 +++- > >> hw/vfio/migration.c | 4 +++- > >> include/hw/vfio/vfio-common.h | 2 +- > >> include/hw/virtio/virtio-net.h | 2 +- > >> include/migration/misc.h | 6 +++--- > >> migration/migration.c | 16 ++++++++-------- > >> net/vhost-vdpa.c | 6 ++++-- > >> ui/spice-core.c | 8 +++++--- > >> 8 files changed, 28 insertions(+), 20 deletions(-) > >> > >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > >> index 7a2846f..9570353 100644 > >> --- a/hw/net/virtio-net.c > >> +++ b/hw/net/virtio-net.c > >> @@ -3529,11 +3529,13 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s) > >> } > >> } > >> > >> -static void virtio_net_migration_state_notifier(Notifier *notifier, void *data) > >> +static int virtio_net_migration_state_notifier(NotifierWithReturn *notifier, > >> + void *data, Error **errp) > >> { > >> MigrationState *s = data; > >> VirtIONet *n = container_of(notifier, VirtIONet, migration_state); > >> virtio_net_handle_migration_primary(n, s); > >> + return 0; > >> } > > > > I should have mentioned this earlier.. we have a trend recently to modify > > retval to boolean when Error** existed, e.g.: > > > > https://lore.kernel.org/all/20231017202633.296756-5-peterx@redhat.com/ > > > > Let's start using boolean too here? Previous patches may need touch-ups > > too for this. > > > > Other than that it all looks good here. Thanks, > > Boolean makes sense when there is only one way to recover from failure. > However, when the notifier may fail in different ways, and recovery differs > for each, then the function should return an int errno. NotifierWithReturn > could have future uses that need multiple return values and multiple recovery > paths above the notifier_with_return_list_notify level, so IMO the function > should continue to return int for generality. Ah ok. Please then add a comment either in the commit message or code for that. Thanks.
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 7a2846f..9570353 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -3529,11 +3529,13 @@ static void virtio_net_handle_migration_primary(VirtIONet *n, MigrationState *s) } } -static void virtio_net_migration_state_notifier(Notifier *notifier, void *data) +static int virtio_net_migration_state_notifier(NotifierWithReturn *notifier, + void *data, Error **errp) { MigrationState *s = data; VirtIONet *n = container_of(notifier, VirtIONet, migration_state); virtio_net_handle_migration_primary(n, s); + return 0; } static bool failover_hide_primary_device(DeviceListener *listener, diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c index 70e6b1a..6b6acc4 100644 --- a/hw/vfio/migration.c +++ b/hw/vfio/migration.c @@ -754,7 +754,8 @@ static void vfio_vmstate_change(void *opaque, bool running, RunState state) mig_state_to_str(new_state)); } -static void vfio_migration_state_notifier(Notifier *notifier, void *data) +static int vfio_migration_state_notifier(NotifierWithReturn *notifier, + void *data, Error **errp) { MigrationState *s = data; VFIOMigration *migration = container_of(notifier, VFIOMigration, @@ -770,6 +771,7 @@ static void vfio_migration_state_notifier(Notifier *notifier, void *data) case MIGRATION_STATUS_FAILED: vfio_migration_set_state_or_reset(vbasedev, VFIO_DEVICE_STATE_RUNNING); } + return 0; } static void vfio_migration_free(VFIODevice *vbasedev) diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h index 9b7ef7d..4a6c262 100644 --- a/include/hw/vfio/vfio-common.h +++ b/include/hw/vfio/vfio-common.h @@ -62,7 +62,7 @@ typedef struct VFIORegion { typedef struct VFIOMigration { struct VFIODevice *vbasedev; VMChangeStateEntry *vm_state; - Notifier migration_state; + NotifierWithReturn migration_state; uint32_t device_state; int data_fd; void *data_buffer; diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h index 55977f0..eaee8f4 100644 --- a/include/hw/virtio/virtio-net.h +++ b/include/hw/virtio/virtio-net.h @@ -221,7 +221,7 @@ struct VirtIONet { DeviceListener primary_listener; QDict *primary_opts; bool primary_opts_from_json; - Notifier migration_state; + NotifierWithReturn migration_state; VirtioNetRssData rss_data; struct NetRxPkt *rx_pkt; struct EBPFRSSContext ebpf_rss; diff --git a/include/migration/misc.h b/include/migration/misc.h index 5e65c18..b62e351 100644 --- a/include/migration/misc.h +++ b/include/migration/misc.h @@ -60,9 +60,9 @@ void migration_object_init(void); void migration_shutdown(void); bool migration_is_idle(void); bool migration_is_active(MigrationState *); -void migration_add_notifier(Notifier *notify, - void (*func)(Notifier *notifier, void *data)); -void migration_remove_notifier(Notifier *notify); +void migration_add_notifier(NotifierWithReturn *notify, + NotifierWithReturnFunc func); +void migration_remove_notifier(NotifierWithReturn *notify); void migration_call_notifiers(MigrationState *s); bool migration_in_setup(MigrationState *); bool migration_has_finished(MigrationState *); diff --git a/migration/migration.c b/migration/migration.c index 98c5c3e..fa662a5 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -68,8 +68,8 @@ #include "sysemu/dirtylimit.h" #include "qemu/sockets.h" -static NotifierList migration_state_notifiers = - NOTIFIER_LIST_INITIALIZER(migration_state_notifiers); +static NotifierWithReturnList migration_state_notifiers = + NOTIFIER_WITH_RETURN_LIST_INITIALIZER(migration_state_notifiers); /* Messages sent on the return path from destination to source */ enum mig_rp_message_type { @@ -1424,24 +1424,24 @@ static void migrate_fd_cancel(MigrationState *s) } } -void migration_add_notifier(Notifier *notify, - void (*func)(Notifier *notifier, void *data)) +void migration_add_notifier(NotifierWithReturn *notify, + NotifierWithReturnFunc func) { notify->notify = func; - notifier_list_add(&migration_state_notifiers, notify); + notifier_with_return_list_add(&migration_state_notifiers, notify); } -void migration_remove_notifier(Notifier *notify) +void migration_remove_notifier(NotifierWithReturn *notify) { if (notify->notify) { - notifier_remove(notify); + notifier_with_return_remove(notify); notify->notify = NULL; } } void migration_call_notifiers(MigrationState *s) { - notifier_list_notify(&migration_state_notifiers, s); + notifier_with_return_list_notify(&migration_state_notifiers, s, 0); } bool migration_in_setup(MigrationState *s) diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c index 3726ee5..1c00519 100644 --- a/net/vhost-vdpa.c +++ b/net/vhost-vdpa.c @@ -34,7 +34,7 @@ typedef struct VhostVDPAState { NetClientState nc; struct vhost_vdpa vhost_vdpa; - Notifier migration_state; + NotifierWithReturn migration_state; VHostNetState *vhost_net; /* Control commands shadow buffers */ @@ -322,7 +322,8 @@ static void vhost_vdpa_net_log_global_enable(VhostVDPAState *s, bool enable) } } -static void vdpa_net_migration_state_notifier(Notifier *notifier, void *data) +static int vdpa_net_migration_state_notifier(NotifierWithReturn *notifier, + void *data, Error **errp) { MigrationState *migration = data; VhostVDPAState *s = container_of(notifier, VhostVDPAState, @@ -333,6 +334,7 @@ static void vdpa_net_migration_state_notifier(Notifier *notifier, void *data) } else if (migration_has_failed(migration)) { vhost_vdpa_net_log_global_enable(s, false); } + return 0; } static void vhost_vdpa_net_data_start_first(VhostVDPAState *s) diff --git a/ui/spice-core.c b/ui/spice-core.c index 37b277f..b3cd229 100644 --- a/ui/spice-core.c +++ b/ui/spice-core.c @@ -42,7 +42,7 @@ /* core bits */ static SpiceServer *spice_server; -static Notifier migration_state; +static NotifierWithReturn migration_state; static const char *auth = "spice"; static char *auth_passwd; static time_t auth_expires = TIME_MAX; @@ -568,12 +568,13 @@ static SpiceInfo *qmp_query_spice_real(Error **errp) return info; } -static void migration_state_notifier(Notifier *notifier, void *data) +static int migration_state_notifier(NotifierWithReturn *notifier, + void *data, Error **errp) { MigrationState *s = data; if (!spice_have_target_host) { - return; + return 0; } if (migration_in_setup(s)) { @@ -586,6 +587,7 @@ static void migration_state_notifier(Notifier *notifier, void *data) spice_server_migrate_end(spice_server, false); spice_have_target_host = false; } + return 0; } int qemu_spice_migrate_info(const char *hostname, int port, int tls_port,
Change all migration notifiers to type NotifierWithReturn, so notifiers can return an error status in a future patch. For now, pass NULL for the notifier error parameter, and do not check the return value. Signed-off-by: Steve Sistare <steven.sistare@oracle.com> --- hw/net/virtio-net.c | 4 +++- hw/vfio/migration.c | 4 +++- include/hw/vfio/vfio-common.h | 2 +- include/hw/virtio/virtio-net.h | 2 +- include/migration/misc.h | 6 +++--- migration/migration.c | 16 ++++++++-------- net/vhost-vdpa.c | 6 ++++-- ui/spice-core.c | 8 +++++--- 8 files changed, 28 insertions(+), 20 deletions(-)