[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: |
David Hildenbrand |
Subject: |
Re: [Qemu-devel] [PATCH v7 5/6] s390x/cpu: Add error handling to cpu creation |
Date: |
Thu, 3 Mar 2016 08:47:59 +0100 |
> >> +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
- [Qemu-devel] [PATCH v7 0/6] Allow hotplug of s390 CPUs, Matthew Rosato, 2016/03/01
- [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