Re: [Qemu-devel] [PATCH 04/25] qom: make Object a type

["Andreas Färber" <afaerber@suse.de>]

Am 03.04.2012 13:15, schrieb Paolo Bonzini:
> Right now the base Object class has a special NULL type.  Change this so
> that we will be able to add class_init and class_base_init callbacks.
> To do this, remove some special casing of ObjectClass that is not really
> necessary.

While the patch itself looks good, we should be aware that this changes
semantics: Before this patch, lack of .parent is identical to .parent =
TYPE_OBJECT; with this patch that would become another base class.
Should be mentioned in the commit message and all TypeInfos need to be
reviewed. I don't think accidentally growing base classes is a good
idea. Maybe whitelist valid base classes in type_register_static() and
abort otherwise? Then at least we'd catch it by just running the
executables.

> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  include/qemu/object.h |    2 +-
>  qom/object.c          |   59 +++++++++++++++++++++++++------------------------
>  2 files changed, 31 insertions(+), 30 deletions(-)
> 
> diff --git a/include/qemu/object.h b/include/qemu/object.h
> index ccaea7d..22f646d 100644
> --- a/include/qemu/object.h
> +++ b/include/qemu/object.h
> @@ -33,7 +33,7 @@ typedef struct TypeInfo TypeInfo;
>  typedef struct InterfaceClass InterfaceClass;
>  typedef struct InterfaceInfo InterfaceInfo;
>  
> -#define TYPE_OBJECT NULL
> +#define TYPE_OBJECT "object"
>  
>  /**
>   * SECTION:object.h
> diff --git a/qom/object.c b/qom/object.c
> index 6ff1c19..585619d 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -210,7 +210,7 @@ static void type_class_interface_init(TypeImpl *ti, InterfaceImpl *iface)
>  
>  static void type_initialize(TypeImpl *ti)
>  {
> -    size_t class_size = sizeof(ObjectClass);
> +    TypeImpl *parent;
>      int i;
>  
>      if (ti->class) {
> @@ -221,30 +221,24 @@ static void type_initialize(TypeImpl *ti)
>      ti->instance_size = type_object_get_size(ti);
>  
>      ti->class = g_malloc0(ti->class_size);
> -    ti->class->type = ti;
> -
> -    if (type_has_parent(ti)) {
> -        TypeImpl *parent = type_get_parent(ti);
>  
> +    parent = type_get_parent(ti);
> +    if (parent) {
>          type_initialize(parent);
>  
> -        class_size = parent->class_size;
>          g_assert(parent->class_size <= ti->class_size);
> +        memcpy(ti->class, parent->class, parent->class_size);
> +    }
>  
> -        memcpy((void *)ti->class + sizeof(ObjectClass),
> -               (void *)parent->class + sizeof(ObjectClass),
> -               parent->class_size - sizeof(ObjectClass));

Is this really safe? Before we would only copy the contents of derived
classes. Missing some explanations IMO.

> +    ti->class->type = ti;
>  
> -        while (parent) {
> -            if (parent->class_base_init) {
> -                parent->class_base_init(ti->class, ti->class_data);
> -            }
> -            parent = type_get_parent(parent);
> +    while (parent) {
> +        if (parent->class_base_init) {
> +            parent->class_base_init(ti->class, ti->class_data);
>          }
> +        parent = type_get_parent(parent);
>      }
>  
> -    memset((void *)ti->class + class_size, 0, ti->class_size - class_size);
> -
>      for (i = 0; i < ti->num_interfaces; i++) {
>          type_class_interface_init(ti, &ti->interfaces[i]);
>      }
> @@ -467,19 +461,6 @@ Object *object_dynamic_cast(Object *obj, const char *typename)
>  }
>  
>  
> -static void register_types(void)
> -{
> -    static TypeInfo interface_info = {
> -        .name = TYPE_INTERFACE,
> -        .instance_size = sizeof(Interface),
> -        .abstract = true,
> -    };
> -
> -    type_interface = type_register_static(&interface_info);
> -}
> -
> -type_init(register_types)
> -
>  Object *object_dynamic_cast_assert(Object *obj, const char *typename)
>  {
>      Object *inst;
> @@ -1233,3 +1214,23 @@ void object_property_add_str(Object *obj, const char *name,
>                          property_release_str,
>                          prop, errp);
>  }
> +
> +static void register_types(void)
> +{
> +    static TypeInfo interface_info = {
> +        .name = TYPE_INTERFACE,
> +        .instance_size = sizeof(Interface),
> +        .abstract = true,
> +    };
> +
> +    static TypeInfo object_info = {
> +        .name = TYPE_OBJECT,
> +        .instance_size = sizeof(Object),
> +        .abstract = true,
> +    };
> +
> +    type_interface = type_register_static(&interface_info);
> +    type_register_static(&object_info);
> +}
> +
> +type_init(register_types)

Thanks for moving these down.

Is there a reason not to make them static const just before
register_types()?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg