Re: [Qemu-devel] [PATCH v7 5/6] s390x/cpu: Add error handling to cpu creation

[Igor Mammedov <imammedo@redhat.com>]

On Thu, 3 Mar 2016 12:52:17 -0500
Matthew Rosato <mjrosato@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
>