From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46536) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VLb0o-0001y4-MJ for qemu-devel@nongnu.org; Mon, 16 Sep 2013 11:46:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VLb0g-0000LN-P3 for qemu-devel@nongnu.org; Mon, 16 Sep 2013 11:46:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35546) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VLb0g-0000L9-Gh for qemu-devel@nongnu.org; Mon, 16 Sep 2013 11:46:46 -0400 Date: Mon, 16 Sep 2013 18:48:51 +0300 From: "Michael S. Tsirkin" Message-ID: <20130916154851.GB4141@redhat.com> References: <20130915172331.GA2821@redhat.com> <5236A5ED.3000804@suse.de> <20130916123303.GA2992@redhat.com> <523722BE.90202@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <523722BE.90202@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 05:24:46PM +0200, Andreas F=E4rber wrote: > Am 16.09.2013 14:33, schrieb Michael S. Tsirkin: > > 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 t= he > >>> common case where the value is a constant. > >>> > >>> Signed-off-by: Michael S. Tsirkin > >>> --- > >>> > >>> I'm using this patch in my acpi work - any objections > >>> to applying it on my tree? > >> > >> Actually yes: Apart from the clang issues raised and the disturbing > >> upper-casing of arguments, this is hardcoding "int" type and NULL er= rp, > >> so I don't think it deserves to live in object.h as is. I do agree t= hat > >> we could use more helper functions to deal with dynamic properties. > >> > >> So what about taking bool/string property helpers as example and put= ting > >> 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 poi= nter > >> 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. > >=20 > > This reminds me. > > [mst@robin qemu]$ git grep object_property_set_bool > > backends/rng.c: object_property_set_bool(OBJECT(s), true, "opened"= , errp); > > backends/tpm.c: object_property_set_bool(OBJECT(s), true, "opened"= , errp); >=20 > These look like two distinct properties used once each. >=20 > [...] > > hw/core/qdev.c: object_property_set_bool(OBJECT(dev), true, "reali= zed", &local_err); > [...] > > hw/core/qdev.c: object_property_set_bool(obj, false, "realized= ", NULL); > > hw/i386/pc.c: object_property_set_bool(OBJECT(cpu), true, "realize= d", &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_d= ev), true, "realized", errp) > > hw/scsi/scsi-bus.c: object_property_set_bool(OBJECT(dev), true, "r= ealized", &err); > [...] > > target-alpha/cpu.c: object_property_set_bool(OBJECT(cpu), true, "r= ealized", NULL); > > target-arm/helper.c: object_property_set_bool(OBJECT(cpu), true, "= realized", NULL); > > target-cris/cpu.c: object_property_set_bool(OBJECT(cpu), true, "re= alized", NULL); > > target-i386/cpu.c: object_property_set_bool(OBJECT(cpu), true,= "pmu", &err); > > target-i386/cpu.c: object_property_set_bool(OBJECT(cpu), true, "re= alized", &error); > > target-lm32/helper.c: object_property_set_bool(OBJECT(cpu), true, = "realized", NULL); > > target-m68k/helper.c: object_property_set_bool(OBJECT(cpu), true, = "realized", NULL); > > target-microblaze/translate.c: object_property_set_bool(OBJECT(cpu= ), true, "realized", NULL); > > target-mips/translate.c: object_property_set_bool(OBJECT(cpu), tru= e, "realized", NULL); > > target-moxie/cpu.c: object_property_set_bool(OBJECT(cpu), true, "r= ealized", NULL); > > target-openrisc/cpu.c: object_property_set_bool(OBJECT(cpu), true,= "realized", NULL); > > target-ppc/translate_init.c: object_property_set_bool(OBJECT(cpu),= true, "realized", &err); > > target-s390x/helper.c: object_property_set_bool(OBJECT(cpu), true,= "realized", NULL); > > target-sh4/cpu.c: object_property_set_bool(OBJECT(cpu), true, "rea= lized", NULL); > > target-sparc/cpu.c: object_property_set_bool(OBJECT(cpu), true, "r= ealized", NULL); > > target-unicore32/helper.c: object_property_set_bool(OBJECT(cpu), t= rue, "realized", NULL); > > target-xtensa/helper.c: object_property_set_bool(OBJECT(cpu), true= , "realized", NULL); >=20 > Leaving the bulk for "realized". >=20 > >=20 > > Shouldn't we have a constant for the "realized" string? >=20 > That's a two-sided sword: We actually shouldn't be setting realized =3D > true manually but once on machine init - in that case we wouldn't > strictly need a constant. >=20 > I pushed to get that central infrastructure in place to spare me/us the > repetitive realized =3D true setting, but Paolo shot it down, asking fo= r a > full-fledged solver to make ordering guarantees. >=20 > > If there's a typo somewhere it will all fail at runtime > > in a hard to debug way, won't it? >=20 > It would. However, this is typically executed once on startup, so with > proper error handling we should notice this immediately. My qom-test > (that Anthony didn't take for 1.6 and I still need to respin) served to > test them, with focus on SysBusDevices. >=20 > You are cordially invited to add trivial qtests covering instantiation > of PCI/virtio devices you care about. :) >=20 > Andreas http://sweng.the-davies.net/Home/rustys-api-design-manifesto Even then: it will be at best "5. Do it right or it will always break at runtime." We need to switch to APIs at "9. The compiler/linker won't let you get it wrong." > --=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