From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35656) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWDwY-0007vE-MW for qemu-devel@nongnu.org; Sun, 30 Aug 2015 21:31:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZWDwV-0004N5-Fa for qemu-devel@nongnu.org; Sun, 30 Aug 2015 21:31:30 -0400 Received: from [59.151.112.132] (port=24735 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZWDwU-0004LZ-Mm for qemu-devel@nongnu.org; Sun, 30 Aug 2015 21:31:27 -0400 Message-ID: <55E3AE69.6060004@cn.fujitsu.com> Date: Mon, 31 Aug 2015 09:31:21 +0800 From: Yang Hongyang MIME-Version: 1.0 References: <1440583182-5828-1-git-send-email-yanghy@cn.fujitsu.com> <1440583182-5828-2-git-send-email-yanghy@cn.fujitsu.com> <874mjmno2w.fsf@blackfin.pond.sub.org> <55DE7729.7050106@cn.fujitsu.com> <87pp27abxa.fsf@blackfin.pond.sub.org> In-Reply-To: <87pp27abxa.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v8 01/11] net: add a new object netfilter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: thuth@redhat.com, zhang.zhanghailiang@huawei.com, lizhijian@cn.fujitsu.com, jasowang@redhat.com, qemu-devel@nongnu.org, mrhines@linux.vnet.ibm.com, stefanha@redhat.com, Paolo Bonzini On 08/28/2015 07:29 PM, Markus Armbruster wrote: > Yang Hongyang writes: > >> On 08/26/2015 10:04 PM, Markus Armbruster wrote: >>> Missed a bunch of revisions of this series, please excuse gaps in my >>> understanding. >> >> Thank you for the review. >> >>> >>> Yang Hongyang writes: >>> >>>> Add the framework for a new netfilter object and a new >>>> -netfilter CLI option as a basis for the following patches. >>>> >>>> Signed-off-by: Yang Hongyang >>>> CC: Paolo Bonzini >>>> CC: Eric Blake >>>> Reviewed-by: Thomas Huth >>>> --- >>>> include/net/filter.h | 15 +++++++++++++++ >>>> include/sysemu/sysemu.h | 1 + >>>> net/Makefile.objs | 1 + >>>> net/filter.c | 27 +++++++++++++++++++++++++++ >>>> qemu-options.hx | 1 + >>>> vl.c | 13 +++++++++++++ >>>> 6 files changed, 58 insertions(+) >>>> create mode 100644 include/net/filter.h >>>> create mode 100644 net/filter.c >>>> >>>> diff --git a/include/net/filter.h b/include/net/filter.h >>>> new file mode 100644 >>>> index 0000000..4242ded >>>> --- /dev/null >>>> +++ b/include/net/filter.h >>>> @@ -0,0 +1,15 @@ >>>> +/* >>>> + * Copyright (c) 2015 FUJITSU LIMITED >>>> + * >>>> + * This work is licensed under the terms of the GNU GPL, version 2 or >>>> + * later. See the COPYING file in the top-level directory. >>>> + */ >>>> + >>>> +#ifndef QEMU_NET_FILTER_H >>>> +#define QEMU_NET_FILTER_H >>>> + >>>> +#include "qemu-common.h" >>>> + >>>> +int net_init_filters(void); >>>> + >>>> +#endif /* QEMU_NET_FILTER_H */ >>>> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h >>>> index 44570d1..15d6d00 100644 >>>> --- a/include/sysemu/sysemu.h >>>> +++ b/include/sysemu/sysemu.h >>>> @@ -212,6 +212,7 @@ extern QemuOptsList qemu_chardev_opts; >>>> extern QemuOptsList qemu_device_opts; >>>> extern QemuOptsList qemu_netdev_opts; >>>> extern QemuOptsList qemu_net_opts; >>>> +extern QemuOptsList qemu_netfilter_opts; >>>> extern QemuOptsList qemu_global_opts; >>>> extern QemuOptsList qemu_mon_opts; >>>> >>>> diff --git a/net/Makefile.objs b/net/Makefile.objs >>>> index ec19cb3..914aec0 100644 >>>> --- a/net/Makefile.objs >>>> +++ b/net/Makefile.objs >>>> @@ -13,3 +13,4 @@ common-obj-$(CONFIG_HAIKU) += tap-haiku.o >>>> common-obj-$(CONFIG_SLIRP) += slirp.o >>>> common-obj-$(CONFIG_VDE) += vde.o >>>> common-obj-$(CONFIG_NETMAP) += netmap.o >>>> +common-obj-y += filter.o >>>> diff --git a/net/filter.c b/net/filter.c >>>> new file mode 100644 >>>> index 0000000..4e40f08 >>>> --- /dev/null >>>> +++ b/net/filter.c >>>> @@ -0,0 +1,27 @@ >>>> +/* >>>> + * Copyright (c) 2015 FUJITSU LIMITED >>>> + * >>>> + * This work is licensed under the terms of the GNU GPL, version 2 or >>>> + * later. See the COPYING file in the top-level directory. >>>> + */ >>>> + >>>> +#include "qemu-common.h" >>>> +#include "net/filter.h" >>>> + >>>> +int net_init_filters(void) >>>> +{ >>>> + return 0; >>>> +} >>>> + >>>> +QemuOptsList qemu_netfilter_opts = { >>>> + .name = "netfilter", >>>> + .implied_opt_name = "type", >>>> + .head = QTAILQ_HEAD_INITIALIZER(qemu_netfilter_opts.head), >>>> + .desc = { >>>> + /* >>>> + * no elements => accept any params >>>> + * validation will happen later >>>> + */ >>>> + { /* end of list */ } >>>> + }, >>>> +}; >>> >>> Ignorant question: why dynamic validation (empty .desc) instead of >>> statically defined parameters? The documentation you add in PATCH 10 >>> suggests they're not actually dynamic. >> >> Actually I was following the netdev stuff. There might be bunch of filters, >> and I don't know the params of each filter until it is realized. > > I realized that later on in your series. > >>>> diff --git a/qemu-options.hx b/qemu-options.hx >>>> index 77f5853..0d52d02 100644 >>>> --- a/qemu-options.hx >>>> +++ b/qemu-options.hx >>>> @@ -1575,6 +1575,7 @@ DEF("net", HAS_ARG, QEMU_OPTION_net, >>>> "socket][,vlan=n][,option][,option][,...]\n" >>>> " old way to initialize a host network interface\n" >>>> " (use the -netdev option if possible instead)\n", QEMU_ARCH_ALL) >>>> +DEF("netfilter", HAS_ARG, QEMU_OPTION_netfilter, "", QEMU_ARCH_ALL) >>>> STEXI >>>> @item -net nic[,vlan=@var{n}][,macaddr=@var{mac}][,model=@var{type}] [,name=@var{name}][,addr=@var{addr}][,vectors=@var{v}] >>>> @findex -net >>> >>> Wrong spot: this is between DEF(net, ...) and its STEXI..ETEXI stanza. >>> Suggest to move it behind the ETEXI. >>> >>> Missing help string. You add the help in PATCH 10. What about adding >>> it here already? Would serve as a hint of the things to come later in >>> your series. >>> >>> Missing STEXI..ETEXI stanza for the user manual. >> >> If I understand correctly, you are suggesting separate the netfilter from >> net, seems more reasonable, so I should add something like: >> DEF("netfilter", HAS_ARG, QEMU_OPTION_netfilter, "", QEMU_ARCH_ALL) >> STEXI..ETEXI >> >> after the DEF(net, ...) and its STEXI..ETEXI stanza, am I right? > > Yes. Always keep DEF(whatever, ...) and its STEXI..ETEXI together. > > Inserting netfilter after net seems the most logical place. Yes, thank you. > > [...] > . > -- Thanks, Yang.