Re: [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command

[Eric Blake]

On 12/10/2013 10:00 AM, Paolo Bonzini wrote:
> Add two commands that are the monitor counterparts of -object.  The commands
> have the same Visitor-based implementation, but use different kinds of
> visitors so that the HMP command has a DWIM string-based syntax, while
> the QMP variant accepts a stricter JSON-based properties dictionary.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---

> +++ b/qapi-schema.json
> @@ -2759,6 +2759,26 @@
>  { 'command': 'netdev_del', 'data': {'id': 'str'} }
>  
>  ##
> +# @object_add:

I'd prefer object-add for the QMP name (particularly since that's what
you called it in your commit message).

> +#
> +# Create a QOM object.
> +#
> +# @qom-type: the class name for the object to be created
> +#
> +# @id: the name of the new object
> +#
> +# @props: #optional a list of properties to be passed to the backend

s/list/dictionary/

> +#
> +# Returns: Nothing on success
> +#          Error if @qom-type is not a valid class name
> +#
> +# Since: 2.0
> +##
> +{ 'command': 'object_add',

Again, object-add

> +  'data': {'qom-type': 'str', 'id': 'str', '*props': 'dict'},
> +  'gen': 'no' }

This feels VERY open-coded.  No where else in qapi-schema do we have
'dict' as a type; using it violates all sorts of type-safety (which, I
guess, is the point), making it impossible to introspect what keys are
valid for use in the "props":{...} dictionary.  Do we really want to
play this fast and loose with the type system, or should we try harder
to make this a robust self-describing union of types?

That is, why can't we have object-add use a discriminated union, where
qom-type is the discriminator, and where props is an appropriate JSON
struct type that corresponds to the branch of the union, so that we get
full introspection on the set of valid keys to put in props for any
given qom-type?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature