* [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties @ 2013-09-15 17:23 Michael S. Tsirkin 2013-09-15 17:54 ` Peter Maydell 2013-09-16 6:32 ` Andreas Färber 0 siblings, 2 replies; 14+ messages in thread From: Michael S. Tsirkin @ 2013-09-15 17:23 UTC (permalink / raw) To: qemu-devel Cc: Paolo Bonzini, Stefan Hajnoczi, Andreas Färber, Anthony Liguori Add a helper macro for adding read-only properties, that works in the common case where the value is a constant. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- I'm using this patch in my acpi work - any objections to applying it on my tree? include/qom/object.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) 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 <glib.h> #include <stdint.h> #include <stdbool.h> +#include "qemu/typedefs.h" #include "qemu/queue.h" 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); +/* 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_VISITOR, \ + void *OBJECT_ADD_PROP_OPAQUE, \ + const char *OBJECT_ADD_PROP_NAME, \ + struct Error **OBJECT_ADD_PROP_VALUE_ERR) \ + { \ + int64_t OBJECT_ADD_PROP_VALUE = 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); /** -- MST ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties 2013-09-15 17:23 [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties Michael S. Tsirkin @ 2013-09-15 17:54 ` Peter Maydell 2013-09-15 20:06 ` Michael S. Tsirkin 2013-09-16 6:32 ` Andreas Färber 1 sibling, 1 reply; 14+ messages in thread From: Peter Maydell @ 2013-09-15 17:54 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Anthony Liguori, Paolo Bonzini, QEMU Developers, Stefan Hajnoczi, Andreas Färber On 15 September 2013 18:23, Michael S. Tsirkin <mst@redhat.com> wrote: > +/* 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_VISITOR, \ > + void *OBJECT_ADD_PROP_OPAQUE, \ > + const char *OBJECT_ADD_PROP_NAME, \ > + struct Error **OBJECT_ADD_PROP_VALUE_ERR) \ > + { \ > + int64_t OBJECT_ADD_PROP_VALUE = 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) This won't compile in clang, because it doesn't support nested functions. Clang is our primary compiler for MacOS hosts and is useful for various testing scenarios too (like its "integer sanitizing" compile options) so I'd rather we didn't break it... thanks -- PMM ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties 2013-09-15 17:54 ` Peter Maydell @ 2013-09-15 20:06 ` Michael S. Tsirkin 0 siblings, 0 replies; 14+ messages in thread From: Michael S. Tsirkin @ 2013-09-15 20:06 UTC (permalink / raw) To: Peter Maydell Cc: Anthony Liguori, Paolo Bonzini, QEMU Developers, Stefan Hajnoczi, Andreas Färber On Sun, Sep 15, 2013 at 06:54:35PM +0100, Peter Maydell wrote: > On 15 September 2013 18:23, Michael S. Tsirkin <mst@redhat.com> wrote: > > +/* 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_VISITOR, \ > > + void *OBJECT_ADD_PROP_OPAQUE, \ > > + const char *OBJECT_ADD_PROP_NAME, \ > > + struct Error **OBJECT_ADD_PROP_VALUE_ERR) \ > > + { \ > > + int64_t OBJECT_ADD_PROP_VALUE = 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) > > This won't compile in clang, because it doesn't support nested > functions. Clang is our primary compiler for MacOS hosts and > is useful for various testing scenarios too (like its "integer sanitizing" > compile options) so I'd rather we didn't break it... > > thanks > -- PMM Hmm nasty. I guess we can get by using static variables. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties 2013-09-15 17:23 [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties Michael S. Tsirkin 2013-09-15 17:54 ` Peter Maydell @ 2013-09-16 6:32 ` Andreas Färber 2013-09-16 12:33 ` Michael S. Tsirkin 2013-09-16 16:07 ` Michael S. Tsirkin 1 sibling, 2 replies; 14+ messages in thread From: Andreas Färber @ 2013-09-16 6:32 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel, Anthony Liguori, Igor Mammedov 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. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > 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 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. So what about taking bool/string property helpers as example and putting 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 pointer 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. It does touch on the issue I brought up on a KVM call a couple weeks ago 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. Regards, Andreas > > include/qom/object.h | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > 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 <glib.h> > #include <stdint.h> > #include <stdbool.h> > +#include "qemu/typedefs.h" > #include "qemu/queue.h" > > 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); > > +/* 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_VISITOR, \ > + void *OBJECT_ADD_PROP_OPAQUE, \ > + const char *OBJECT_ADD_PROP_NAME, \ > + struct Error **OBJECT_ADD_PROP_VALUE_ERR) \ > + { \ > + int64_t OBJECT_ADD_PROP_VALUE = 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); > > /** > -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties 2013-09-16 6:32 ` Andreas Färber @ 2013-09-16 12:33 ` Michael S. Tsirkin 2013-09-16 15:24 ` Andreas Färber 2013-09-16 16:07 ` Michael S. Tsirkin 1 sibling, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2013-09-16 12:33 UTC (permalink / raw) To: Andreas Färber Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel, Anthony Liguori, Igor Mammedov On Mon, Sep 16, 2013 at 08:32:13AM +0200, Andreas Färber 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. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > 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 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. > > So what about taking bool/string property helpers as example and putting > 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 pointer > 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", errp); backends/tpm.c: object_property_set_bool(OBJECT(s), true, "opened", errp); 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", NULL); 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, "realized", &err); include/qom/object.h: * object_property_set_bool: include/qom/object.h:void object_property_set_bool(Object *obj, bool value, qom/object.c:void object_property_set_bool(Object *obj, bool value, target-alpha/cpu.c: object_property_set_bool(OBJECT(cpu), true, "realized", 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, "realized", 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, "realized", &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), true, "realized", NULL); target-moxie/cpu.c: object_property_set_bool(OBJECT(cpu), true, "realized", 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, "realized", NULL); target-sparc/cpu.c: object_property_set_bool(OBJECT(cpu), true, "realized", 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, "realized", 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 ago > 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. > > Regards, > Andreas > > > > > include/qom/object.h | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > 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 <glib.h> > > #include <stdint.h> > > #include <stdbool.h> > > +#include "qemu/typedefs.h" > > #include "qemu/queue.h" > > > > 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); > > > > +/* 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_VISITOR, \ > > + void *OBJECT_ADD_PROP_OPAQUE, \ > > + const char *OBJECT_ADD_PROP_NAME, \ > > + struct Error **OBJECT_ADD_PROP_VALUE_ERR) \ > > + { \ > > + int64_t OBJECT_ADD_PROP_VALUE = 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); > > > > /** > > > > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties 2013-09-16 12:33 ` Michael S. Tsirkin @ 2013-09-16 15:24 ` Andreas Färber 2013-09-16 15:41 ` Paolo Bonzini 2013-09-16 15:48 ` Michael S. Tsirkin 0 siblings, 2 replies; 14+ messages in thread From: Andreas Färber @ 2013-09-16 15:24 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel, Anthony Liguori, Igor Mammedov Am 16.09.2013 14:33, schrieb Michael S. Tsirkin: > On Mon, Sep 16, 2013 at 08:32:13AM +0200, Andreas Färber 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. >>> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> --- >>> >>> 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 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. >> >> So what about taking bool/string property helpers as example and putting >> 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 pointer >> 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", errp); > backends/tpm.c: object_property_set_bool(OBJECT(s), true, "opened", errp); These look like two distinct properties used once each. [...] > hw/core/qdev.c: object_property_set_bool(OBJECT(dev), true, "realized", &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, "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, "realized", &err); [...] > target-alpha/cpu.c: object_property_set_bool(OBJECT(cpu), true, "realized", 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, "realized", 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, "realized", &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), true, "realized", NULL); > target-moxie/cpu.c: object_property_set_bool(OBJECT(cpu), true, "realized", 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, "realized", NULL); > target-sparc/cpu.c: object_property_set_bool(OBJECT(cpu), true, "realized", 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, "realized", NULL); Leaving the bulk for "realized". > > Shouldn't we have a constant for the "realized" string? That's a two-sided sword: We actually shouldn't be setting realized = true manually but once on machine init - in that case we wouldn't strictly need a constant. I pushed to get that central infrastructure in place to spare me/us the repetitive realized = true setting, but Paolo shot it down, asking for a full-fledged solver to make ordering guarantees. > If there's a typo somewhere it will all fail at runtime > in a hard to debug way, won't it? 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. You are cordially invited to add trivial qtests covering instantiation of PCI/virtio devices you care about. :) Andreas -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties 2013-09-16 15:24 ` Andreas Färber @ 2013-09-16 15:41 ` Paolo Bonzini 2013-09-16 15:48 ` Michael S. Tsirkin 1 sibling, 0 replies; 14+ messages in thread From: Paolo Bonzini @ 2013-09-16 15:41 UTC (permalink / raw) To: Andreas Färber Cc: Igor Mammedov, Stefan Hajnoczi, qemu-devel, Anthony Liguori, Michael S. Tsirkin Il 16/09/2013 17:24, Andreas Färber ha scritto: >> > >> > Shouldn't we have a constant for the "realized" string? > That's a two-sided sword: We actually shouldn't be setting realized = > true manually but once on machine init - in that case we wouldn't > strictly need a constant. > > I pushed to get that central infrastructure in place to spare me/us the > repetitive realized = true setting, but Paolo shot it down, asking for a > full-fledged solver to make ordering guarantees. > Actually, I said my understanding of the problem was that we have two "conflicting" hierarchies. I didn't really ask for a solver, more like a topological sort actually, which is very simple to implement. But above everything else I asked to prove me wrong. I provided IIRC an example where the hierarchies were conflicting, could that example be incorrect? The discussion died down. Could it be a topic for tomorrow's call? I certainly would prefer to have realized = true at machine-ready time, I think it was even part of the very first RFC realized series that were posted. Paolo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties 2013-09-16 15:24 ` Andreas Färber 2013-09-16 15:41 ` Paolo Bonzini @ 2013-09-16 15:48 ` Michael S. Tsirkin 2013-09-16 15:56 ` Paolo Bonzini 1 sibling, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2013-09-16 15:48 UTC (permalink / raw) To: Andreas Färber Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel, Anthony Liguori, Igor Mammedov On Mon, Sep 16, 2013 at 05:24:46PM +0200, Andreas Färber wrote: > Am 16.09.2013 14:33, schrieb Michael S. Tsirkin: > > On Mon, Sep 16, 2013 at 08:32:13AM +0200, Andreas Färber 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. > >>> > >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>> --- > >>> > >>> 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 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. > >> > >> So what about taking bool/string property helpers as example and putting > >> 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 pointer > >> 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", errp); > > backends/tpm.c: object_property_set_bool(OBJECT(s), true, "opened", errp); > > These look like two distinct properties used once each. > > [...] > > hw/core/qdev.c: object_property_set_bool(OBJECT(dev), true, "realized", &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, "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, "realized", &err); > [...] > > target-alpha/cpu.c: object_property_set_bool(OBJECT(cpu), true, "realized", 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, "realized", 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, "realized", &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), true, "realized", NULL); > > target-moxie/cpu.c: object_property_set_bool(OBJECT(cpu), true, "realized", 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, "realized", NULL); > > target-sparc/cpu.c: object_property_set_bool(OBJECT(cpu), true, "realized", 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, "realized", NULL); > > Leaving the bulk for "realized". > > > > > Shouldn't we have a constant for the "realized" string? > > That's a two-sided sword: We actually shouldn't be setting realized = > true manually but once on machine init - in that case we wouldn't > strictly need a constant. > > I pushed to get that central infrastructure in place to spare me/us the > repetitive realized = true setting, but Paolo shot it down, asking for a > full-fledged solver to make ordering guarantees. > > > If there's a typo somewhere it will all fail at runtime > > in a hard to debug way, won't it? > > 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. > > You are cordially invited to add trivial qtests covering instantiation > of PCI/virtio devices you care about. :) > > 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." > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties 2013-09-16 15:48 ` Michael S. Tsirkin @ 2013-09-16 15:56 ` Paolo Bonzini 2013-09-16 16:01 ` Michael S. Tsirkin 0 siblings, 1 reply; 14+ messages in thread From: Paolo Bonzini @ 2013-09-16 15:56 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Igor Mammedov, Stefan Hajnoczi, Andreas Färber, Anthony Liguori, qemu-devel Il 16/09/2013 17:48, Michael S. Tsirkin ha scritto: > 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." We definitely can get at least to "make check won't let you get it wrong", which is somewhere in the middle. Paolo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties 2013-09-16 15:56 ` Paolo Bonzini @ 2013-09-16 16:01 ` Michael S. Tsirkin 2013-09-16 16:07 ` Paolo Bonzini 0 siblings, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2013-09-16 16:01 UTC (permalink / raw) To: Paolo Bonzini Cc: Igor Mammedov, Stefan Hajnoczi, Andreas Färber, Anthony Liguori, qemu-devel On Mon, Sep 16, 2013 at 05:56:56PM +0200, Paolo Bonzini wrote: > Il 16/09/2013 17:48, Michael S. Tsirkin ha scritto: > > 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." > > We definitely can get at least to "make check won't let you get it > wrong", which is somewhere in the middle. > > Paolo We can't. make check just runs unit tests. So it can catch changes, but it can not catch bugs in new interfaces. -- MST ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties 2013-09-16 16:01 ` Michael S. Tsirkin @ 2013-09-16 16:07 ` Paolo Bonzini 2013-09-16 16:51 ` Michael S. Tsirkin 0 siblings, 1 reply; 14+ messages in thread From: Paolo Bonzini @ 2013-09-16 16:07 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Igor Mammedov, Stefan Hajnoczi, Andreas Färber, Anthony Liguori, qemu-devel Il 16/09/2013 18:01, Michael S. Tsirkin ha scritto: > On Mon, Sep 16, 2013 at 05:56:56PM +0200, Paolo Bonzini wrote: >> Il 16/09/2013 17:48, Michael S. Tsirkin ha scritto: >>> 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." >> >> We definitely can get at least to "make check won't let you get it >> wrong", which is somewhere in the middle. >> >> Paolo > > We can't. > make check just runs unit tests. > So it can catch changes, but it can not catch bugs in new > interfaces. We can have "make check" run QEMU once for each board, which would trap things that will always break at runtime such as a misspelled property. Similarly, we could have tests that try to instantiate every device, even if they do not do anything with the guest-visible device. Such dummy tests can catch bugs in interface changes. I look forward to discussing the future of qos and qtest at KVM Forum... :) Paolo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties 2013-09-16 16:07 ` Paolo Bonzini @ 2013-09-16 16:51 ` Michael S. Tsirkin 2013-09-16 17:03 ` Paolo Bonzini 0 siblings, 1 reply; 14+ messages in thread From: Michael S. Tsirkin @ 2013-09-16 16:51 UTC (permalink / raw) To: Paolo Bonzini Cc: Igor Mammedov, Stefan Hajnoczi, Andreas Färber, Anthony Liguori, qemu-devel On Mon, Sep 16, 2013 at 06:07:33PM +0200, Paolo Bonzini wrote: > Il 16/09/2013 18:01, Michael S. Tsirkin ha scritto: > > On Mon, Sep 16, 2013 at 05:56:56PM +0200, Paolo Bonzini wrote: > >> Il 16/09/2013 17:48, Michael S. Tsirkin ha scritto: > >>> 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." > >> > >> We definitely can get at least to "make check won't let you get it > >> wrong", which is somewhere in the middle. > >> > >> Paolo > > > > We can't. > > make check just runs unit tests. > > So it can catch changes, but it can not catch bugs in new > > interfaces. > > We can have "make check" run QEMU once for each board, which would trap > things that will always break at runtime such as a misspelled property. > Similarly, we could have tests that try to instantiate every device, > even if they do not do anything with the guest-visible device. Such > dummy tests can catch bugs in interface changes. They won't catch bugs for properties that 1. change after device is instanciated 2. are accessed after device is instanciated > I look forward to discussing the future of qos and qtest at KVM Forum... :) > > Paolo This is not the topic I started really. Testing isn't a replacement for type safety. C is a compiled language for a reason, qom needs a set of wrappers to get that back, strings should only be used for external interfaces. -- MST ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties 2013-09-16 16:51 ` Michael S. Tsirkin @ 2013-09-16 17:03 ` Paolo Bonzini 0 siblings, 0 replies; 14+ messages in thread From: Paolo Bonzini @ 2013-09-16 17:03 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Igor Mammedov, Stefan Hajnoczi, Andreas Färber, Anthony Liguori, qemu-devel Il 16/09/2013 18:51, Michael S. Tsirkin ha scritto: >> > >> > We can have "make check" run QEMU once for each board, which would trap >> > things that will always break at runtime such as a misspelled property. >> > Similarly, we could have tests that try to instantiate every device, >> > even if they do not do anything with the guest-visible device. Such >> > dummy tests can catch bugs in interface changes. > They won't catch bugs for properties that > 1. change after device is instanciated > 2. are accessed after device is instanciated True, but I don't think there are any at this time if you s/instantiated/realized/. Paolo ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties 2013-09-16 6:32 ` Andreas Färber 2013-09-16 12:33 ` Michael S. Tsirkin @ 2013-09-16 16:07 ` Michael S. Tsirkin 1 sibling, 0 replies; 14+ messages in thread From: Michael S. Tsirkin @ 2013-09-16 16:07 UTC (permalink / raw) To: Andreas Färber Cc: Paolo Bonzini, Stefan Hajnoczi, qemu-devel, Anthony Liguori, Igor Mammedov On Mon, Sep 16, 2013 at 08:32:13AM +0200, Andreas Färber 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. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > 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, BTW there are no upper casing of arguments: internal variable names in macros must include macro name otherwise you get hard to debug collisions: #define SQUARE_ME(x) \ do { \ int var = x; \ x = var * x; \ } while (0) \ int v = 2; SQUARE_ME(v); // v is 4 int var = 2; SQUARE_ME(var); // BUG: var is still 2 > 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. > > So what about taking bool/string property helpers as example and putting > 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 pointer > 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. > > It does touch on the issue I brought up on a KVM call a couple weeks ago > 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. > > Regards, > Andreas OK this makes you happy? All calls pass NULL as Error so I omitted it for now. We can add later if anyone has a use for it. diff --git a/include/qom/object.h b/include/qom/object.h index 1a7b71a..90dce59 100644 --- a/include/qom/object.h +++ b/include/qom/object.h @@ -795,6 +795,27 @@ void object_property_add(Object *obj, const char *name, const char *type, void object_property_del(Object *obj, const char *name, struct Error **errp); /** + * object_property_add_uint8_t_ptr: + * object_property_add_uint16_t_ptr: + * object_property_add_uint32_t_ptr: + * object_property_add_uint64_t_ptr: + * @obj: the object to add a property to + * @name: the name of the property + * @v: pointer to value + * + * Add an integer property in memory. This function will add a + * property of the appropriate type. + */ +void object_property_add_uint8_t_ptr(Object *obj, const char *name, + const uint8_t *v); +void object_property_add_uint16_t_ptr(Object *obj, const char *name, + const uint16_t *v); +void object_property_add_uint32_t_ptr(Object *obj, const char *name, + const uint32_t *v); +void object_property_add_uint64_t_ptr(Object *obj, const char *name, + const uint64_t *v); + +/** * object_property_find: * @obj: the object * @name: the name of the property ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-09-16 17:03 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-15 17:23 [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties Michael S. Tsirkin 2013-09-15 17:54 ` Peter Maydell 2013-09-15 20:06 ` Michael S. Tsirkin 2013-09-16 6:32 ` Andreas Färber 2013-09-16 12:33 ` Michael S. Tsirkin 2013-09-16 15:24 ` Andreas Färber 2013-09-16 15:41 ` Paolo Bonzini 2013-09-16 15:48 ` Michael S. Tsirkin 2013-09-16 15:56 ` Paolo Bonzini 2013-09-16 16:01 ` Michael S. Tsirkin 2013-09-16 16:07 ` Paolo Bonzini 2013-09-16 16:51 ` Michael S. Tsirkin 2013-09-16 17:03 ` Paolo Bonzini 2013-09-16 16:07 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).