diff mbox

[v3,1/3] qom: make base type user-creatable abstract

Message ID 20160926101627.14296-2-lma@suse.com (mailing list archive)
State New, archived
Headers show

Commit Message

Lin Ma Sept. 26, 2016, 10:16 a.m. UTC 
Signed-off-by: Lin Ma <lma@suse.com>
---
 qom/object_interfaces.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Daniel P. Berrangé Sept. 26, 2016, 10:37 a.m. UTC | #1 
On Mon, Sep 26, 2016 at 06:16:25PM +0800, Lin Ma wrote:
> Signed-off-by: Lin Ma <lma@suse.com>
> ---
>  qom/object_interfaces.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index bf59846..9288242 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -217,6 +217,7 @@ static void register_types(void)
>      static const TypeInfo uc_interface_info = {
>          .name          = TYPE_USER_CREATABLE,
>          .parent        = TYPE_INTERFACE,
> +        .abstract      = true,
>          .class_size = sizeof(UserCreatableClass),
>      };

This doesn't make any conceptual sense. UserCreatable is an inteface and
by definition all interfaces are abstract.

Were you trying to fix some particular real bug here ? If so, we almost
certainly need a different fix to what's suggested here, because QOM
should automatically treat all interfaces as abstract by their very
nature.

Regards,
Daniel
Lin Ma Oct. 10, 2016, 2:32 p.m. UTC | #2 
>>> "Daniel P. Berrange" <berrange@redhat.com> 2016/9/26 星期一 下午 6:37 >>>
>On Mon, Sep 26, 2016 at 06:16:25PM +0800, Lin Ma wrote:
>> Signed-off-by: Lin Ma <lma@suse.com>
>> ---
>>  qom/object_interfaces.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
>> index bf59846..9288242 100644
>> --- a/qom/object_interfaces.c
>> +++ b/qom/object_interfaces.c
>> @@ -217,6 +217,7 @@ static void register_types(void)
>>	  static const TypeInfo uc_interface_info = {
>>		  .name   	   = TYPE_USER_CREATABLE,
>>		  .parent   	 = TYPE_INTERFACE,
>> +	    .abstract 	 = true,
>>		  .class_size = sizeof(UserCreatableClass),
>>	  };
>
>This doesn't make any conceptual sense. UserCreatable is an inteface and
>by definition all interfaces are abstract.
>
Sorry for the late reply, I was on vacation.
oh...indeed, add '.abstract = true' for an interface is not a good idea.

>Were you trying to fix some particular real bug here ? If so, we almost
>certainly need a different fix to what's suggested here, because QOM
>should automatically treat all interfaces as abstract by their very
>nature.
>
For other interfaces, say 'acpi-device-interface' or 'ipmi-interface', it's fine
as they are not user creatable. but for 'user-creatable', when performing
'qemu-system-x86_64 -object user-creatable,id=foo', segfault happens:
 
    $ qemu-system-x86_64 -object user-creatable,id=foo
    **
    ERROR:/work/armbru/qemu/qom/object.c:361:object_initialize_with_type: assertion failed (type->instance_size >= sizeof(Object)): (0 >= 40)
    Aborted (core dumped)
 
 
How about the following changes to skip TYPE_USER_CREATABLE in function
user_creatable_add_type ?
 
@@ -102,7 +102,8 @@ Object *user_creatable_add_type(const char *type, const char *id,
		 return NULL;
	 }
 
-    if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE)) {
+    if (!object_class_dynamic_cast(klass, TYPE_USER_CREATABLE) ||
+	    !strcmp(type, TYPE_USER_CREATABLE)) {
		 error_setg(errp, "object type '%s' isn't supported by object-add",
				    type);
		 return NULL;
...
 
@@ -229,7 +230,8 @@ int user_creatable_help_func(void *opaque, QemuOpts *opts, Error **errp)
				 list = list->next) {
			 const char *name;
			 name = object_class_get_name(OBJECT_CLASS(list->data));
-		    printf("%s\n", name);
+		    if (strcmp(name, TYPE_USER_CREATABLE))
+			    printf("%s\n", name);
...
 
 
Thanks,
Lin
Paolo Bonzini Oct. 10, 2016, 2:45 p.m. UTC | #3 
On 10/10/2016 16:32, Lin Ma wrote:
>>This doesn't make any conceptual sense. UserCreatable is an inteface and
>>by definition all interfaces are abstract.
>
> Sorry for the late reply, I was on vacation.
> oh...indeed, add '.abstract = true' for an interface is not a good idea.

Perhaps Lin Ma's patch actually made some sense.  If all interfaces are
abstract, why shouldn't object_class_is_abstract say so?

The right fix then would be to add a new check in there, using
type_is_ancestor(..., type_interface).  This would also fix
object_new_with_propv.

Paolo
Daniel P. Berrangé Oct. 10, 2016, 3:01 p.m. UTC | #4 
On Mon, Oct 10, 2016 at 04:45:56PM +0200, Paolo Bonzini wrote:
> 
> 
> On 10/10/2016 16:32, Lin Ma wrote:
> >>This doesn't make any conceptual sense. UserCreatable is an inteface and
> >>by definition all interfaces are abstract.
> >
> > Sorry for the late reply, I was on vacation.
> > oh...indeed, add '.abstract = true' for an interface is not a good idea.
> 
> Perhaps Lin Ma's patch actually made some sense.  If all interfaces are
> abstract, why shouldn't object_class_is_abstract say so?

object_class_is_abstract() should say so, but we should not have to
express that manually when defining the interface - anything that is
define as an interface should be automatically abstract

Regards,
Daniel
Paolo Bonzini Oct. 10, 2016, 4:37 p.m. UTC | #5 
On 10/10/2016 17:01, Daniel P. Berrange wrote:
> On Mon, Oct 10, 2016 at 04:45:56PM +0200, Paolo Bonzini wrote:
>>
>>
>> On 10/10/2016 16:32, Lin Ma wrote:
>>>> This doesn't make any conceptual sense. UserCreatable is an inteface and
>>>> by definition all interfaces are abstract.
>>>
>>> Sorry for the late reply, I was on vacation.
>>> oh...indeed, add '.abstract = true' for an interface is not a good idea.
>>
>> Perhaps Lin Ma's patch actually made some sense.  If all interfaces are
>> abstract, why shouldn't object_class_is_abstract say so?
> 
> object_class_is_abstract() should say so, but we should not have to
> express that manually when defining the interface - anything that is
> define as an interface should be automatically abstract

Yes, of course, see my suggestion right after that line. :)

Paolo
diff mbox

Patch

diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index bf59846..9288242 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -217,6 +217,7 @@  static void register_types(void)
     static const TypeInfo uc_interface_info = {
         .name          = TYPE_USER_CREATABLE,
         .parent        = TYPE_INTERFACE,
+        .abstract      = true,
         .class_size = sizeof(UserCreatableClass),
     };