On 2013-07-30 14:21, Igor Mammedov wrote:
> On Tue, 30 Jul 2013 13:00:40 +0200
> Jan Kiszka <jan.kis...@siemens.com> 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
