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 <address@hidden>
> > ---
> > 
> > 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.
address@hidden 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