Re: [Qemu-devel] Error handling in cpu_x86_create

[Jan Kiszka]

On 2013-07-30 14:21, Igor Mammedov wrote:
> On Tue, 30 Jul 2013 13:00:40 +0200
> Jan Kiszka <address@hidden> wrote:
> 
>> Hi Igor,
>>
>> just noticed by chance that error handling in cpu_x86_create is likely
>> broken after your changes. Any error after cpu = ...object_new() will
>> not properly release the CPU object again nor report NULL to the caller.
>> That means errors should slip through, no? And cpu_x86_init looks similar.
> Failure of object_new() in cpu_x86_create() is not checked since it should
> never fail.
> 
> As for following error handling caller of cpu_x86_create(..., errp) should
> use errp to check for errors and release failed cpu.
> 
> cpu_x86_init() that calls it has:
> ===
> out:
>     if (error) {
>         fprintf(stderr, "%s\n", error_get_pretty(error));
>         error_free(error);
>         if (cpu != NULL) {
>             object_unref(OBJECT(cpu));
> ===
> similar code-path exists in pc_new_cpu()

Not truly:

    cpu = cpu_x86_create(cpu_model, icc_bridge, errp);
    if (!cpu) {
        return cpu;
    }

    object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
    object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);

    if (local_err) {
        if (cpu != NULL) {
            object_unref(OBJECT(cpu));
            cpu = NULL;
        }


That's what made me think the error is supposed to be derived from cpu
== NULL, rather than from errp. So the actual uncleanness is here: errp
should be checked instead of cpu, right?

Jan

> 
> end goal is to replace cpu_x86_create() with just object_new(FOO_CPU),
> but that needs to x86 CPU subclasses and properties in place so
> we could remove/isolate legacy stuff to legacy hook.
> 
> Did you hit any particular problem with current code?
> 
>>
>> Jan
>>
> 

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux