Re: [PATCH v4 1/3] s390x: fix memleaks in cpu_finalize

[Pan Nengyuan]

On 3/5/2020 4:34 PM, David Hildenbrand wrote:
> 
>>  #if !defined(CONFIG_USER_ONLY)
>>      MachineState *ms = MACHINE(qdev_get_machine());
>>      unsigned int max_cpus = ms->smp.max_cpus;
>> +
>> +    cpu->env.tod_timer =
>> +        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
>> +    cpu->env.cpu_timer =
>> +        timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
>> +
>>      if (cpu->env.core_id >= max_cpus) {
>>          error_setg(&err, "Unable to add CPU with core-id: %" PRIu32
>>                     ", maximum core-id: %d", cpu->env.core_id,
>> @@ -224,9 +230,38 @@ static void s390_cpu_realizefn(DeviceState *dev, Error 
>> **errp)
>>  
>>      scc->parent_realize(dev, &err);
>>  out:
>> +    if (cpu->env.tod_timer) {
>> +        timer_del(cpu->env.tod_timer);
>> +    }
>> +    if (cpu->env.cpu_timer) {
>> +        timer_del(cpu->env.cpu_timer);
>> +    }
>> +    timer_free(cpu->env.tod_timer);
>> +    timer_free(cpu->env.cpu_timer);
> 
> timer_free() should be sufficient, as it cannot be running, no?

Yes, it's redundant.

> 
>>      error_propagate(errp, err);
>>  }
>>  
>> +static void s390_cpu_unrealizefn(DeviceState *dev, Error **errp)
>> +{
>> +    S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
>> +    Error *err = NULL;
>> +
>> +#if !defined(CONFIG_USER_ONLY)
>> +    S390CPU *cpu = S390_CPU(dev);
>> +
>> +    timer_del(cpu->env.tod_timer);
>> +    timer_del(cpu->env.cpu_timer);
>> +    timer_free(cpu->env.tod_timer);
>> +    timer_free(cpu->env.cpu_timer);
>> +#endif
>> +
>> +    scc->parent_unrealize(dev, &err);
>> +    if (err != NULL) {
>> +        error_propagate(errp, err);
>> +        return;
>> +    }
>> +}
> 
> Simply a
> 
> scc->parent_unrealize(dev, errp) and you can drop the temporary variable.

Fine, it looks more clear and I will change it.
And I think it's the same on x86_cpu_unrealize/ppc_cpu_unrealize, I refer to 
the implement of them.

Thanks.

> 
>