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

[Alberto Faria]

On Wed, Aug 3, 2022 at 11:46 AM Dr. David Alan Gilbert
<dgilbert@redhat.com> 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.
>
> Hmm.

I guess this generalizes to the bigger question of whether a global
"return-value-never-used" check makes sense and brings value. Maybe
there are too many cases where it would be preferable to keep the
return value for consistency? Maybe they're not that many and could be
tagged with __attribute__((unused))?

But in this particular case, perhaps we could drop the Error **errp
parameter and directly pass &error_fatal to migrate_params_check() and
migrate_caps_check().

Alberto