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

[Steven Sistare]

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