From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:39403) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXYPI-0000dN-3O for qemu-devel@nongnu.org; Thu, 24 May 2012 09:48:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SXYPF-00076O-NO for qemu-devel@nongnu.org; Thu, 24 May 2012 09:48:47 -0400 Received: from mx1.redhat.com ([209.132.183.28]:26595) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SXYPF-000767-FM for qemu-devel@nongnu.org; Thu, 24 May 2012 09:48:45 -0400 Message-ID: <4FBE3C38.8040401@redhat.com> Date: Thu, 24 May 2012 15:48:40 +0200 From: Igor Mammedov MIME-Version: 1.0 References: <1337859784-24097-1-git-send-email-armbru@redhat.com> <1337859784-24097-3-git-send-email-armbru@redhat.com> <4FBE31CD.1080101@codemonkey.ws> In-Reply-To: <4FBE31CD.1080101@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC 2/2] qmp: New command qom-new List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: pbonzini@redhat.com, Markus Armbruster , afaerber@suse.de, qemu-devel@nongnu.org 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"}, {"name": "i440fx", "type": "child"}, {"name": "unattached", >> "type": "child"}, {"name": "peripheral", "type": "child"}, {"name": "peripheral-anon", "type": "child"}]} >> >> 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 >> --- >> 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