From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53785) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUclV-0007Uo-5j for qemu-devel@nongnu.org; Wed, 26 Aug 2015 11:37:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUclR-0001HU-SJ for qemu-devel@nongnu.org; Wed, 26 Aug 2015 11:37:29 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40323) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUclR-0001H4-Ke for qemu-devel@nongnu.org; Wed, 26 Aug 2015 11:37:25 -0400 References: <1440583182-5828-1-git-send-email-yanghy@cn.fujitsu.com> <1440583182-5828-4-git-send-email-yanghy@cn.fujitsu.com> <878u8ym649.fsf@blackfin.pond.sub.org> From: Eric Blake Message-ID: <55DDDD33.3000507@redhat.com> Date: Wed, 26 Aug 2015 09:37:23 -0600 MIME-Version: 1.0 In-Reply-To: <878u8ym649.fsf@blackfin.pond.sub.org> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="X5ORpquwQ6aH07E0T18MSg8fVDvOXHiKM" Subject: Re: [Qemu-devel] [PATCH v8 03/11] netfilter: add netfilter_{add|del} commands List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster , Yang Hongyang Cc: thuth@redhat.com, zhang.zhanghailiang@huawei.com, lizhijian@cn.fujitsu.com, jasowang@redhat.com, qemu-devel@nongnu.org, mrhines@linux.vnet.ibm.com, Luiz Capitulino , stefanha@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --X5ORpquwQ6aH07E0T18MSg8fVDvOXHiKM Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 08/26/2015 09:17 AM, Markus Armbruster wrote: > Only reviewing QAPI/QMP and HMP interface parts for now. >=20 > I apologize for not having reviewed this series earlier. v8 is awfully= > late for the kind of review comments I have. And I've also been behind the curve, intending to review this since it touches API, but not getting there yet. >> +## >> +{ 'command': 'netfilter_add', >> + 'data': { >> + 'type': 'str', >> + 'id': 'str', >> + 'netdev': 'str', >> + '*chain': 'str', >> + '*props': '**'}, 'gen': false } >=20 > I figure you're merely following netdev_add precedence here (can't faul= t > you for that), but netdev_add cheats, and we shouldn't add more cheats.= >=20 > First, 'gen': false is best avoided. It suppresses the generated > marshaller, and that lets you cheat. There are cases where we can't do= > without, but I don't think this is one. >=20 > When we suppress the generated marshaller, 'data' is at best a > declaration of intent; the code can do something else entirely. For > instance, netdev_add declares >=20 > { 'command': 'netdev_add', > 'data': {'type': 'str', 'id': 'str', '*props': '**'}, > 'gen': false } >=20 > but the '*props' part is a bald-faced lie: it doesn't take a 'props' > argument. See > http://lists.gnu.org/archive/html/qemu-devel/2015-08/msg00460.html > and maybe even slides 37-38 of > https://events.linuxfoundation.org/sites/events/files/slides/armbru-qem= u-introspection.pdf >=20 > I didn't check your code, but I suspect yours is a lie, too. >=20 > I intend to clean up netdev_add, hopefully soon. >=20 > You should use a proper QAPI data type here. I guess the flat union I > sketched in my reply to PATCH 2 would do nicely, except we don't suppor= t > commands with union type data, yet. I expect to add support to clean u= p > netdev_del. In fact, I've already proposed adding such support: http://thread.gmane.org/gmane.comp.emulators.qemu/356265/focus=3D356291 >=20 > If you don't want to wait for that (understandable!), then I suggest yo= u > keep 'NetFilter' a struct type for now, use it as command data here, an= d > we convert it to a flat union later. Must be done before the release, > to avoid backward incompatibility. >=20 > Then this becomes something like: >=20 > { 'command': 'netfilter-add', 'data': 'NetFilter' } or use NetFilter as a union, but have the command be: { 'command': 'netfilter-add', 'data': { 'data': 'NetFilter' } } where you have to pass an extra layer of nesting at the QMP layer. >=20 > If you need the command to take arguments you don't want to put into > NetFilter for some reason, I can help you achieve that cleanly. In fact, having the 'NetFilter' union be a single argument of a larger struct makes that larger struct the nice place to stick any additional arguments that don't need to be part of the union. >=20 >> + >> +## >> +# @netfilter_del: >> +# >> +# Remove a netfilter. >> +# >> +# @id: the name of the netfilter to remove >> +# >> +# Returns: Nothing on success >> +# If @id is not a valid netfilter, DeviceNotFound >> +# >> +# Since: 2.5 >> +## >> +{ 'command': 'netfilter_del', 'data': {'id': 'str'} } Please name new QMP commands with '-' not '_'; this should be 'netfilter-del'. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --X5ORpquwQ6aH07E0T18MSg8fVDvOXHiKM Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJV3d0zAAoJEKeha0olJ0Nq1cQH/jALYkAS8LOCkTocJO1waG9h G3ntfuXr1axaRapwE2/6oEloczU8px4I4GHjIm92Rcaq5iLfTYmqDYVxNrvt10ny e0d6EEwy0nB0KgvhUxEFwA6X0kO4f27601ZERL+5xy/Fn8e5ESMx04Lee26JlrhD zSUVhtJluGJt93U9jPRWyjHW7kwREcNL7DeAcTYxV/t2Cses3XB7HmXCIYi30d4J W3OX0av2uQT86PcBkzbOWsYbbc7Qj5eG7t1UTYnYfDaSdCMuTrp9FJ9R0KkneFnL c/fzt7kP5hhcg9z8M+7/ebSMI08I7RYC131kuPlTwSb9O3Tua/+ppGfNgHxdYmI= =Ovx6 -----END PGP SIGNATURE----- --X5ORpquwQ6aH07E0T18MSg8fVDvOXHiKM--