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

[Igor Mammedov <imammedo@redhat.com>]

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