From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36739) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWE1f-0001QV-Bv for qemu-devel@nongnu.org; Sun, 30 Aug 2015 21:36:48 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWE1b-0007We-2Y for qemu-devel@nongnu.org; Sun, 30 Aug 2015 21:36:47 -0400 Received: from [59.151.112.132] (port=58191 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWE1Z-0007TJ-Lo for qemu-devel@nongnu.org; Sun, 30 Aug 2015 21:36:42 -0400 Message-ID: <55E3AFA2.1040702@cn.fujitsu.com> Date: Mon, 31 Aug 2015 09:36:34 +0800 From: Yang Hongyang MIME-Version: 1.0 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> <55DDDD33.3000507@redhat.com> <87k2sfabku.fsf@blackfin.pond.sub.org> In-Reply-To: <87k2sfabku.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit 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 , Eric Blake 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 On 08/28/2015 07:37 PM, Markus Armbruster wrote: > Eric Blake writes: > >> On 08/26/2015 09:17 AM, Markus Armbruster wrote: >>> Only reviewing QAPI/QMP and HMP interface parts for now. >>> >>> 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 } >>> >>> I figure you're merely following netdev_add precedence here (can't fault >>> you for that), but netdev_add cheats, and we shouldn't add more cheats. >>> >>> 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. >>> >>> 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 >>> >>> { 'command': 'netdev_add', >>> 'data': {'type': 'str', 'id': 'str', '*props': '**'}, >>> 'gen': false } >>> >>> 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-qemu-introspection.pdf >>> >>> I didn't check your code, but I suspect yours is a lie, too. >>> >>> I intend to clean up netdev_add, hopefully soon. >>> >>> 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 support >>> commands with union type data, yet. I expect to add support to clean up >>> netdev_del. >> >> In fact, I've already proposed adding such support: >> >> http://thread.gmane.org/gmane.comp.emulators.qemu/356265/focus=356291 > > In my review queue. Which has become sickeningly long again... > >>> >>> If you don't want to wait for that (understandable!), then I suggest you >>> keep 'NetFilter' a struct type for now, use it as command data here, and >>> we convert it to a flat union later. Must be done before the release, >>> to avoid backward incompatibility. >>> >>> Then this becomes something like: >>> >>> { '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. >> >>> >>> 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. > > To make progress, I suggest you try the following: > > 1. Make NetFilter a flat union, as I suggested in my review of PATCH 2. > > 2. Use Eric's idea above, because it avoids the dependency on code > that's still under review. > > Drawback: extra layer of nesting. Ugly, but not the end of the world, > and we still have a chance to peel it off before the next release. Thanks for the explanation, I will try to see if I can fully understand your point. > > [...] > . > -- Thanks, Yang.