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
> > <dgilb...@redhat.com> 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.
> > >
> > > 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 :|
