| Message ID | 20130915172331.GA2821@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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); > > /** >
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
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
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
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
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
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.
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
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.
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
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); /**
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(+)