From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52646) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bL8rg-0002kJ-Rj for qemu-devel@nongnu.org; Thu, 07 Jul 2016 08:57:13 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bL8rc-0007ba-Ll for qemu-devel@nongnu.org; Thu, 07 Jul 2016 08:57:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43469) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bL8rc-0007au-Dg for qemu-devel@nongnu.org; Thu, 07 Jul 2016 08:57:08 -0400 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (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 8F4F180082 for ; Thu, 7 Jul 2016 12:57:07 +0000 (UTC) From: Markus Armbruster References: <1467514729-29366-1-git-send-email-eblake@redhat.com> <1467514729-29366-15-git-send-email-eblake@redhat.com> Date: Thu, 07 Jul 2016 14:57:05 +0200 In-Reply-To: <1467514729-29366-15-git-send-email-eblake@redhat.com> (Eric Blake's message of "Sat, 2 Jul 2016 20:58:47 -0600") Message-ID: <87r3b5obby.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH v8 14/16] 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 , Luiz Capitulino 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, and exposes > those types through introspection, and without breaking command > line parsing. > > Inline the function netdev_add() into its lone remaining caller. > > There are a few places where the QMP 'netdev_add' command is now > more strict: anywhere that the QAPI lists an integer member, we > now require a strict JSON integer (previously, we allowed both > integers and strings, because the conversion from QMP to QemuOpts > back to QObject collapsed them into integers). For example, > pre-patch, both of these examples succeed, but post-patch, the > second example fails: > > {'execute':'netdev_add', > 'arguments':{'id':'net1', 'type':'hubport', 'hubid':1}} > {"return": {}} > {'execute':'netdev_add', > 'arguments':{'id':'net2', 'type':'hubport', 'hubid':"2"}} > {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'hubid', expected: integer"}} Needs to be covered in release notes. > If that turns out to be problematic, we could resolve the problem > by either adding a loose parsing mode to qmp-input-visitor (teaching > it to accept a string encoding of an integer in place of an actual > integer), or by using QAPI alternates. But that work should be > in followup patches, if at all. The sloppy typing is an accidental misfeature that is at odds with QMP's design. As far as I can tell, it's not documented anywhere. But lots of things aren't documented anywhere; the questions is whether they're used. If we find we must keep netdev_add misfeature-compatible, I want it deprecated in favor of a new command that adheres to the QMP design. Do we have to keep it misfeature-compatible proactively? How comfortable are we with "wait see whether anything breaks" here? The scenarios I want to avoid are "OMG, hold the release while we improvise a misfeature-compatibility hack", and "OMG, we need a stable release for misfeature compatibility a.s.a.p." I'm okay with breaking misfeature compatibility in this patch, and unbreak it separately if necessary. > In qmp_netdev_add(), we still have to create a QemuOpts object > so that qmp_netdev_del() will be able to remove a hotplugged > network device; but the opts->head remains empty since we now > manage all parsing through the QAPI object rather than QemuOpts. This use of QemuOpts as a network backend directory has always been somewhat questionable. More so now the QemuOpts is empty. But it's not this patch's business. > Signed-off-by: Eric Blake