From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:60062)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <jan.kiszka@siemens.com>) id 1V5HRd-0002K2-CS
	for qemu-devel@nongnu.org; Fri, 02 Aug 2013 11:39:16 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <jan.kiszka@siemens.com>) id 1V5HRX-0001qF-Fq
	for qemu-devel@nongnu.org; Fri, 02 Aug 2013 11:39:09 -0400
Received: from thoth.sbs.de ([192.35.17.2]:15577)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <jan.kiszka@siemens.com>) id 1V5HRX-0001po-6i
	for qemu-devel@nongnu.org; Fri, 02 Aug 2013 11:39:03 -0400
Message-ID: <51FBD294.3060704@siemens.com>
Date: Fri, 02 Aug 2013 17:39:00 +0200
From: Jan Kiszka <jan.kiszka@siemens.com>
MIME-Version: 1.0
References: <51F79CD8.7070807@siemens.com>
	<20130730142143.10a5734d@nial.usersys.redhat.com>
In-Reply-To: <20130730142143.10a5734d@nial.usersys.redhat.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Subject: Re: [Qemu-devel] Error handling in cpu_x86_create
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Igor Mammedov <imammedo@redhat.com>
Cc: qemu-devel <qemu-devel@nongnu.org>

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