[Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[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?

 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

Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[Michael S. Tsirkin]

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.

Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[Andreas Färber]

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

Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[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);
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

Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[Andreas Färber]

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

Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[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,

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

Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH] qom: helper macro for adding read-only properties

[Paolo Bonzini]

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