From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50297) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abCn9-00078r-Cx for qemu-devel@nongnu.org; Wed, 02 Mar 2016 14:50:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1abCn6-00062T-5o for qemu-devel@nongnu.org; Wed, 02 Mar 2016 14:50:39 -0500 Received: from e33.co.us.ibm.com ([32.97.110.151]:49177) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1abCn5-00062B-Rp for qemu-devel@nongnu.org; Wed, 02 Mar 2016 14:50:36 -0500 Received: from localhost by e33.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 2 Mar 2016 12:50:34 -0700 Received: from b03cxnp08026.gho.boulder.ibm.com (b03cxnp08026.gho.boulder.ibm.com [9.17.130.18]) by d03dlp02.boulder.ibm.com (Postfix) with ESMTP id C84D63E40060 for ; Wed, 2 Mar 2016 12:50:31 -0700 (MST) Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by b03cxnp08026.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u22JoVUx39649532 for ; Wed, 2 Mar 2016 12:50:31 -0700 Received: from d03av02.boulder.ibm.com (localhost [127.0.0.1]) by d03av02.boulder.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u22JoTTg002557 for ; Wed, 2 Mar 2016 12:50:31 -0700 References: <1456866806-31466-1-git-send-email-mjrosato@linux.vnet.ibm.com> <1456866806-31466-6-git-send-email-mjrosato@linux.vnet.ibm.com> <20160302085716.7874dd8d@thinkpad-w530> From: Matthew Rosato Message-ID: <56D74400.5020308@linux.vnet.ibm.com> Date: Wed, 2 Mar 2016 14:50:24 -0500 MIME-Version: 1.0 In-Reply-To: <20160302085716.7874dd8d@thinkpad-w530> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v7 5/6] s390x/cpu: Add error handling to cpu creation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Hildenbrand Cc: imammedo@redhat.com, qemu-devel@nongnu.org, agraf@suse.de, borntraeger@de.ibm.com, bharata@linux.vnet.ibm.com, cornelia.huck@de.ibm.com, pbonzini@redhat.com, afaerber@suse.de, rth@twiddle.net >> +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