From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35784) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abXQK-0003vN-R7 for qemu-devel@nongnu.org; Thu, 03 Mar 2016 12:52:29 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abXQG-0007u6-O7 for qemu-devel@nongnu.org; Thu, 03 Mar 2016 12:52:28 -0500 Received: from e31.co.us.ibm.com ([32.97.110.149]:59262) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abXQG-0007tx-Hj for qemu-devel@nongnu.org; Thu, 03 Mar 2016 12:52:24 -0500 Received: from localhost by e31.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 3 Mar 2016 10:52:22 -0700 Received: from b01cxnp22034.gho.pok.ibm.com (b01cxnp22034.gho.pok.ibm.com [9.57.198.24]) by d03dlp02.boulder.ibm.com (Postfix) with ESMTP id 19E513E40047 for ; Thu, 3 Mar 2016 10:52:19 -0700 (MST) Received: from d01av05.pok.ibm.com (d01av05.pok.ibm.com [9.56.224.195]) by b01cxnp22034.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u23HqIjf31064244 for ; Thu, 3 Mar 2016 17:52:18 GMT Received: from d01av05.pok.ibm.com (localhost [127.0.0.1]) by d01av05.pok.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u23Hm42u031022 for ; Thu, 3 Mar 2016 12:48:04 -0500 References: <1456866806-31466-1-git-send-email-mjrosato@linux.vnet.ibm.com> <1456866806-31466-6-git-send-email-mjrosato@linux.vnet.ibm.com> <20160302085716.7874dd8d@thinkpad-w530> From: Matthew Rosato Message-ID: <56D879D1.7050509@linux.vnet.ibm.com> Date: Thu, 3 Mar 2016 12:52:17 -0500 MIME-Version: 1.0 In-Reply-To: <20160302085716.7874dd8d@thinkpad-w530> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 5/6] s390x/cpu: Add error handling to cpu creation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Hildenbrand Cc: imammedo@redhat.com, qemu-devel@nongnu.org, agraf@suse.de, borntraeger@de.ibm.com, bharata@linux.vnet.ibm.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, afaerber@suse.de, rth@twiddle.net >> +S390CPU *s390_new_cpu(MachineState *machine, int64_t id, Error **errp) >> +{ >> + S390CPU *cpu = NULL; >> + Error *local_err = NULL; > > Think the naming schema is "err" now. > >> + >> + if (id >= max_cpus) { >> + error_setg(errp, "Unable to add CPU: %" PRIi64 >> + ", max allowed: %d", id, max_cpus - 1); >> + goto out; > > Could we also move this check to the realize function? > >> + } >> + >> + cpu = cpu_s390x_create(machine->cpu_model, &local_err); >> + if (local_err != NULL) { >> + goto out; >> + } >> + >> + object_property_set_int(OBJECT(cpu), id, "id", &local_err); > > We should add a check in between > > if (err) { > goto out; > } > >> + object_property_set_bool(OBJECT(cpu), true, "realized", &local_err); >> + >> +out: >> + if (cpu != NULL) { >> + object_unref(OBJECT(cpu)); > > Is the object_unref() here correct? > I know that we have one reference from VCPU creation. Where does the second one > come from (is it from the hotplug handler? then I'd prefer a comment here :D ) > After some digging, I believe this unref is not necessary for s390 (bus-less) and I'm now questioning the i386 code that I used as a base... @Igor/Andreas: In i386, looks like the unrefs were due to the ref created when adding the cpu to the icc bus. Andreas moved the checks outside of pc_new_cpu and explains their purpose here: 0e3bd562 - pc: Ensure non-zero CPU ref count after attaching to ICC bus But then a subsequent patch removed the bus and left the unrefs: 46232aaa - cpu/apic: drop icc bus/bridge Should that patch not have also dropped the unrefs in pc_hot_add_cpu() and pc_cpus_init()? Matt