Re: [PATCH V2 03/11] migration: convert to NotifierWithReturn

[Peter Xu]

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.

-- 
Peter Xu