qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Eric Blake <eblake@redhat.com>
To: Paolo Bonzini <pbonzini@redhat.com>, qemu-devel@nongnu.org
Cc: imammedo@redhat.com, akong@redhat.com, armbru@redhat.com,
	lcapitulino@redhat.com
Subject: Re: [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command
Date: Tue, 10 Dec 2013 11:00:15 -0700	[thread overview]
Message-ID: <52A756AF.7000309@redhat.com> (raw)
In-Reply-To: <1386694828-19786-5-git-send-email-pbonzini@redhat.com>

[-- Attachment #1: Type: text/plain, Size: 2012 bytes --]

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 <pbonzini@redhat.com>
> ---

> +++ 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

  reply	other threads:[~2013-12-10 18:00 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-12-10 17:00 [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del Paolo Bonzini
2013-12-10 17:00 ` [Qemu-devel] [PATCH 1/5] rng: initialize file descriptor to -1 Paolo Bonzini
2013-12-11 23:14   ` Eric Blake
2013-12-10 17:00 ` [Qemu-devel] [PATCH 2/5] qom: fix leak for objects created with -object Paolo Bonzini
2013-12-11 23:15   ` Eric Blake
2013-12-10 17:00 ` [Qemu-devel] [PATCH 3/5] qom: catch errors in object_property_add_child Paolo Bonzini
2013-12-11 23:16   ` Eric Blake
2013-12-10 17:00 ` [Qemu-devel] [PATCH 4/5] monitor: add object-add (QMP) and object_add (HMP) command Paolo Bonzini
2013-12-10 18:00   ` Eric Blake [this message]
2013-12-10 18:15     ` Paolo Bonzini
2013-12-13  2:55       ` Wenchao Xia
2013-12-13 12:19         ` Paolo Bonzini
2013-12-16 20:02       ` Luiz Capitulino
2013-12-17  7:20         ` Markus Armbruster
2013-12-19 21:53           ` Michael Roth
2014-01-07 12:00             ` Markus Armbruster
2014-01-07 12:51               ` Paolo Bonzini
2013-12-10 17:00 ` [Qemu-devel] [PATCH 5/5] monitor: add object-del (QMP) and object_del " Paolo Bonzini
2013-12-10 18:01   ` Eric Blake
2013-12-10 18:17     ` Paolo Bonzini
2013-12-12 15:43 ` [Qemu-devel] [PATCH 0/5] Monitor commands for object-add/del Igor Mammedov
2013-12-16 20:03 ` Luiz Capitulino

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=52A756AF.7000309@redhat.com \
    --to=eblake@redhat.com \
    --cc=akong@redhat.com \
    --cc=armbru@redhat.com \
    --cc=imammedo@redhat.com \
    --cc=lcapitulino@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).