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

[Markus Armbruster]

Anthony Liguori <address@hidden> 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<address@hidden>
>> ---
>>   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);
>> +}