Re: [PATCH] accel: remove dead statement and useless assertion

["Alex Bennée" <alex.bennee@linaro.org>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> ops is assigned again just below, and the result of the assignment must
> be non-NULL.
>
> Originally, the check for NULL was meant to be a check for the existence
> of the ops class:
>
>     ops = ACCEL_OPS_CLASS(object_class_by_name(ops_name));
>     ...
>     g_assert(ops != NULL);
>
> (where the ops assignment begot the one that I am removing); but this is
> meaningless now that oc is checked to be non-NULL before ops is assigned
> (commit 5141e9a23fc, "accel: abort if we fail to load the accelerator
> plugin", 2022-11-06).
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>


Although we could further clean-up with:

modified   accel/accel-system.c
@@ -65,7 +65,7 @@ void accel_setup_post(MachineState *ms)
 void accel_system_init_ops_interfaces(AccelClass *ac)
 {
     const char *ac_name;
-    char *ops_name;
+    g_autofree char *ops_name = NULL;
     ObjectClass *oc;
     AccelOpsClass *ops;
 
@@ -79,7 +79,6 @@ void accel_system_init_ops_interfaces(AccelClass *ac)
         error_report("fatal: could not load module for type '%s'", ops_name);
         exit(1);
     }
-    g_free(ops_name);
     ops = ACCEL_OPS_CLASS(oc);
     /*
      * all accelerators need to define ops, providing at least a mandatory


> ---
>  accel/accel-system.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/accel/accel-system.c b/accel/accel-system.c
> index f6c947dd821..61d689935e1 100644
> --- a/accel/accel-system.c
> +++ b/accel/accel-system.c
> @@ -73,19 +73,17 @@ void accel_system_init_ops_interfaces(AccelClass *ac)
>      g_assert(ac_name != NULL);
>  
>      ops_name = g_strdup_printf("%s" ACCEL_OPS_SUFFIX, ac_name);
> -    ops = ACCEL_OPS_CLASS(module_object_class_by_name(ops_name));
>      oc = module_object_class_by_name(ops_name);
>      if (!oc) {
>          error_report("fatal: could not load module for type '%s'", ops_name);
>          exit(1);
>      }
>      g_free(ops_name);
> -    ops = ACCEL_OPS_CLASS(oc);
>      /*
>       * all accelerators need to define ops, providing at least a mandatory
>       * non-NULL create_vcpu_thread operation.
>       */
> -    g_assert(ops != NULL);
> +    ops = ACCEL_OPS_CLASS(oc);
>      if (ops->ops_init) {
>          ops->ops_init(ops);
>      }

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro