From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37533) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDXXB-0002EL-GG for qemu-devel@nongnu.org; Thu, 16 Jun 2016 09:40:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bDXX7-0001yS-4n for qemu-devel@nongnu.org; Thu, 16 Jun 2016 09:40:36 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49415) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bDXX6-0001yG-Ji for qemu-devel@nongnu.org; Thu, 16 Jun 2016 09:40:32 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BE4F7693F8 for ; Thu, 16 Jun 2016 13:40:31 +0000 (UTC) From: Markus Armbruster References: <1463784024-17242-1-git-send-email-eblake@redhat.com> <1463784024-17242-14-git-send-email-eblake@redhat.com> Date: Thu, 16 Jun 2016 15:40:29 +0200 In-Reply-To: <1463784024-17242-14-git-send-email-eblake@redhat.com> (Eric Blake's message of "Fri, 20 May 2016 16:40:22 -0600") Message-ID: <87h9ct2qwi.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v7 13/15] net: Complete qapi-fication of netdev_add List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Jason Wang Eric Blake writes: > We finally have all the required pieces for doing a type-safe > representation of netdev_add as a flat union, where the > discriminator 'type' now selects which additional members may > appear in the "arguments" JSON object sent over QMP, while > making no changes to the set of previously-valid QMP commands > that would work, and without breaking command line parsing. Isn't it amazing that you pulled this off without a compatibility break? > > Signed-off-by: Eric Blake > > --- > v7: no change > v6: don't lose qemu_opts handling > --- > qapi-schema.json | 15 +++------------ > include/net/net.h | 1 - > net/net.c | 6 +++--- > qmp-commands.hx | 2 +- > 4 files changed, 7 insertions(+), 17 deletions(-) > > diff --git a/qapi-schema.json b/qapi-schema.json > index 4fee44b..fee4d07 100644 > --- a/qapi-schema.json > +++ b/qapi-schema.json > @@ -2301,26 +2301,17 @@ > # > # Add a network backend. > # > -# @type: the type of network backend. Current valid values are 'user', 'tap', > -# 'vde', 'socket', 'dump' and 'bridge' > +# @type: the type of network backend; determines which additional arguments > +# are valid. See Netdev documentation for more details. > # > # @id: the name of the new network backend > # > -# Additional arguments depend on the type. > -# > -# TODO This command effectively bypasses QAPI completely due to its > -# "additional arguments" business. It shouldn't have been added to > -# the schema in this form. It should be qapified properly, or > -# replaced by a properly qapified command. > -# > # Since: 0.14.0 > # > # Returns: Nothing on success > # If @type is not a valid network backend, DeviceNotFound > ## > -{ 'command': 'netdev_add', > - 'data': {'type': 'str', 'id': 'str'}, > - 'gen': false } # so we can get the additional arguments > +{ 'command': 'netdev_add', 'data': 'Netdev', 'box': true } > > ## > # @netdev_del: > diff --git a/include/net/net.h b/include/net/net.h > index 74b32e0..8f9be98 100644 > --- a/include/net/net.h > +++ b/include/net/net.h > @@ -193,7 +193,6 @@ void net_cleanup(void); > void hmp_host_net_add(Monitor *mon, const QDict *qdict); > void hmp_host_net_remove(Monitor *mon, const QDict *qdict); > void netdev_add(QemuOpts *opts, Error **errp); > -void qmp_netdev_add(QDict *qdict, QObject **ret, Error **errp); Only a dead QemuOpts is a good QemuOpts. > > int net_hub_id_for_client(NetClientState *nc, int *id); > NetClientState *net_hub_port_find(int hub_id); > diff --git a/net/net.c b/net/net.c > index e159506..b93ff00 100644 > --- a/net/net.c > +++ b/net/net.c > @@ -1188,7 +1188,7 @@ void netdev_add(QemuOpts *opts, Error **errp) > net_client_init(opts, true, errp); > } > > -void qmp_netdev_add(QDict *qdict, QObject **ret, Error **errp) > +void qmp_netdev_add(Netdev *netdev, Error **errp) > { > Error *local_err = NULL; > QemuOptsList *opts_list; > @@ -1199,12 +1199,12 @@ void qmp_netdev_add(QDict *qdict, QObject **ret, Error **errp) > goto out; > } > > - opts = qemu_opts_from_qdict(opts_list, qdict, &local_err); > + opts = qemu_opts_create(opts_list, netdev->id, 1, &local_err); We keep creating a QemuOpts for the network backend, but its opts->head remains empty. We need it for qmp_netdev_del(). Possibly more, but I doubt it. I think this is worth a mention in the commit message. > if (local_err) { > goto out; > } > > - netdev_add(opts, &local_err); > + net_client_init1(netdev, true, &local_err); > if (local_err) { > qemu_opts_del(opts); > goto out; Suggest to inline netdev_add() into its only remaining caller hmp_netdev_add(). > diff --git a/qmp-commands.hx b/qmp-commands.hx > index 94847e5..3804030 100644 > --- a/qmp-commands.hx > +++ b/qmp-commands.hx > @@ -949,7 +949,7 @@ EQMP > { > .name = "netdev_add", > .args_type = "netdev:O", > - .mhandler.cmd_new = qmp_netdev_add, > + .mhandler.cmd_new = qmp_marshal_netdev_add, > }, > > SQMP