On Thu, 3 Mar 2016 12:52:17 -0500
Matthew Rosato <mjros...@linux.vnet.ibm.com> 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
>
