Re: [RFC v2 02/10] Drop unused static function return values

[Daniel P . Berrangé]

On Wed, Aug 03, 2022 at 11:46:26AM +0100, Dr. David Alan Gilbert wrote:
> * Alberto Faria (afaria@redhat.com) wrote:
> > Make non-void static functions whose return values are ignored by
> > all callers return void instead.
> > 
> > These functions were found by static-analyzer.py.
> > 
> > Not all occurrences of this problem were fixed.
> > 
> > Signed-off-by: Alberto Faria <afaria@redhat.com>
> 
> <snip>
> 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index e03f698a3c..4698080f96 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -175,7 +175,7 @@ static MigrationIncomingState *current_incoming;
> >  
> >  static GSList *migration_blockers;
> >  
> > -static bool migration_object_check(MigrationState *ms, Error **errp);
> > +static void migration_object_check(MigrationState *ms, Error **errp);
> >  static int migration_maybe_pause(MigrationState *s,
> >                                   int *current_active_state,
> >                                   int new_state);
> > @@ -4485,15 +4485,15 @@ static void migration_instance_init(Object *obj)
> >   * Return true if check pass, false otherwise. Error will be put
> >   * inside errp if provided.
> >   */
> > -static bool migration_object_check(MigrationState *ms, Error **errp)
> > +static void migration_object_check(MigrationState *ms, Error **errp)
> >  {
> 
> I'm not sure if this is a good change.
> Where we have a function that returns an error via an Error ** it's
> normal practice for us to return a bool to say whether it generated an
> error.
> 
> Now, in our case we only call it with error_fatal:
> 
>     migration_object_check(current_migration, &error_fatal);
> 
> so the bool isn't used/checked.
> 
> So I'm a bit conflicted:
> 
>   a) Using error_fatal is the easiest way to handle this function
>   b) Things taking Error ** normally do return a flag value
>   c) But it's not used in this case.

Yep, migration_object_check is following our recommended design
pattern for error reporting. It is valid to either check the
return value, or pass error_fatal / error_abort.

The fact that no /current/ callers happen to check the return
value is not a reason to make it 'void'.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|