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

[Richard W.M. Jones]

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:

      Error: CHECKED_RETURN (CWE-252): [#def37]
      libnbd-1.12.5/fuse/operations.c:180: check_return: Calling "nbd_poll" 
without checking return value (as is done elsewhere 5 out of 6 times).
      libnbd-1.12.5/examples/aio-connect-read.c:96: example_checked: Example 1: 
"nbd_poll(nbd, -1)" has its value checked in "nbd_poll(nbd, -1) == -1".
      libnbd-1.12.5/examples/aio-connect-read.c:128: example_checked: Example 
2: "nbd_poll(nbd, -1)" has its value checked in "nbd_poll(nbd, -1) == -1".
      libnbd-1.12.5/examples/strict-structured-reads.c:246: example_checked: 
Example 3: "nbd_poll(nbd, -1)" has its value checked in "nbd_poll(nbd, -1) == 
-1".
      libnbd-1.12.5/ocaml/nbd-c.c:2599: example_assign: Example 4: Assigning: 
"r" = return value from "nbd_poll(h, timeout)".
      libnbd-1.12.5/ocaml/nbd-c.c:2602: example_checked: Example 4 (cont.): "r" 
has its value checked in "r == -1".
      libnbd-1.12.5/python/methods.c:2806: example_assign: Example 5: 
Assigning: "ret" = return value from "nbd_poll(h, timeout)".
      libnbd-1.12.5/python/methods.c:2808: example_checked: Example 5 (cont.): 
"ret" has its value checked in "ret == -1".
      #  178|       /* Dispatch work while there are commands in flight. */
      #  179|       while (thread->in_flight > 0)
      #  180|->       nbd_poll (h, -1);
      #  181|     }
      #  182|

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?

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org