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