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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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;