diff mbox

[RFC,v0,5/8] object: make interfaces concrete

Message ID 76ad34dd30a83d96a97bc7443245ee9c165e053f.1339578989.git.peter.crosthwaite@petalogix.com
State New
Headers show

Commit Message

Peter A. G. Crosthwaite June 13, 2012, 9:38 a.m. UTC
Objects that define interface delegate the creation of the interface object
to the interface type. These means that object_new() when called recursively by
the interface instantior is going to bork because its trying to instantiate
an abstract type. Fixed by making interface types concrete.

Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
---
 qom/object.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Comments

Andreas Färber June 13, 2012, 10:28 a.m. UTC | #1
Am 13.06.2012 12:03, schrieb Paolo Bonzini:
> Il 13/06/2012 11:38, Peter A. G. Crosthwaite ha scritto:
>> Objects that define interface delegate the creation of the interface object
>> to the interface type. These means that object_new() when called recursively by

I am pretty certain that object_new() is NOT called recursively! The
static helpers of object_instantiate() are, and abstractness should not
matter there or none of the, e.g., CPU subclasses could be created... So
if there is a problem this description is bogus.

>> the interface instantior is going to bork because its trying to instantiate
>> an abstract type. Fixed by making interface types concrete.
>>
>> Signed-off-by: Peter A. G. Crosthwaite <peter.crosthwaite@petalogix.com>
>> ---
>>  qom/object.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/qom/object.c b/qom/object.c
>> index 1eba795..c3a7a47 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -191,7 +191,7 @@ static void type_class_interface_init(TypeImpl *ti, InterfaceImpl *iface)
>>          .parent = iface->parent,
>>          .class_size = sizeof(InterfaceClass),
>>          .class_init = iface->interface_initfn,
>> -        .abstract = true,
>> +        .abstract = false,
>>      };
>>      char *name = g_strdup_printf("<%s::%s>", ti->name, iface->parent);
>>  
>>
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Why? Object is abstract, too, and in patch 3/8 a type is being derived
from TYPE_INTERFACE. So if we want to make interface non-abstract then
we don't need the Container type either since it is simply the
non-abstract version of Object. My guess is that, if at all, something
else is going wrong and this is papering over it.

Andreas
Paolo Bonzini June 13, 2012, 10:42 a.m. UTC | #2
Il 13/06/2012 12:28, Andreas Färber ha scritto:
> I am pretty certain that object_new() is NOT called recursively!

It is for interfaces:

object_new
  ->object_new_with_type
  ->object_initialize_with_type
  ->object_init_with_type
  ->object_interface_init

and then:

static void object_interface_init(Object *obj, InterfaceImpl *iface)
{
    TypeImpl *ti = iface->type;
    Interface *iface_obj;

    assert(!object_is_interface(obj));
    iface_obj = INTERFACE(object_new(ti->name));
    iface_obj->iface_obj = obj;
    iface_obj->iface_parent.interfaces = INTERFACE_MAGIC;

    obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
}

Paolo
Andreas Färber June 13, 2012, 12:59 p.m. UTC | #3
Am 13.06.2012 12:42, schrieb Paolo Bonzini:
> Il 13/06/2012 12:28, Andreas Färber ha scritto:
>> I am pretty certain that object_new() is NOT called recursively!
> 
> It is for interfaces:
> 
> object_new
>   ->object_new_with_type
>   ->object_initialize_with_type
>   ->object_init_with_type
>   ->object_interface_init
> 
> and then:
> 
> static void object_interface_init(Object *obj, InterfaceImpl *iface)
> {
>     TypeImpl *ti = iface->type;
>     Interface *iface_obj;
> 
>     assert(!object_is_interface(obj));
>     iface_obj = INTERFACE(object_new(ti->name));
>     iface_obj->iface_obj = obj;
>     iface_obj->iface_parent.interfaces = INTERFACE_MAGIC;
> 
>     obj->interfaces = g_slist_prepend(obj->interfaces, iface_obj);
> }

Ouch! One can argue that's still not recursive, but what matters more
this borks Anthony's in-place object_initialize() concept.

Two solutions come to mind:
* allocate the interfaces as part of object_new() beyond instance_size
* let initialize stage return Error*

I'm starting to tend towards the latter since that also addresses
failure to add properties.

Andreas
Paolo Bonzini June 13, 2012, 1:02 p.m. UTC | #4
Il 13/06/2012 14:59, Andreas Färber ha scritto:
> Ouch! One can argue that's still not recursive, but what matters more
> this borks Anthony's in-place object_initialize() concept.
> 
> Two solutions come to mind:
> * allocate the interfaces as part of object_new() beyond instance_size

That won't work if you initialize in place, because you cannot allocate
the room for the interface.  It is possible to put Interface objects
explicitly in the class, and pass an offset when registering the type so
that they can be initialized in place.

But I still think we're fighting windmills...

Paolo
diff mbox

Patch

diff --git a/qom/object.c b/qom/object.c
index 1eba795..c3a7a47 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -191,7 +191,7 @@  static void type_class_interface_init(TypeImpl *ti, InterfaceImpl *iface)
         .parent = iface->parent,
         .class_size = sizeof(InterfaceClass),
         .class_init = iface->interface_initfn,
-        .abstract = true,
+        .abstract = false,
     };
     char *name = g_strdup_printf("<%s::%s>", ti->name, iface->parent);