[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 5/6] s390x/cpu: Add error handling to cpu cre
From: |
Matthew Rosato |
Subject: |
Re: [Qemu-devel] [PATCH v7 5/6] s390x/cpu: Add error handling to cpu creation |
Date: |
Thu, 3 Mar 2016 12:52:17 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 |
>> +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
- [Qemu-devel] [PATCH v7 1/6] s390x/cpu: Cleanup init in preparation for hotplug, (continued)
- [Qemu-devel] [PATCH v7 1/6] s390x/cpu: Cleanup init in preparation for hotplug, Matthew Rosato, 2016/03/01
- [Qemu-devel] [PATCH v7 4/6] s390x/cpu: Add CPU property links, Matthew Rosato, 2016/03/01
- [Qemu-devel] [PATCH v7 2/6] s390x/cpu: Set initial CPU state in common routine, Matthew Rosato, 2016/03/01
- [Qemu-devel] [PATCH v7 6/6] s390x/cpu: Allow hotplug of CPUs, Matthew Rosato, 2016/03/01
- [Qemu-devel] [PATCH v7 5/6] s390x/cpu: Add error handling to cpu creation, Matthew Rosato, 2016/03/01
- Re: [Qemu-devel] [PATCH v7 5/6] s390x/cpu: Add error handling to cpu creation, David Hildenbrand, 2016/03/02
[Qemu-devel] [PATCH v7 3/6] s390x/cpu: Move some CPU initialization into realize, Matthew Rosato, 2016/03/01