From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48916) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VLXxH-0002rC-Pg for qemu-devel@nongnu.org; Mon, 16 Sep 2013 08:31:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VLXxB-0004T5-Kz for qemu-devel@nongnu.org; Mon, 16 Sep 2013 08:31:03 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16005) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VLXxB-0004Sw-CY for qemu-devel@nongnu.org; Mon, 16 Sep 2013 08:30:57 -0400 Date: Mon, 16 Sep 2013 15:33:03 +0300 From: "Michael S. Tsirkin" Message-ID: <20130916123303.GA2992@redhat.com> References: <20130915172331.GA2821@redhat.com> <5236A5ED.3000804@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <5236A5ED.3000804@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: Paolo Bonzini , Stefan Hajnoczi , qemu-devel@nongnu.org, Anthony Liguori , Igor Mammedov On Mon, Sep 16, 2013 at 08:32:13AM +0200, Andreas F=E4rber wrote: > Am 15.09.2013 19:23, schrieb Michael S. Tsirkin: > > Add a helper macro for adding read-only properties, that works in the > > common case where the value is a constant. > >=20 > > Signed-off-by: Michael S. Tsirkin > > --- > >=20 > > I'm using this patch in my acpi work - any objections > > to applying it on my tree? >=20 > Actually yes: Apart from the clang issues raised and the disturbing > upper-casing of arguments, this is hardcoding "int" type and NULL errp, > so I don't think it deserves to live in object.h as is. I do agree that > we could use more helper functions to deal with dynamic properties. >=20 > So what about taking bool/string property helpers as example and puttin= g > intX_t getters into object.c, using a passed-through opaque argument to > obtain the value? We could then have real object_property_add_int32() > etc. functions using the appropriate type name, with field/value pointe= r > and Error** arguments. A pointer can be assumed to hold up to uint32_t > values or, to keep the API more general, use a local static const > variable for non-field values. This reminds me. [mst@robin qemu]$ git grep object_property_set_bool backends/rng.c: object_property_set_bool(OBJECT(s), true, "opened", er= rp); backends/tpm.c: object_property_set_bool(OBJECT(s), true, "opened", er= rp); hw/core/qdev-properties.c: object_property_set_bool(OBJECT(dev), value= , name, &errp); hw/core/qdev.c: object_property_set_bool(OBJECT(dev), true, "realized"= , &local_err); hw/core/qdev.c: object_property_set_bool(obj, prop->defval, prop->= name, &local_err); hw/core/qdev.c: object_property_set_bool(obj, false, "realized", N= ULL); hw/i386/pc.c: object_property_set_bool(OBJECT(cpu), true, "realized", = &local_err); hw/pci-host/prep.c: object_property_set_bool(OBJECT(&s->pci_dev), true= , "realized", errp); hw/pci-host/versatile.c: object_property_set_bool(OBJECT(&s->pci_dev),= true, "realized", errp) hw/scsi/scsi-bus.c: object_property_set_bool(OBJECT(dev), true, "reali= zed", &err); include/qom/object.h: * object_property_set_bool: include/qom/object.h:void object_property_set_bool(Object *obj, bool valu= e, qom/object.c:void object_property_set_bool(Object *obj, bool value, target-alpha/cpu.c: object_property_set_bool(OBJECT(cpu), true, "reali= zed", NULL); target-arm/helper.c: object_property_set_bool(OBJECT(cpu), true, "real= ized", NULL); target-cris/cpu.c: object_property_set_bool(OBJECT(cpu), true, "realiz= ed", NULL); target-i386/cpu.c: object_property_set_bool(OBJECT(cpu), true, "pm= u", &err); target-i386/cpu.c: object_property_set_bool(OBJECT(cpu), true, "realiz= ed", &error); target-lm32/helper.c: object_property_set_bool(OBJECT(cpu), true, "rea= lized", NULL); target-m68k/helper.c: object_property_set_bool(OBJECT(cpu), true, "rea= lized", NULL); target-microblaze/translate.c: object_property_set_bool(OBJECT(cpu), t= rue, "realized", NULL); target-mips/translate.c: object_property_set_bool(OBJECT(cpu), true, "= realized", NULL); target-moxie/cpu.c: object_property_set_bool(OBJECT(cpu), true, "reali= zed", NULL); target-openrisc/cpu.c: object_property_set_bool(OBJECT(cpu), true, "re= alized", NULL); target-ppc/translate_init.c: object_property_set_bool(OBJECT(cpu), tru= e, "realized", &err); target-s390x/helper.c: object_property_set_bool(OBJECT(cpu), true, "re= alized", NULL); target-sh4/cpu.c: object_property_set_bool(OBJECT(cpu), true, "realize= d", NULL); target-sparc/cpu.c: object_property_set_bool(OBJECT(cpu), true, "reali= zed", NULL); target-unicore32/helper.c: object_property_set_bool(OBJECT(cpu), true,= "realized", NULL); target-xtensa/helper.c: object_property_set_bool(OBJECT(cpu), true, "r= ealized", NULL); Shouldn't we have a constant for the "realized" string? If there's a typo somewhere it will all fail at runtime in a hard to debug way, won't it? > It does touch on the issue I brought up on a KVM call a couple weeks ag= o > of how dynamic and static properties are supposed to relate. I > personally welcome making dynamic properties more easy to deal with; an > alternative might be to extend qdev-properties.c with > DEFINE_PROP_READONLY_UINT32() etc. CC'ing Igor, who has dealt with > dynamic-vs.-static properties for X86CPU. >=20 > Regards, > Andreas >=20 > >=20 > > include/qom/object.h | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > >=20 > > diff --git a/include/qom/object.h b/include/qom/object.h > > index 1a7b71a..4787de6 100644 > > --- a/include/qom/object.h > > +++ b/include/qom/object.h > > @@ -17,6 +17,7 @@ > > #include > > #include > > #include > > +#include "qemu/typedefs.h" > > #include "qemu/queue.h" > > =20 > > struct Visitor; > > @@ -792,6 +793,26 @@ void object_property_add(Object *obj, const char= *name, const char *type, > > ObjectPropertyRelease *release, > > void *opaque, struct Error **errp); > > =20 > > +/* Add a property that is an integer constant. */ > > +#define OBJECT_ADD_PROP_CONST(obj, name, value) = \ > > + do { = \ > > + void OBJECT_ADD_PROP_GET(Object *OBJECT_ADD_PROP_OBJ, = \ > > + struct Visitor *OBJECT_ADD_PROP_VIS= ITOR, \ > > + void *OBJECT_ADD_PROP_OPAQUE, = \ > > + const char *OBJECT_ADD_PROP_NAME, = \ > > + struct Error **OBJECT_ADD_PROP_VALU= E_ERR) \ > > + { = \ > > + int64_t OBJECT_ADD_PROP_VALUE =3D value; = \ > > + = \ > > + visit_type_int64(OBJECT_ADD_PROP_VISITOR, = \ > > + &OBJECT_ADD_PROP_VALUE, = \ > > + OBJECT_ADD_PROP_NAME, = \ > > + OBJECT_ADD_PROP_VALUE_ERR); = \ > > + } = \ > > + object_property_add(obj, name, "int", OBJECT_ADD_PROP_GET, = \ > > + NULL, NULL, NULL, NULL); = \ > > + } while (0) > > + > > void object_property_del(Object *obj, const char *name, struct Error= **errp); > > =20 > > /** > >=20 >=20 >=20 > --=20 > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrn= berg