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

[Matthew Rosato]

>> +static void s390_cpu_get_id(Object *obj, Visitor *v, const char *name,
>> +                            void *opaque, Error **errp)
>> +{
>> +    S390CPU *cpu = S390_CPU(obj);
>> +    int64_t value = cpu->id;
>> +
>> +    visit_type_int(v, name, &value, errp);
>> +}
>> +
>> +static void s390_cpu_set_id(Object *obj, Visitor *v, const char *name,
>> +                            void *opaque, Error **errp)
>> +{
>> +    S390CPU *cpu = S390_CPU(obj);
>> +    DeviceState *dev = DEVICE(obj);
>> +    const int64_t min = 0;
>> +    const int64_t max = UINT32_MAX;
>> +    Error *local_err = NULL;
>> +    int64_t value;
>> +
>> +    if (dev->realized) {
>> +        error_setg(errp, "Attempt to set property '%s' on '%s' after "
>> +                   "it was realized", name, object_get_typename(obj));
>> +        return;
>> +    }
>> +
>> +    visit_type_int(v, name, &value, &local_err);
>> +    if (local_err) {
>> +        error_propagate(errp, local_err);
>> +        return;
>> +    }
>> +    if (value < min || value > max) {
>> +        error_setg(errp, "Property %s.%s doesn't take value %" PRId64
>> +                   " (minimum: %" PRId64 ", maximum: %" PRId64 ")" ,
>> +                   object_get_typename(obj), name, value, min, max);
>> +        return;
>> +    }
>> +    if ((value != cpu->id) && cpu_exists(value)) {
>> +        error_setg(errp, "CPU with ID %" PRIi64 " exists", value);
>> +        return;
>> +    }
>> +    cpu->id = value;
>> +}
> 
> Just curious, what about using a simple
> 
> object_property_set_int() and doing all the checks in realize() ?
> 
> Then we could live without manual getter/setter (and without the realize 
> check).
> 

I think we still need at least a manual setter, even if you want to move
the checks to realize.

See something like object_property_add_uint64_ptr() -- It sets a
boilerplate get routine, and no set routine -- I think this presumes you
set your property upfront (at add time), never change it for the life of
the object, but want to read it later.
By comparison, S390CPU.id is set sometime after instance_init, based on
input.

So, we call object_property_set_int() to update it --  This just passes
the provided int value to the setter routine associated with the
property.  If one doesn't exist, you get:
qemu: Insufficient permission to perform this operation

I think this is also why we want to check for dev->realized in the
setter routine, to make sure the property is not being changed "too
late" -- Once the cpu is realized, the ID is baked and can't be changed.

Or did I misunderstand your idea here?

Matt