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

[David Hildenbrand]

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

If we care about malicious users, wanting to set id's after realize that is
true. But I am no QOM expert and don't know if that is a scenarios that
has to be taken care of. But as I see similar code for other properties,
I assume we are better off doing it also that way.

David