Am 10.03.2015 um 22:57 schrieb Eduardo Habkost:
> Instead of passing icc_bridge from the PC initialization code to
> cpu_x86_create(), make the PC initialization code attach the CPU to
> icc_bridge.
>
> The only difference here is that icc_bridge attachment will now be done
> after x86_cpu_parse_featurestr() is called. But this shouldn't make any
> difference, as property setters shouldn't depend on icc_bridge.
>
> Signed-off-by: Eduardo Habkost <[email protected]>
> ---
> Changes v1 -> v2:
> * Keep existing check for NULL icc_bridge and error reporting, instead
> of assing assert(icc_bridge)
> ---
> hw/i386/pc.c | 13 +++++++++++--
> target-i386/cpu.c | 14 ++------------
> target-i386/cpu.h | 3 +--
> 3 files changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index b5b2aad..a26e0ec 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -992,18 +992,27 @@ void pc_acpi_smi_interrupt(void *opaque, int irq, int
> level)
> static X86CPU *pc_new_cpu(const char *cpu_model, int64_t apic_id,
> DeviceState *icc_bridge, Error **errp)
> {
> - X86CPU *cpu;
> + X86CPU *cpu = NULL;
> Error *local_err = NULL;
>
> - cpu = cpu_x86_create(cpu_model, icc_bridge, &local_err);
> + if (icc_bridge == NULL) {
> + error_setg(&local_err, "Invalid icc-bridge value");
> + goto out;
> + }
> +
> + cpu = cpu_x86_create(cpu_model, &local_err);
We had previously discussed reference counting. Here I would expect:
OBJECT(cpu)->ref == 1
> if (local_err != NULL) {
> error_propagate(errp, local_err);
> return NULL;
> }
>
> + qdev_set_parent_bus(DEVICE(cpu), qdev_get_child_bus(icc_bridge, "icc"));
OBJECT(cpu)->ref == 2
> + object_unref(OBJECT(cpu));
OBJECT(cpu)->ref == 1
> +
> object_property_set_int(OBJECT(cpu), apic_id, "apic-id", &local_err);
> object_property_set_bool(OBJECT(cpu), true, "realized", &local_err);
OBJECT(cpu)->ref == 1 or 2 depending on DeviceClass::realize :)
>
> +out:
> if (local_err) {
> error_propagate(errp, local_err);
> object_unref(OBJECT(cpu));
object_unref(NULL) looks unusual but is valid.
Should we change the return NULL to jump here, too, then?
OBJECT(cpu)->ref == 0 or 1
I wonder whether we need another object_unref(OBJECT(cpu)) for the
non-error case, either here or in the callers? Out of scope for this
patch, of course.
Regards,
Andreas
[snip]
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu,
Graham Norton; HRB 21284 (AG Nürnberg)
