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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Aug 03, 2022 at 12:15:20PM +0100, Richard W.M. Jones wrote:
> On Wed, Aug 03, 2022 at 12:07:19PM +0100, Alberto Faria wrote:
> > 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().
> 
> If it helps to think about this, Coverity checks for consistency.
> Across the whole code base, is the return value of a function used or
> ignored consistently.  You will see Coverity errors like:

> What it's saying is that in this code base, nbd_poll's return value
> was checked by the caller 5 out of 6 times, but ignored here.  (This
> turned out to be a real bug which we fixed).
>
> It seems like the check implemented in your patch is: If the return
> value is used 0 times anywhere in the code base, change the return
> value to 'void'.  Coverity would not flag this.
> 
> Maybe a consistent use check is better?

Inconsistent return value checking is designed-in behaviour for
QEMU's current Error handling coding pattern with error_abort/fatal.

If we wanted to make it consistent we would need to require that
all methods with 'Error **errp' are tagged 'attribute(unused)'
and then provide

  # define ignore_value(x) \
     (__extension__ ({ __typeof__ (x) __x = (x); (void) __x; }))

Such that if you want to use  error_abort/error_fatal, you
need to explicitly discard the return value eg

   ignore_value(some_method(&error_abort));

If I was starting QEMU fresh, I think like the attribute(unused)
anntation and explicit discard, but to retrofit it now would
require updated about 3000 current callers which pass &error_abort
and &error_fatal.

On the flipside I am willing to bet that doing this work would
identify existing bugs where we don't pass error_abort/fatal
and also fail to check for failure. So there would be potential
payback for the vast churn

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 :|