From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39615) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abm7H-0005Sj-BN for qemu-devel@nongnu.org; Fri, 04 Mar 2016 04:33:48 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abm7C-0002sh-CA for qemu-devel@nongnu.org; Fri, 04 Mar 2016 04:33:47 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58070) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abm7C-0002sd-6C for qemu-devel@nongnu.org; Fri, 04 Mar 2016 04:33:42 -0500 Date: Fri, 4 Mar 2016 10:33:37 +0100 From: Igor Mammedov Message-ID: <20160304103337.4f95fcd9@nial.brq.redhat.com> In-Reply-To: <56D879D1.7050509@linux.vnet.ibm.com> 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> <56D879D1.7050509@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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: Matthew Rosato Cc: borntraeger@de.ibm.com, qemu-devel@nongnu.org, agraf@suse.de, David Hildenbrand , bharata@linux.vnet.ibm.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, afaerber@suse.de, rth@twiddle.net On Thu, 3 Mar 2016 12:52:17 -0500 Matthew Rosato wrote: > >> +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()? nope, bus made it own ref, nref is needed here to avoid leaking object as device_realize() implicitly adds it to /machine/devices/unattached/ creating an extra ref along the way. > > Matt >