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

[Matthew Rosato <mjrosato@linux.vnet.ibm.com>]

>> +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