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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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.

Dave

>      MigrationCapabilityStatusList *head = NULL;
>      /* Assuming all off */
> -    bool cap_list[MIGRATION_CAPABILITY__MAX] = { 0 }, ret;
> +    bool cap_list[MIGRATION_CAPABILITY__MAX] = { 0 };
>      int i;
>  
>      if (!migrate_params_check(&ms->parameters, errp)) {
> -        return false;
> +        return;
>      }
>  
>      for (i = 0; i < MIGRATION_CAPABILITY__MAX; i++) {
> @@ -4502,12 +4502,10 @@ static bool migration_object_check(MigrationState *ms, Error **errp)
>          }
>      }
>  
> -    ret = migrate_caps_check(cap_list, head, errp);
> +    migrate_caps_check(cap_list, head, errp);
>  
>      /* It works with head == NULL */
>      qapi_free_MigrationCapabilityStatusList(head);
> -
> -    return ret;
>  }
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK