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

[Anthony Liguori <anthony@codemonkey.ws>]

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);
> +}