On Wed, Aug 03, 2022 at 11:46:26AM +0100, Dr. David Alan Gilbert wrote:
> * Alberto Faria (afa...@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 <afa...@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 :|
