[Qemu-devel] [PATCH RFC 0/2] QMP command qom-new

[Qemu-devel] [PATCH RFC 0/2] QMP command qom-new

[Markus Armbruster]

Beware: second patch is the product of voodoo-coding.

Markus Armbruster (2):
  qom: Give type_get_by_name() external linkage
  qmp: New command qom-new

 include/qemu/object.h |    8 ++++++++
 qapi-schema.json      |   22 ++++++++++++++++++++++
 qmp-commands.hx       |    5 +++++
 qmp.c                 |   27 +++++++++++++++++++++++++++
 qom/object.c          |    2 +-
 5 files changed, 63 insertions(+), 1 deletions(-)

-- 
1.7.6.5

[Qemu-devel] [PATCH RFC 1/2] qom: Give type_get_by_name() external linkage

[Markus Armbruster]

Following the type_register() precedence, the definition returns
"TypeImpl *", while the declaration returns "Type".  Fine, because the
latter is actually a typedef for the former.

For the record, I don't think this typedef'ing away the "*" is a good
idea.  It only complicates matters.  When I see

    TypeImpl *type_get_by_name(const char *name)

it's immediately obvious that the value has pointer semantics, and a
null pointer must be the error value.

Moreover, it's unusual in QEMU code.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qemu/object.h |    8 ++++++++
 qom/object.c          |    2 +-
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/include/qemu/object.h b/include/qemu/object.h
index d93b772..3648462 100644
--- a/include/qemu/object.h
+++ b/include/qemu/object.h
@@ -535,6 +535,14 @@ Type type_register_static(const TypeInfo *info);
 Type type_register(const TypeInfo *info);
 
 /**
+ * type_get_by_name:
+ * @name: The name of the type
+ *
+ * Returns: The #Type with that name if it exists, else NULL.
+ */
+Type type_get_by_name(const char *name);
+
+/**
  * object_class_dynamic_cast_assert:
  * @klass: The #ObjectClass to attempt to cast.
  * @typename: The QOM typename of the class to cast to.
diff --git a/qom/object.c b/qom/object.c
index 6f839ad..94ea0f9 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -140,7 +140,7 @@ TypeImpl *type_register_static(const TypeInfo *info)
     return type_register(info);
 }
 
-static TypeImpl *type_get_by_name(const char *name)
+TypeImpl *type_get_by_name(const char *name)
 {
     if (name == NULL) {
         return NULL;
-- 
1.7.6.5

[Qemu-devel] [PATCH RFC 2/2] qmp: New command qom-new

[Markus Armbruster]

To create objects via QMP.

Test case:

$ upstream-qemu --enable-kvm -S -m 384 -vnc :0 -monitor stdio -chardev socket,id=qmp,path=test-qmp,server=on,wait=off -mon mode=control,chardev=qmp

Conversation on the qmp socket:
{"QMP": {"version": {"qemu": {"micro": 93, "minor": 0, "major": 1}, "package": ""}, "capabilities": []}}
{ "execute": "qmp_capabilities" }
{"return": {}}
{"execute":"qom-new","arguments":{"parent":"/xxx", "prop-name":"test-qmp-new", "type-name":"xxx"}}
{"error": {"class": "DeviceNotFound", "desc": "Device '/xxx' not found", "data": {"device": "/xxx"}}}
{"execute":"qom-new","arguments":{"parent":"/machine", "prop-name":"test-qmp-new", "type-name":"xxx"}}
{"error": {"class": "InvalidParameterValue", "desc": "Parameter 'type-name' expects a type name", "data": {"name": "type-name", "expected": "a type name"}}}
{"execute":"qom-new","arguments":{"parent":"/machine", "prop-name":"test-qmp-new", "type-name":"container"}}
{"return": "/machine/test-qmp-new"}
{"execute":"qom-list","arguments":{"path":"/machine"}}
{"return": [{"name": "test-qmp-new", "type": "child<container>"}, {"name": "i440fx", "type": "child<i440FX-pcihost>"}, {"name": "unattached", "type": "child<container>"}, {"name": "peripheral", "type": "child<container>"}, {"name": "peripheral-anon", "type": "child<container>"}]}

Note: qdev objects (subtype of TYPE_DEVICE) created with qom-new lack
additional magic performed by qdev_try_create(), and almost certainly
won't work.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qapi-schema.json |   22 ++++++++++++++++++++++
 qmp-commands.hx  |    5 +++++
 qmp.c            |   27 +++++++++++++++++++++++++++
 3 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/qapi-schema.json b/qapi-schema.json
index 2ca7195..ab9e68b 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1699,6 +1699,28 @@
   'returns': [ 'ObjectTypeInfo' ] }
 
 ##
+# qom-new:
+#
+# Create a new object
+#
+# @parent: the parent's path within the object model.  See @qom-get
+# for a description of paths.
+#
+# @prop-name: the name of the property to add to the parent.
+#
+# @type: the new object's type name
+#
+# Returns: The new object's canonical absolute path
+#
+# Since: 1.2
+#
+# Notes: This command is experimental and may change syntax in future releases.
+##
+{ 'command': 'qom-new',
+  'data': { 'parent': 'str', 'prop-name': 'str', 'type-name': 'str' },
+  'returns': 'str' }
+
+##
 # @migrate
 #
 # Migrates the current running guest to another Virtual Machine.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index db980fa..53adda2 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -2142,3 +2142,8 @@ EQMP
         .args_type  = "implements:s?,abstract:b?",
         .mhandler.cmd_new = qmp_marshal_input_qom_list_types,
     },
+    {
+        .name       = "qom-new",
+        .args_type  = "parent:s,prop-name:s,type-name:s",
+        .mhandler.cmd_new = qmp_marshal_input_qom_new,
+    },
diff --git a/qmp.c b/qmp.c
index fee9fb2..cad5610 100644
--- a/qmp.c
+++ b/qmp.c
@@ -417,3 +417,30 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
 
     return ret;
 }
+
+char *qmp_qom_new(const char *parent, const char *prop_name,
+                  const char *type_name, Error **errp)
+{
+    Object *p, *obj;
+    Type type;
+
+    // TODO anything fancy with containger_get() needed?
+    p = object_resolve_path(parent, NULL);
+    if (!p) {
+        error_set(errp, QERR_DEVICE_NOT_FOUND, parent);
+        return NULL;
+    }
+
+    type = type_get_by_name(type_name);
+    if (!type) {
+        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
+                  "type-name", "a type name");
+        return NULL;
+    }
+    obj = object_new_with_type(type);
+
+    // TODO bombs if p is an interface object; can this happen?
+    object_property_add_child(p, prop_name, obj, NULL);
+
+    return object_get_canonical_path(obj);
+}
-- 
1.7.6.5

Re: [Qemu-devel] [PATCH RFC 2/2] qmp: New command qom-new

[Paolo Bonzini]

Il 24/05/2012 13:43, Markus Armbruster ha scritto:
> Note: qdev objects (subtype of TYPE_DEVICE) created with qom-new lack
> additional magic performed by qdev_try_create(), and almost certainly
> won't work.

True.  With the patches on the list, all that adev_try_create does is really

    dev->parent_bus = bus;
    bus_add_child(bus, dev);

We should change that to a parent_bus property, so that device_add is
really qom-new + qom-set parent_bus.

Paolo

Re: [Qemu-devel] [PATCH RFC 0/2] QMP command qom-new

[Andreas Färber]

Am 24.05.2012 13:43, schrieb Markus Armbruster:
> Beware: second patch is the product of voodoo-coding.

Hm, I don't like the voodoo. ;) I would rather expose a proper C API
like object_try_new(const char *, Error **) than opening up the TypeImpl
internals to the public and hand-coding it everywhere. I ran into a
similar error-catching scenario where I needed to check for class
existence in some qdev_try_* function. And there were still too many
asserts in the QOM core for my taste.

The QMP command itself looks good to me.

Andreas

> Markus Armbruster (2):
>   qom: Give type_get_by_name() external linkage
>   qmp: New command qom-new
> 
>  include/qemu/object.h |    8 ++++++++
>  qapi-schema.json      |   22 ++++++++++++++++++++++
>  qmp-commands.hx       |    5 +++++
>  qmp.c                 |   27 +++++++++++++++++++++++++++
>  qom/object.c          |    2 +-
>  5 files changed, 63 insertions(+), 1 deletions(-)

-- 
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 RFC 2/2] qmp: New command qom-new

[Anthony Liguori]

On 05/24/2012 06:51 AM, Paolo Bonzini wrote:
> Il 24/05/2012 13:43, Markus Armbruster ha scritto:
>> Note: qdev objects (subtype of TYPE_DEVICE) created with qom-new lack
>> additional magic performed by qdev_try_create(), and almost certainly
>> won't work.
>
> True.  With the patches on the list, all that adev_try_create does is really
>
>      dev->parent_bus = bus;
>      bus_add_child(bus, dev);
>
> We should change that to a parent_bus property,

parent_bus is a property with the QOM bus series (which is part of your Push, 
push, series, no?).

You still can't add a bus as a child.  We need a magic child[*] link that acts 
like a clone file.  Shouldn't be hard to add.

Regards,

Anthony Liguori

  so that device_add is
> really qom-new + qom-set parent_bus.
>
> Paolo

Re: [Qemu-devel] [PATCH RFC 2/2] qmp: New command qom-new

[Andreas Färber]

Am 24.05.2012 14:58, schrieb Anthony Liguori:
> On 05/24/2012 06:51 AM, Paolo Bonzini wrote:
>> Il 24/05/2012 13:43, Markus Armbruster ha scritto:
>>> Note: qdev objects (subtype of TYPE_DEVICE) created with qom-new lack
>>> additional magic performed by qdev_try_create(), and almost certainly
>>> won't work.
>>
>> True.  With the patches on the list, all that adev_try_create does is
>> really
>>
>>      dev->parent_bus = bus;
>>      bus_add_child(bus, dev);
>>
>> We should change that to a parent_bus property,
> 
> parent_bus is a property with the QOM bus series (which is part of your
> Push, push, series, no?).

Note it's on qom-next already since last night but I discovered a minor
issue where I am going to move some code between two patches of yours
for consistency. Will send out official notices when I'm done.

Andreas

> 
> You still can't add a bus as a child.  We need a magic child[*] link
> that acts like a clone file.  Shouldn't be hard to add.
> 
> Regards,
> 
> Anthony Liguori
> 
>  so that device_add is
>> really qom-new + qom-set parent_bus.
>>
>> Paolo
> 


-- 
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 RFC 2/2] qmp: New command qom-new

[Anthony Liguori]

On 05/24/2012 06:43 AM, Markus Armbruster wrote:
> To create objects via QMP.
>
> Test case:
>
> $ upstream-qemu --enable-kvm -S -m 384 -vnc :0 -monitor stdio -chardev socket,id=qmp,path=test-qmp,server=on,wait=off -mon mode=control,chardev=qmp
>
> Conversation on the qmp socket:
> {"QMP": {"version": {"qemu": {"micro": 93, "minor": 0, "major": 1}, "package": ""}, "capabilities": []}}
> { "execute": "qmp_capabilities" }
> {"return": {}}
> {"execute":"qom-new","arguments":{"parent":"/xxx", "prop-name":"test-qmp-new", "type-name":"xxx"}}
> {"error": {"class": "DeviceNotFound", "desc": "Device '/xxx' not found", "data": {"device": "/xxx"}}}
> {"execute":"qom-new","arguments":{"parent":"/machine", "prop-name":"test-qmp-new", "type-name":"xxx"}}
> {"error": {"class": "InvalidParameterValue", "desc": "Parameter 'type-name' expects a type name", "data": {"name": "type-name", "expected": "a type name"}}}
> {"execute":"qom-new","arguments":{"parent":"/machine", "prop-name":"test-qmp-new", "type-name":"container"}}
> {"return": "/machine/test-qmp-new"}
> {"execute":"qom-list","arguments":{"path":"/machine"}}
> {"return": [{"name": "test-qmp-new", "type": "child<container>"}, {"name": "i440fx", "type": "child<i440FX-pcihost>"}, {"name": "unattached", "type": "child<container>"}, {"name": "peripheral", "type": "child<container>"}, {"name": "peripheral-anon", "type": "child<container>"}]}
>
> Note: qdev objects (subtype of TYPE_DEVICE) created with qom-new lack
> additional magic performed by qdev_try_create(), and almost certainly
> won't work.
>
> Signed-off-by: Markus Armbruster<armbru@redhat.com>
> ---
>   qapi-schema.json |   22 ++++++++++++++++++++++
>   qmp-commands.hx  |    5 +++++
>   qmp.c            |   27 +++++++++++++++++++++++++++
>   3 files changed, 54 insertions(+), 0 deletions(-)
>
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 2ca7195..ab9e68b 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1699,6 +1699,28 @@
>     'returns': [ 'ObjectTypeInfo' ] }
>
>   ##
> +# qom-new:
> +#
> +# Create a new object
> +#
> +# @parent: the parent's path within the object model.  See @qom-get
> +# for a description of paths.
> +#
> +# @prop-name: the name of the property to add to the parent.
> +#
> +# @type: the new object's type name
> +#
> +# Returns: The new object's canonical absolute path
> +#
> +# Since: 1.2
> +#
> +# Notes: This command is experimental and may change syntax in future releases.
> +##
> +{ 'command': 'qom-new',
> +  'data': { 'parent': 'str', 'prop-name': 'str', 'type-name': 'str' },
> +  'returns': 'str' }
> +
> +##
>   # @migrate
>   #
>   # Migrates the current running guest to another Virtual Machine.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index db980fa..53adda2 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -2142,3 +2142,8 @@ EQMP
>           .args_type  = "implements:s?,abstract:b?",
>           .mhandler.cmd_new = qmp_marshal_input_qom_list_types,
>       },
> +    {
> +        .name       = "qom-new",
> +        .args_type  = "parent:s,prop-name:s,type-name:s",
> +        .mhandler.cmd_new = qmp_marshal_input_qom_new,
> +    },
> diff --git a/qmp.c b/qmp.c
> index fee9fb2..cad5610 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -417,3 +417,30 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
>
>       return ret;
>   }
> +
> +char *qmp_qom_new(const char *parent, const char *prop_name,
> +                  const char *type_name, Error **errp)
> +{
> +    Object *p, *obj;
> +    Type type;
> +
> +    // TODO anything fancy with containger_get() needed?

I'm not sure how I feel about this.  I never intended for a user to be able to 
create objects that were arbitrary children of other objects.

In some ways, I think this is almost too powerful of an interface to expose to 
users.  I like things like device_add() better that only creates objects of 
TYPE_DEVICE that are always in /peripherial.

For block, we'd have a similar interface that always created objects of 
TYPE_BLOCK_DRIVER and put them in /block.

> +    p = object_resolve_path(parent, NULL);
> +    if (!p) {
> +        error_set(errp, QERR_DEVICE_NOT_FOUND, parent);
> +        return NULL;
> +    }
> +
> +    type = type_get_by_name(type_name);
> +    if (!type) {
> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> +                  "type-name", "a type name");
> +        return NULL;
> +    }
> +    obj = object_new_with_type(type);
> +
> +    // TODO bombs if p is an interface object; can this happen?

All interface types have .abstract set and there's an assert() to validate that 
an object isn't abstract when creating.  So it should be object_new_with_type() 
that's asserting, not object_property_add_child.

We'll probably want to enforce that qom-new isn't given an abstract type by 
introducing a type_is_abstract() or something like that.

Regards,

Anthony Liguori

> +    object_property_add_child(p, prop_name, obj, NULL);
> +
> +    return object_get_canonical_path(obj);
> +}

Re: [Qemu-devel] [PATCH RFC 0/2] QMP command qom-new

[Anthony Liguori]

On 05/24/2012 07:32 AM, Andreas Färber wrote:
> Am 24.05.2012 13:43, schrieb Markus Armbruster:
>> Beware: second patch is the product of voodoo-coding.
>
> Hm, I don't like the voodoo. ;) I would rather expose a proper C API
> like object_try_new(const char *, Error **) than opening up the TypeImpl
> internals to the public and hand-coding it everywhere. I ran into a
> similar error-catching scenario where I needed to check for class
> existence in some qdev_try_* function.

There are very few places where errors can be handled gracefully.  They are 
exceptions and can be treated as such.

I think it's far better for the QOM infrastructure to assert when it detects 
something bad because 99% of the users of QOM do not even attempt to handle 
errors gracefully.

Regards,

Anthony Liguori

  And there were still too many
> asserts in the QOM core for my taste.
>
> The QMP command itself looks good to me.
>
> Andreas
>
>> Markus Armbruster (2):
>>    qom: Give type_get_by_name() external linkage
>>    qmp: New command qom-new
>>
>>   include/qemu/object.h |    8 ++++++++
>>   qapi-schema.json      |   22 ++++++++++++++++++++++
>>   qmp-commands.hx       |    5 +++++
>>   qmp.c                 |   27 +++++++++++++++++++++++++++
>>   qom/object.c          |    2 +-
>>   5 files changed, 63 insertions(+), 1 deletions(-)
>

Re: [Qemu-devel] [PATCH RFC 0/2] QMP command qom-new

[Peter Maydell]

On 24 May 2012 14:06, Anthony Liguori <anthony@codemonkey.ws> wrote:
> There are very few places where errors can be handled gracefully.  They are
> exceptions and can be treated as such.
>
> I think it's far better for the QOM infrastructure to assert when it detects
> something bad because 99% of the users of QOM do not even attempt to handle
> errors gracefully

Last time I was trying to argue for keeping the "create, set properties,
realize" interface for devices/objects as simple as possible you wanted
it to have an error-return interface rather than asserting...

-- PMM

Re: [Qemu-devel] [PATCH RFC 2/2] qmp: New command qom-new

[Paolo Bonzini]

Il 24/05/2012 14:58, Anthony Liguori ha scritto:
>>
>> We should change that to a parent_bus property,
> 
> parent_bus is a property with the QOM bus series

I missed that... BTW do we need an object_property_add_link_readonly?

Paolo

Re: [Qemu-devel] [PATCH RFC 2/2] qmp: New command qom-new

[Igor Mammedov]

On 05/24/2012 03:04 PM, Anthony Liguori wrote:
> On 05/24/2012 06:43 AM, Markus Armbruster wrote:
>> To create objects via QMP.
>>
>> Test case:
>>
>> $ upstream-qemu --enable-kvm -S -m 384 -vnc :0 -monitor stdio -chardev socket,id=qmp,path=test-qmp,server=on,wait=off -mon mode=control,chardev=qmp
>>
>> Conversation on the qmp socket:
>> {"QMP": {"version": {"qemu": {"micro": 93, "minor": 0, "major": 1}, "package": ""}, "capabilities": []}}
>> { "execute": "qmp_capabilities" }
>> {"return": {}}
>> {"execute":"qom-new","arguments":{"parent":"/xxx", "prop-name":"test-qmp-new", "type-name":"xxx"}}
>> {"error": {"class": "DeviceNotFound", "desc": "Device '/xxx' not found", "data": {"device": "/xxx"}}}
>> {"execute":"qom-new","arguments":{"parent":"/machine", "prop-name":"test-qmp-new", "type-name":"xxx"}}
>> {"error": {"class": "InvalidParameterValue", "desc": "Parameter 'type-name' expects a type name", "data": {"name": "type-name", "expected": "a type
>> name"}}}
>> {"execute":"qom-new","arguments":{"parent":"/machine", "prop-name":"test-qmp-new", "type-name":"container"}}
>> {"return": "/machine/test-qmp-new"}
>> {"execute":"qom-list","arguments":{"path":"/machine"}}
>> {"return": [{"name": "test-qmp-new", "type": "child<container>"}, {"name": "i440fx", "type": "child<i440FX-pcihost>"}, {"name": "unattached",
>> "type": "child<container>"}, {"name": "peripheral", "type": "child<container>"}, {"name": "peripheral-anon", "type": "child<container>"}]}
>>
>> Note: qdev objects (subtype of TYPE_DEVICE) created with qom-new lack
>> additional magic performed by qdev_try_create(), and almost certainly
>> won't work.
>>
>> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>> ---
>> qapi-schema.json | 22 ++++++++++++++++++++++
>> qmp-commands.hx | 5 +++++
>> qmp.c | 27 +++++++++++++++++++++++++++
>> 3 files changed, 54 insertions(+), 0 deletions(-)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 2ca7195..ab9e68b 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1699,6 +1699,28 @@
>> 'returns': [ 'ObjectTypeInfo' ] }
>>
>> ##
>> +# qom-new:
>> +#
>> +# Create a new object
>> +#
>> +# @parent: the parent's path within the object model. See @qom-get
>> +# for a description of paths.
>> +#
>> +# @prop-name: the name of the property to add to the parent.
>> +#
>> +# @type: the new object's type name
>> +#
>> +# Returns: The new object's canonical absolute path
>> +#
>> +# Since: 1.2
>> +#
>> +# Notes: This command is experimental and may change syntax in future releases.
>> +##
>> +{ 'command': 'qom-new',
>> + 'data': { 'parent': 'str', 'prop-name': 'str', 'type-name': 'str' },
>> + 'returns': 'str' }
>> +
>> +##
>> # @migrate
>> #
>> # Migrates the current running guest to another Virtual Machine.
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index db980fa..53adda2 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -2142,3 +2142,8 @@ EQMP
>> .args_type = "implements:s?,abstract:b?",
>> .mhandler.cmd_new = qmp_marshal_input_qom_list_types,
>> },
>> + {
>> + .name = "qom-new",
>> + .args_type = "parent:s,prop-name:s,type-name:s",
>> + .mhandler.cmd_new = qmp_marshal_input_qom_new,
>> + },
>> diff --git a/qmp.c b/qmp.c
>> index fee9fb2..cad5610 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -417,3 +417,30 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
>>
>> return ret;
>> }
>> +
>> +char *qmp_qom_new(const char *parent, const char *prop_name,
>> + const char *type_name, Error **errp)
>> +{
>> + Object *p, *obj;
>> + Type type;
>> +
>> + // TODO anything fancy with containger_get() needed?
>
> I'm not sure how I feel about this. I never intended for a user to be able to create objects that were arbitrary children of other objects.
>
> In some ways, I think this is almost too powerful of an interface to expose to users. I like things like device_add() better that only creates objects
> of TYPE_DEVICE that are always in /peripherial.
>
> For block, we'd have a similar interface that always created objects of TYPE_BLOCK_DRIVER and put them in /block.

Will we have a special cases for every incompatible device types that is going to be hot-plugged via device_add monitor command?

For CPUs my thoughts were moving in opposite direction, like:
  - make possible to create and initialize CPU as a regular QOM object
  - hack qdev_device_add() to allow not only TYPE_DEVICE to be created there

There are patches out there that make cpu a child of /machine at board level.
But for hot-added objects parent could be specified as a property
or knowledge about parent hard-coded inside of object itself or
hard-coded in device_add().
Which one of them likely to be adopted?

>> + p = object_resolve_path(parent, NULL);
>> + if (!p) {
>> + error_set(errp, QERR_DEVICE_NOT_FOUND, parent);
>> + return NULL;
>> + }
>> +
>> + type = type_get_by_name(type_name);
>> + if (!type) {
>> + error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> + "type-name", "a type name");
>> + return NULL;
>> + }
>> + obj = object_new_with_type(type);
>> +
>> + // TODO bombs if p is an interface object; can this happen?
>
> All interface types have .abstract set and there's an assert() to validate that an object isn't abstract when creating. So it should be
> object_new_with_type() that's asserting, not object_property_add_child.
>
> We'll probably want to enforce that qom-new isn't given an abstract type by introducing a type_is_abstract() or something like that.
>
> Regards,
>
> Anthony Liguori
>
>> + object_property_add_child(p, prop_name, obj, NULL);
>> +
>> + return object_get_canonical_path(obj);
>> +}
>
>

-- 
-----
  Igor

Re: [Qemu-devel] [PATCH RFC 2/2] qmp: New command qom-new

[Andreas Färber]

Am 24.05.2012 15:48, schrieb Igor Mammedov:
> On 05/24/2012 03:04 PM, Anthony Liguori wrote:
>>
>> I'm not sure how I feel about this. I never intended for a user to be
>> able to create objects that were arbitrary children of other objects.
>>
>> In some ways, I think this is almost too powerful of an interface to
>> expose to users. I like things like device_add() better that only
>> creates objects
>> of TYPE_DEVICE that are always in /peripherial.
>>
>> For block, we'd have a similar interface that always created objects
>> of TYPE_BLOCK_DRIVER and put them in /block.
> 
> Will we have a special cases for every incompatible device types that is
> going to be hot-plugged via device_add monitor command?
> 
> For CPUs my thoughts were moving in opposite direction, like:
>  - make possible to create and initialize CPU as a regular QOM object
>  - hack qdev_device_add() to allow not only TYPE_DEVICE to be created there
> 
> There are patches out there that make cpu a child of /machine at board
> level.
> But for hot-added objects parent could be specified as a property
> or knowledge about parent hard-coded inside of object itself or
> hard-coded in device_add().
> Which one of them likely to be adopted?

For system emulation I am working towards making the CPU a device so
that we can reuse common device infrastructure:

https://github.com/afaerber/qemu-cpu/commits/qom-cpu-dev

That's independent of what QMP commands we provide to the user though.

If we created a TYPE_X86_CPU with -device, we would not get an APIC
attached currently.
If however we created a container object as suggested by Peter and
others before, then we cannot as easily modify properties of the child
objects (family, vendor, etc. of CPU) via command line. Same issue as
with SoCs (the sh7750 realize discussion).

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 RFC 2/2] qmp: New command qom-new

[Anthony Liguori]

On 05/24/2012 08:48 AM, Igor Mammedov wrote:
> On 05/24/2012 03:04 PM, Anthony Liguori wrote:
>> On 05/24/2012 06:43 AM, Markus Armbruster wrote:
>>> To create objects via QMP.
>>>
>>> Test case:
>>>
>>> $ upstream-qemu --enable-kvm -S -m 384 -vnc :0 -monitor stdio -chardev
>>> socket,id=qmp,path=test-qmp,server=on,wait=off -mon mode=control,chardev=qmp
>>>
>>> Conversation on the qmp socket:
>>> {"QMP": {"version": {"qemu": {"micro": 93, "minor": 0, "major": 1},
>>> "package": ""}, "capabilities": []}}
>>> { "execute": "qmp_capabilities" }
>>> {"return": {}}
>>> {"execute":"qom-new","arguments":{"parent":"/xxx",
>>> "prop-name":"test-qmp-new", "type-name":"xxx"}}
>>> {"error": {"class": "DeviceNotFound", "desc": "Device '/xxx' not found",
>>> "data": {"device": "/xxx"}}}
>>> {"execute":"qom-new","arguments":{"parent":"/machine",
>>> "prop-name":"test-qmp-new", "type-name":"xxx"}}
>>> {"error": {"class": "InvalidParameterValue", "desc": "Parameter 'type-name'
>>> expects a type name", "data": {"name": "type-name", "expected": "a type
>>> name"}}}
>>> {"execute":"qom-new","arguments":{"parent":"/machine",
>>> "prop-name":"test-qmp-new", "type-name":"container"}}
>>> {"return": "/machine/test-qmp-new"}
>>> {"execute":"qom-list","arguments":{"path":"/machine"}}
>>> {"return": [{"name": "test-qmp-new", "type": "child<container>"}, {"name":
>>> "i440fx", "type": "child<i440FX-pcihost>"}, {"name": "unattached",
>>> "type": "child<container>"}, {"name": "peripheral", "type":
>>> "child<container>"}, {"name": "peripheral-anon", "type": "child<container>"}]}
>>>
>>> Note: qdev objects (subtype of TYPE_DEVICE) created with qom-new lack
>>> additional magic performed by qdev_try_create(), and almost certainly
>>> won't work.
>>>
>>> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>>> ---
>>> qapi-schema.json | 22 ++++++++++++++++++++++
>>> qmp-commands.hx | 5 +++++
>>> qmp.c | 27 +++++++++++++++++++++++++++
>>> 3 files changed, 54 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/qapi-schema.json b/qapi-schema.json
>>> index 2ca7195..ab9e68b 100644
>>> --- a/qapi-schema.json
>>> +++ b/qapi-schema.json
>>> @@ -1699,6 +1699,28 @@
>>> 'returns': [ 'ObjectTypeInfo' ] }
>>>
>>> ##
>>> +# qom-new:
>>> +#
>>> +# Create a new object
>>> +#
>>> +# @parent: the parent's path within the object model. See @qom-get
>>> +# for a description of paths.
>>> +#
>>> +# @prop-name: the name of the property to add to the parent.
>>> +#
>>> +# @type: the new object's type name
>>> +#
>>> +# Returns: The new object's canonical absolute path
>>> +#
>>> +# Since: 1.2
>>> +#
>>> +# Notes: This command is experimental and may change syntax in future releases.
>>> +##
>>> +{ 'command': 'qom-new',
>>> + 'data': { 'parent': 'str', 'prop-name': 'str', 'type-name': 'str' },
>>> + 'returns': 'str' }
>>> +
>>> +##
>>> # @migrate
>>> #
>>> # Migrates the current running guest to another Virtual Machine.
>>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>>> index db980fa..53adda2 100644
>>> --- a/qmp-commands.hx
>>> +++ b/qmp-commands.hx
>>> @@ -2142,3 +2142,8 @@ EQMP
>>> .args_type = "implements:s?,abstract:b?",
>>> .mhandler.cmd_new = qmp_marshal_input_qom_list_types,
>>> },
>>> + {
>>> + .name = "qom-new",
>>> + .args_type = "parent:s,prop-name:s,type-name:s",
>>> + .mhandler.cmd_new = qmp_marshal_input_qom_new,
>>> + },
>>> diff --git a/qmp.c b/qmp.c
>>> index fee9fb2..cad5610 100644
>>> --- a/qmp.c
>>> +++ b/qmp.c
>>> @@ -417,3 +417,30 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
>>>
>>> return ret;
>>> }
>>> +
>>> +char *qmp_qom_new(const char *parent, const char *prop_name,
>>> + const char *type_name, Error **errp)
>>> +{
>>> + Object *p, *obj;
>>> + Type type;
>>> +
>>> + // TODO anything fancy with containger_get() needed?
>>
>> I'm not sure how I feel about this. I never intended for a user to be able to
>> create objects that were arbitrary children of other objects.
>>
>> In some ways, I think this is almost too powerful of an interface to expose to
>> users. I like things like device_add() better that only creates objects
>> of TYPE_DEVICE that are always in /peripherial.
>>
>> For block, we'd have a similar interface that always created objects of
>> TYPE_BLOCK_DRIVER and put them in /block.
>
> Will we have a special cases for every incompatible device types that is going
> to be hot-plugged via device_add monitor command?
>
> For CPUs my thoughts were moving in opposite direction, like:
> - make possible to create and initialize CPU as a regular QOM object

I think we should make CPUs a TYPE_DEVICE.  That hasn't been possible yet but 
with the bus conversion series, it will be.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH RFC 2/2] qmp: New command qom-new

[Anthony Liguori]

On 05/24/2012 09:01 AM, Andreas Färber wrote:
> Am 24.05.2012 15:48, schrieb Igor Mammedov:
>> On 05/24/2012 03:04 PM, Anthony Liguori wrote:
>>>
>>> I'm not sure how I feel about this. I never intended for a user to be
>>> able to create objects that were arbitrary children of other objects.
>>>
>>> In some ways, I think this is almost too powerful of an interface to
>>> expose to users. I like things like device_add() better that only
>>> creates objects
>>> of TYPE_DEVICE that are always in /peripherial.
>>>
>>> For block, we'd have a similar interface that always created objects
>>> of TYPE_BLOCK_DRIVER and put them in /block.
>>
>> Will we have a special cases for every incompatible device types that is
>> going to be hot-plugged via device_add monitor command?
>>
>> For CPUs my thoughts were moving in opposite direction, like:
>>   - make possible to create and initialize CPU as a regular QOM object
>>   - hack qdev_device_add() to allow not only TYPE_DEVICE to be created there
>>
>> There are patches out there that make cpu a child of /machine at board
>> level.
>> But for hot-added objects parent could be specified as a property
>> or knowledge about parent hard-coded inside of object itself or
>> hard-coded in device_add().
>> Which one of them likely to be adopted?
>
> For system emulation I am working towards making the CPU a device so
> that we can reuse common device infrastructure:

Ah, perfect, I guess great minds think alike :-)

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH RFC 0/2] QMP command qom-new

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 24.05.2012 13:43, schrieb Markus Armbruster:
>> Beware: second patch is the product of voodoo-coding.
>
> Hm, I don't like the voodoo. ;) I would rather expose a proper C API
> like object_try_new(const char *, Error **) than opening up the TypeImpl
> internals to the public and hand-coding it everywhere.

How does returning a TypeImpl * open up TypeImpl any more than
type_register() already does?

>                                                        I ran into a
> similar error-catching scenario where I needed to check for class
> existence in some qdev_try_* function.

If this is a really common pattern, and object_try_new() really saves
code, why not.

Can't see why we need the Error **argument, though.  What kinds of
different errors that do you envisage?  Where "different" means "actual
callers care about the difference".

>                                        And there were still too many
> asserts in the QOM core for my taste.
>
> The QMP command itself looks good to me.

Thanks!

Re: [Qemu-devel] [PATCH RFC 0/2] QMP command qom-new

[Anthony Liguori]

On 05/24/2012 08:18 AM, Peter Maydell wrote:
> On 24 May 2012 14:06, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> There are very few places where errors can be handled gracefully.  They are
>> exceptions and can be treated as such.
>>
>> I think it's far better for the QOM infrastructure to assert when it detects
>> something bad because 99% of the users of QOM do not even attempt to handle
>> errors gracefully
>
> Last time I was trying to argue for keeping the "create, set properties,
> realize" interface for devices/objects as simple as possible you wanted
> it to have an error-return interface rather than asserting...

You need to be more specific than that I'm afraid..

Regards,

Anthony Liguori

>
> -- PMM

Re: [Qemu-devel] [PATCH RFC 2/2] qmp: New command qom-new

[Anthony Liguori]

On 05/24/2012 08:44 AM, Paolo Bonzini wrote:
> Il 24/05/2012 14:58, Anthony Liguori ha scritto:
>>>
>>> We should change that to a parent_bus property,
>>
>> parent_bus is a property with the QOM bus series
>
> I missed that... BTW do we need an object_property_add_link_readonly?

Do we want it to be a read-only link?

I thought everything becomes read-only after realize...

BTW, my plan for parent_bus is to eventually remove it from DeviceState and push 
it into the bus-specific base classes so that instead of link<BusState> it can 
become link<PCIBusState>.

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [PATCH RFC 0/2] QMP command qom-new

[Peter Maydell]

On 24 May 2012 15:10, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 05/24/2012 08:18 AM, Peter Maydell wrote:
>> Last time I was trying to argue for keeping the "create, set properties,
>> realize" interface for devices/objects as simple as possible you wanted
>> it to have an error-return interface rather than asserting...
>
>
> You need to be more specific than that I'm afraid..

https://lists.gnu.org/archive/html/qemu-devel/2012-05/msg00079.html

-- PMM

Re: [Qemu-devel] [PATCH RFC 2/2] qmp: New command qom-new

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> On 05/24/2012 06:43 AM, Markus Armbruster wrote:
>> To create objects via QMP.
>>
>> Test case:
>>
>> $ upstream-qemu --enable-kvm -S -m 384 -vnc :0 -monitor stdio -chardev socket,id=qmp,path=test-qmp,server=on,wait=off -mon mode=control,chardev=qmp
>>
>> Conversation on the qmp socket:
>> {"QMP": {"version": {"qemu": {"micro": 93, "minor": 0, "major": 1}, "package": ""}, "capabilities": []}}
>> { "execute": "qmp_capabilities" }
>> {"return": {}}
>> {"execute":"qom-new","arguments":{"parent":"/xxx", "prop-name":"test-qmp-new", "type-name":"xxx"}}
>> {"error": {"class": "DeviceNotFound", "desc": "Device '/xxx' not found", "data": {"device": "/xxx"}}}
>> {"execute":"qom-new","arguments":{"parent":"/machine", "prop-name":"test-qmp-new", "type-name":"xxx"}}
>> {"error": {"class": "InvalidParameterValue", "desc": "Parameter 'type-name' expects a type name", "data": {"name": "type-name", "expected": "a type name"}}}
>> {"execute":"qom-new","arguments":{"parent":"/machine", "prop-name":"test-qmp-new", "type-name":"container"}}
>> {"return": "/machine/test-qmp-new"}
>> {"execute":"qom-list","arguments":{"path":"/machine"}}
>> {"return": [{"name": "test-qmp-new", "type": "child<container>"}, {"name": "i440fx", "type": "child<i440FX-pcihost>"}, {"name": "unattached", "type": "child<container>"}, {"name": "peripheral", "type": "child<container>"}, {"name": "peripheral-anon", "type": "child<container>"}]}
>>
>> Note: qdev objects (subtype of TYPE_DEVICE) created with qom-new lack
>> additional magic performed by qdev_try_create(), and almost certainly
>> won't work.
>>
>> Signed-off-by: Markus Armbruster<armbru@redhat.com>
>> ---
>>   qapi-schema.json |   22 ++++++++++++++++++++++
>>   qmp-commands.hx  |    5 +++++
>>   qmp.c            |   27 +++++++++++++++++++++++++++
>>   3 files changed, 54 insertions(+), 0 deletions(-)
>>
>> diff --git a/qapi-schema.json b/qapi-schema.json
>> index 2ca7195..ab9e68b 100644
>> --- a/qapi-schema.json
>> +++ b/qapi-schema.json
>> @@ -1699,6 +1699,28 @@
>>     'returns': [ 'ObjectTypeInfo' ] }
>>
>>   ##
>> +# qom-new:
>> +#
>> +# Create a new object
>> +#
>> +# @parent: the parent's path within the object model.  See @qom-get
>> +# for a description of paths.
>> +#
>> +# @prop-name: the name of the property to add to the parent.
>> +#
>> +# @type: the new object's type name
>> +#
>> +# Returns: The new object's canonical absolute path
>> +#
>> +# Since: 1.2
>> +#
>> +# Notes: This command is experimental and may change syntax in future releases.
>> +##
>> +{ 'command': 'qom-new',
>> +  'data': { 'parent': 'str', 'prop-name': 'str', 'type-name': 'str' },
>> +  'returns': 'str' }
>> +
>> +##
>>   # @migrate
>>   #
>>   # Migrates the current running guest to another Virtual Machine.
>> diff --git a/qmp-commands.hx b/qmp-commands.hx
>> index db980fa..53adda2 100644
>> --- a/qmp-commands.hx
>> +++ b/qmp-commands.hx
>> @@ -2142,3 +2142,8 @@ EQMP
>>           .args_type  = "implements:s?,abstract:b?",
>>           .mhandler.cmd_new = qmp_marshal_input_qom_list_types,
>>       },
>> +    {
>> +        .name       = "qom-new",
>> +        .args_type  = "parent:s,prop-name:s,type-name:s",
>> +        .mhandler.cmd_new = qmp_marshal_input_qom_new,
>> +    },
>> diff --git a/qmp.c b/qmp.c
>> index fee9fb2..cad5610 100644
>> --- a/qmp.c
>> +++ b/qmp.c
>> @@ -417,3 +417,30 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
>>
>>       return ret;
>>   }
>> +
>> +char *qmp_qom_new(const char *parent, const char *prop_name,
>> +                  const char *type_name, Error **errp)
>> +{
>> +    Object *p, *obj;
>> +    Type type;
>> +
>> +    // TODO anything fancy with containger_get() needed?
>
> I'm not sure how I feel about this.  I never intended for a user to be
> able to create objects that were arbitrary children of other objects.
>
> In some ways, I think this is almost too powerful of an interface to
> expose to users.  I like things like device_add() better that only
> creates objects of TYPE_DEVICE that are always in /peripherial.
>
> For block, we'd have a similar interface that always created objects
> of TYPE_BLOCK_DRIVER and put them in /block.

Well, it's what you asked for yesterday on IRC :)

Personally, I've never had a problem with giving users more rope.  But I
understand your hesitation.

Commands to create more special kinds of objects are fine with me, as
long as they effectively expand into basic QOM operations, and nothing
else.

Making the basic operations available in QMP as well might help keep us
honest there.

My immediate reason for wanting qom-new goes away when device_add can
create devices that are linked from another device without a qbus in
between, because then I can create fdd devices without also creating
qbus for connecting them to their fdc.  Unless I misunderstood
something, that should land in branch qom-next within days.

What I need to know from you is whether I should develop these patches
further, or shelve them.

>> +    p = object_resolve_path(parent, NULL);
>> +    if (!p) {
>> +        error_set(errp, QERR_DEVICE_NOT_FOUND, parent);
>> +        return NULL;
>> +    }
>> +
>> +    type = type_get_by_name(type_name);
>> +    if (!type) {
>> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
>> +                  "type-name", "a type name");
>> +        return NULL;
>> +    }
>> +    obj = object_new_with_type(type);
>> +
>> +    // TODO bombs if p is an interface object; can this happen?
>
> All interface types have .abstract set and there's an assert() to
> validate that an object isn't abstract when creating.  So it should be
> object_new_with_type() that's asserting, not
> object_property_add_child.

Hmm, looks like that one asserts, too, in object_init_with_type().

> We'll probably want to enforce that qom-new isn't given an abstract
> type by introducing a type_is_abstract() or something like that.

Makes sense.

> Regards,
>
> Anthony Liguori
>
>> +    object_property_add_child(p, prop_name, obj, NULL);
>> +
>> +    return object_get_canonical_path(obj);
>> +}

Re: [Qemu-devel] [PATCH RFC 0/2] QMP command qom-new

[Andreas Färber]

Am 24.05.2012 16:08, schrieb Markus Armbruster:
> Andreas Färber <afaerber@suse.de> writes:
> 
>> Am 24.05.2012 13:43, schrieb Markus Armbruster:
>>> Beware: second patch is the product of voodoo-coding.
>>
>> Hm, I don't like the voodoo. ;) I would rather expose a proper C API
>> like object_try_new(const char *, Error **) than opening up the TypeImpl
>> internals to the public and hand-coding it everywhere.
> 
> How does returning a TypeImpl * open up TypeImpl any more than
> type_register() already does?

type_register[_static]() operates on TypeInfo, not TypeImpl.

I consider TypeImpl an implementation detail of qom/object.c, but maybe
I'm mistaken.

>> similar error-catching scenario where I needed to check for class
>> existence in some qdev_try_* function.
> 
> If this is a really common pattern, and object_try_new() really saves
> code, why not.
> 
> Can't see why we need the Error **argument, though.  What kinds of
> different errors that do you envisage?  Where "different" means "actual
> callers care about the difference".

Well, I thought of class-doesn't-exist vs. out-of-memory. It would just
be to report different textual messages to the end user - as QMP error
response or as stderr output elsewhere. I have better OOM handling
somewhere down my TODO list, including a preallocated Error object. Main
reason is avoiding code duplication though.

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 RFC 0/2] QMP command qom-new

[Anthony Liguori]

On 05/24/2012 09:23 AM, Peter Maydell wrote:
> On 24 May 2012 15:10, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> On 05/24/2012 08:18 AM, Peter Maydell wrote:
>>> Last time I was trying to argue for keeping the "create, set properties,
>>> realize" interface for devices/objects as simple as possible you wanted
>>> it to have an error-return interface rather than asserting...
>>
>>
>> You need to be more specific than that I'm afraid..
>
> https://lists.gnu.org/archive/html/qemu-devel/2012-05/msg00079.html

Ah, okay.  So you're taking those comments a bit out of context.

The right way to use object_new() is:

Object *foo = object_new(TYPE_E1000);

If you mistype TYPE_E1000, you should get a compile failure.  There's basically 
no way that this is going to fail unless you do:

Object *foo = object_new("garbage");

There's no point in adding error handling here because if you make this much of 
a mistake, then you're likely not going to handle errors properly.  That's 
entirely different from:

connect(foo, "baz", bar, "caz");

It's extremely likely that you'll mistype one of the property names.  We have 
existence proofs in the three with qdev_prop_set*().  This can be deep within 
the bowels of device initialization and assert()'ing QEMU doing a device_add is 
a pretty nasty thing to do.  There's also type errors that can occur here that 
are even more likely to happen by accident.

Contrast that with:

foo->baz = &bar->caz;

If you mistype the property names or mess up the types, you will get a compile 
error.

Regards,

Anthony Liguori

>
> -- PMM

Re: [Qemu-devel] [PATCH RFC 0/2] QMP command qom-new

[Markus Armbruster]

Andreas Färber <afaerber@suse.de> writes:

> Am 24.05.2012 16:08, schrieb Markus Armbruster:
>> Andreas Färber <afaerber@suse.de> writes:
>> 
>>> Am 24.05.2012 13:43, schrieb Markus Armbruster:
>>>> Beware: second patch is the product of voodoo-coding.
>>>
>>> Hm, I don't like the voodoo. ;) I would rather expose a proper C API
>>> like object_try_new(const char *, Error **) than opening up the TypeImpl
>>> internals to the public and hand-coding it everywhere.
>> 
>> How does returning a TypeImpl * open up TypeImpl any more than
>> type_register() already does?
>
> type_register[_static]() operates on TypeInfo, not TypeImpl.
>
> I consider TypeImpl an implementation detail of qom/object.c, but maybe
> I'm mistaken.

They return TypeImpl *, just like type_get_by_name().

[...]

Re: [Qemu-devel] [PATCH RFC 0/2] QMP command qom-new

[Peter Maydell]

On 24 May 2012 15:35, Anthony Liguori <anthony@codemonkey.ws> wrote:
> Ah, okay.  So you're taking those comments a bit out of context.
>
> The right way to use object_new() is:
>
> Object *foo = object_new(TYPE_E1000);
>
> If you mistype TYPE_E1000, you should get a compile failure.  There's
> basically no way that this is going to fail unless you do:
>
> Object *foo = object_new("garbage");

But mostly for types we don't in fact have preprocessor defines for
them unless they're subclasses. The average device is created by
passing its name as a plain string. That's just as liable to typos
as property names are. Conversely, you could define preprocessor
macros for all of a device's property names if you thought it would
help.

> There's no point in adding error handling here because if you make this much
> of a mistake, then you're likely not going to handle errors properly.
>  That's entirely different from:
>
> connect(foo, "baz", bar, "caz");
>
> It's extremely likely that you'll mistype one of the property names.  We
> have existence proofs in the three with qdev_prop_set*().  This can be deep
> within the bowels of device initialization and assert()'ing QEMU doing a
> device_add is a pretty nasty thing to do.

If a device is so untested that nobody has ever even attempted to
create it, then you're asking for trouble trying to do anything
with it in a production system. It could just as easily do an accidental
NULL dereference in its init function and kill QEMU that way.

I think this is exactly a case where we want "QOM infrastructure to
assert when it detects something bad" because it's not reasonable
for callers to do error handling for can't-happen situations.

> Contrast that with:
>
> foo->baz = &bar->caz;

...which isn't the way QOM has been set up to work, and in
any case doesn't really work with dynamically created properties.

-- PMM

Re: [Qemu-devel] [PATCH RFC 2/2] qmp: New command qom-new

[Michael Roth]

On Thu, May 24, 2012 at 04:24:53PM +0200, Markus Armbruster wrote:
> Anthony Liguori <anthony@codemonkey.ws> writes:
> 
> > On 05/24/2012 06:43 AM, Markus Armbruster wrote:
> >> To create objects via QMP.
> >>
> >> Test case:
> >>
> >> $ upstream-qemu --enable-kvm -S -m 384 -vnc :0 -monitor stdio -chardev socket,id=qmp,path=test-qmp,server=on,wait=off -mon mode=control,chardev=qmp
> >>
> >> Conversation on the qmp socket:
> >> {"QMP": {"version": {"qemu": {"micro": 93, "minor": 0, "major": 1}, "package": ""}, "capabilities": []}}
> >> { "execute": "qmp_capabilities" }
> >> {"return": {}}
> >> {"execute":"qom-new","arguments":{"parent":"/xxx", "prop-name":"test-qmp-new", "type-name":"xxx"}}
> >> {"error": {"class": "DeviceNotFound", "desc": "Device '/xxx' not found", "data": {"device": "/xxx"}}}
> >> {"execute":"qom-new","arguments":{"parent":"/machine", "prop-name":"test-qmp-new", "type-name":"xxx"}}
> >> {"error": {"class": "InvalidParameterValue", "desc": "Parameter 'type-name' expects a type name", "data": {"name": "type-name", "expected": "a type name"}}}
> >> {"execute":"qom-new","arguments":{"parent":"/machine", "prop-name":"test-qmp-new", "type-name":"container"}}
> >> {"return": "/machine/test-qmp-new"}
> >> {"execute":"qom-list","arguments":{"path":"/machine"}}
> >> {"return": [{"name": "test-qmp-new", "type": "child<container>"}, {"name": "i440fx", "type": "child<i440FX-pcihost>"}, {"name": "unattached", "type": "child<container>"}, {"name": "peripheral", "type": "child<container>"}, {"name": "peripheral-anon", "type": "child<container>"}]}
> >>
> >> Note: qdev objects (subtype of TYPE_DEVICE) created with qom-new lack
> >> additional magic performed by qdev_try_create(), and almost certainly
> >> won't work.
> >>
> >> Signed-off-by: Markus Armbruster<armbru@redhat.com>
> >> ---
> >>   qapi-schema.json |   22 ++++++++++++++++++++++
> >>   qmp-commands.hx  |    5 +++++
> >>   qmp.c            |   27 +++++++++++++++++++++++++++
> >>   3 files changed, 54 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/qapi-schema.json b/qapi-schema.json
> >> index 2ca7195..ab9e68b 100644
> >> --- a/qapi-schema.json
> >> +++ b/qapi-schema.json
> >> @@ -1699,6 +1699,28 @@
> >>     'returns': [ 'ObjectTypeInfo' ] }
> >>
> >>   ##
> >> +# qom-new:
> >> +#
> >> +# Create a new object
> >> +#
> >> +# @parent: the parent's path within the object model.  See @qom-get
> >> +# for a description of paths.
> >> +#
> >> +# @prop-name: the name of the property to add to the parent.
> >> +#
> >> +# @type: the new object's type name
> >> +#
> >> +# Returns: The new object's canonical absolute path
> >> +#
> >> +# Since: 1.2
> >> +#
> >> +# Notes: This command is experimental and may change syntax in future releases.
> >> +##
> >> +{ 'command': 'qom-new',
> >> +  'data': { 'parent': 'str', 'prop-name': 'str', 'type-name': 'str' },
> >> +  'returns': 'str' }
> >> +
> >> +##
> >>   # @migrate
> >>   #
> >>   # Migrates the current running guest to another Virtual Machine.
> >> diff --git a/qmp-commands.hx b/qmp-commands.hx
> >> index db980fa..53adda2 100644
> >> --- a/qmp-commands.hx
> >> +++ b/qmp-commands.hx
> >> @@ -2142,3 +2142,8 @@ EQMP
> >>           .args_type  = "implements:s?,abstract:b?",
> >>           .mhandler.cmd_new = qmp_marshal_input_qom_list_types,
> >>       },
> >> +    {
> >> +        .name       = "qom-new",
> >> +        .args_type  = "parent:s,prop-name:s,type-name:s",
> >> +        .mhandler.cmd_new = qmp_marshal_input_qom_new,
> >> +    },
> >> diff --git a/qmp.c b/qmp.c
> >> index fee9fb2..cad5610 100644
> >> --- a/qmp.c
> >> +++ b/qmp.c
> >> @@ -417,3 +417,30 @@ ObjectTypeInfoList *qmp_qom_list_types(bool has_implements,
> >>
> >>       return ret;
> >>   }
> >> +
> >> +char *qmp_qom_new(const char *parent, const char *prop_name,
> >> +                  const char *type_name, Error **errp)
> >> +{
> >> +    Object *p, *obj;
> >> +    Type type;
> >> +
> >> +    // TODO anything fancy with containger_get() needed?
> >
> > I'm not sure how I feel about this.  I never intended for a user to be
> > able to create objects that were arbitrary children of other objects.
> >
> > In some ways, I think this is almost too powerful of an interface to
> > expose to users.  I like things like device_add() better that only
> > creates objects of TYPE_DEVICE that are always in /peripherial.
> >
> > For block, we'd have a similar interface that always created objects
> > of TYPE_BLOCK_DRIVER and put them in /block.
> 
> Well, it's what you asked for yesterday on IRC :)
> 
> Personally, I've never had a problem with giving users more rope.  But I
> understand your hesitation.
> 
> Commands to create more special kinds of objects are fine with me, as
> long as they effectively expand into basic QOM operations, and nothing
> else.
> 
> Making the basic operations available in QMP as well might help keep us
> honest there.
> 
> My immediate reason for wanting qom-new goes away when device_add can
> create devices that are linked from another device without a qbus in
> between, because then I can create fdd devices without also creating
> qbus for connecting them to their fdc.  Unless I misunderstood
> something, that should land in branch qom-next within days.
> 
> What I need to know from you is whether I should develop these patches
> further, or shelve them.
> 
> >> +    p = object_resolve_path(parent, NULL);
> >> +    if (!p) {
> >> +        error_set(errp, QERR_DEVICE_NOT_FOUND, parent);
> >> +        return NULL;
> >> +    }
> >> +
> >> +    type = type_get_by_name(type_name);
> >> +    if (!type) {
> >> +        error_set(errp, QERR_INVALID_PARAMETER_VALUE,
> >> +                  "type-name", "a type name");
> >> +        return NULL;
> >> +    }
> >> +    obj = object_new_with_type(type);
> >> +
> >> +    // TODO bombs if p is an interface object; can this happen?
> >
> > All interface types have .abstract set and there's an assert() to
> > validate that an object isn't abstract when creating.  So it should be
> > object_new_with_type() that's asserting, not
> > object_property_add_child.
> 
> Hmm, looks like that one asserts, too, in object_init_with_type().

Yah, when I messed around with interfaces in the past I needed the
following changes to avoid an assert, and beyond that a segfault. I'm not
sure how correct this is though: 

    qom: fixes for Interfaces
    
    Don't assert type->abstract == false
    Some abstract classes are still instantiated internally,
    such as for instances of InterfaceClass.
    
    Double check the necessity of this.
    
    Interfaces are always instantiated as Interface objects, whose
    InterfaceClass may be smaller than that of the actual interface class
    that is subclassing it, so use the subclass size instead to make sure we
    allocate enough space to store the virtual functions of the interface.

---
diff --git a/qom/object.c b/qom/object.c
index 39cbcb9..939f0c2 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -186,8 +186,12 @@ static void type_class_interface_init(TypeImpl *ti, InterfaceImpl *iface)
         .class_init = iface->interface_initfn,
         .abstract = true,
     };
+    TypeImpl *parent_ti = type_get_by_name(iface->parent);
     char *name = g_strdup_printf("<%s::%s>", ti->name, iface->parent);
 
+    if (parent_ti) {
+        info.class_size = type_class_get_size(parent_ti);
+    }
     info.name = name;
     iface->type = type_register(&info);
     g_free(name);
@@ -267,7 +271,6 @@ void object_initialize_with_type(void *data, TypeImpl *type)
     g_assert(type->instance_size >= sizeof(Object));
 
     type_class_init(type);
-    g_assert(type->abstract == false);
 
     memset(obj, 0, type->instance_size);
     obj->class = type->class;

Re: [Qemu-devel] [PATCH RFC 2/2] qmp: New command qom-new

[Igor Mammedov]

On 05/24/2012 04:01 PM, Andreas Färber wrote:
> Am 24.05.2012 15:48, schrieb Igor Mammedov:
>> On 05/24/2012 03:04 PM, Anthony Liguori wrote:
>>>
>>> I'm not sure how I feel about this. I never intended for a user to be
>>> able to create objects that were arbitrary children of other objects.
>>>
>>> In some ways, I think this is almost too powerful of an interface to
>>> expose to users. I like things like device_add() better that only
>>> creates objects
>>> of TYPE_DEVICE that are always in /peripherial.
>>>
>>> For block, we'd have a similar interface that always created objects
>>> of TYPE_BLOCK_DRIVER and put them in /block.
>>
>> Will we have a special cases for every incompatible device types that is
>> going to be hot-plugged via device_add monitor command?
>>
>> For CPUs my thoughts were moving in opposite direction, like:
>>   - make possible to create and initialize CPU as a regular QOM object
>>   - hack qdev_device_add() to allow not only TYPE_DEVICE to be created there
>>
>> There are patches out there that make cpu a child of /machine at board
>> level.
>> But for hot-added objects parent could be specified as a property
>> or knowledge about parent hard-coded inside of object itself or
>> hard-coded in device_add().
>> Which one of them likely to be adopted?
>
> For system emulation I am working towards making the CPU a device so
> that we can reuse common device infrastructure:
>
> https://github.com/afaerber/qemu-cpu/commits/qom-cpu-dev
>
> That's independent of what QMP commands we provide to the user though.
Thanks for the link and job you've done on cpus. It looks much smaller/cleaner
than when I've attempted to do it several months ago, that was big and ugly hack.

> If we created a TYPE_X86_CPU with -device, we would not get an APIC
> attached currently.
That is why I'm adding intermediate cpu_model property for now, so that
it would possible to create TYPE_X86_CPU and set cpu_model property which
would create APIC if cpu advertises it.
Then when cpu subclasses implemented and possibly cpu features are converted
to properties, we could move APIC creation to a more appropriate place and
drop cpu_model property.

> If however we created a container object as suggested by Peter and
> others before, then we cannot as easily modify properties of the child
> objects (family, vendor, etc. of CPU) via command line. Same issue as
Yep, that would complicate things. I don't like it for x86 because
container device would be just dumb chip packaging if we try to match it
with a real hardware and as you say it would be difficult to use it.

> with SoCs (the sh7750 realize discussion).
could
>
> Andreas
>

-- 
-----
  Igor