From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37203) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aRZjw-0006cW-34 for qemu-devel@nongnu.org; Fri, 05 Feb 2016 01:19:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aRZjo-0008QT-M4 for qemu-devel@nongnu.org; Fri, 05 Feb 2016 01:19:27 -0500 Received: from mx1.redhat.com ([209.132.183.28]:59847) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aRZjo-0008QO-EM for qemu-devel@nongnu.org; Fri, 05 Feb 2016 01:19:24 -0500 References: <1453883380-10532-1-git-send-email-zhang.zhanghailiang@huawei.com> <1453883380-10532-4-git-send-email-zhang.zhanghailiang@huawei.com> <56AECDA8.6000402@redhat.com> <56AEF788.3080706@huawei.com> <56AF0D63.3010707@redhat.com> <56AF0FC4.7070809@huawei.com> <56AF1F8E.2020604@redhat.com> <56AF23BB.8000203@huawei.com> <56AF2890.4090304@redhat.com> <56AF362E.9000302@huawei.com> From: Jason Wang Message-ID: <56B43EE0.7080405@redhat.com> Date: Fri, 5 Feb 2016 14:19:12 +0800 MIME-Version: 1.0 In-Reply-To: <56AF362E.9000302@huawei.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hailiang Zhang , qemu-devel@nongnu.org Cc: peter.huangpeng@huawei.com, zhangchen.fnst@cn.fujitsu.com, hongyang.yang@easystack.cn On 02/01/2016 06:40 PM, Hailiang Zhang wrote: > On 2016/2/1 17:42, Jason Wang wrote: >> >> >> On 02/01/2016 05:22 PM, Hailiang Zhang wrote: >>> On 2016/2/1 17:04, Jason Wang wrote: >>>> >>>> >>>> On 02/01/2016 03:56 PM, Hailiang Zhang wrote: >>>>> On 2016/2/1 15:46, Jason Wang wrote: >>>>>> >>>>>> >>>>>> On 02/01/2016 02:13 PM, Hailiang Zhang wrote: >>>>>>> On 2016/2/1 11:14, Jason Wang wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 01/27/2016 04:29 PM, zhanghailiang wrote: >>>>>>>>> We add a new helper function netdev_add_filter(), this function >>>>>>>>> can help adding a filter object to a netdev. >>>>>>>>> Besides, we add a is_default member for struct NetFilterState >>>>>>>>> to indicate whether the filter is default or not. >>>>>>>>> >>>>>>>>> Signed-off-by: zhanghailiang >>>>>>>>> --- >>>>>>>>> v2: >>>>>>>>> -Re-implement netdev_add_filter() by re-using >>>>>>>>> object_create() >>>>>>>>> (Jason's suggestion) >>>>>>>>> --- >>>>>>>>> include/net/filter.h | 7 +++++ >>>>>>>>> net/filter.c | 80 >>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>>> 2 files changed, 87 insertions(+) >>>>>>>>> >>>>>>>>> >>>> >>>> [...] >>>> >>>>>>>>> + >>>>>>>>> + optarg = >>>>>>>>> g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s", >>>>>>>>> + filter_type, id, netdev_id, is_default ? "disable" : >>>>>>>>> "enable" >>>>>>>> >>>>>>>> Instead of this, I wonder maybe it's better to: >>>>>>>> >>>>>>>> - store the default filter property into a pointer to string >>>>>>> >>>>>>> Do you mean, pass a string parameter which stores the filter >>>>>>> property >>>>>>> instead of >>>>>>> assemble it in this helper ? >>>>>> >>>>>> Yes. E.g just a global string which could be changed by any >>>>>> subsystem. >>>>>> E.g colo may change it to >>>>>> "filter-buffer,interval=0,status=disable". But >>>>>> filter ids need to be generated automatically. >>>>>> >>>>> >>>>> Got it. Then we don't need the global default_netfilter_type[] in >>>>> patch 5, >>>> >>>> Yes. >>>> >>>>> Just use this global string instead ? >>>> >>>> Right. >>>> >>>>> >>>>>>> >>>>>>>> - colo code may change the pointer to >>>>>>>> "filter-buffer,status=disable" >>>>>>>> >>>>>>> >>>>>>>> Then, there's no need for lots of codes above: >>>>>>>> - no need a "is_default" parameter in netdev_add_filter which >>>>>>>> does not >>>>>>>> scale consider we may want to have more property in the future >>>>>>>> - no need to hacking like "qemu_filter_opts" >>>>>>> >>>>>>> Yes, we can use qemu_find_opts("object") instead of it. >>>>>>> >>>>>>>> - no need to have a special flag like "is_default" >>>>>>>> >>>>>>> >>>>>>> But we have to distinguish the default filter from the common >>>>>>> filter, use the name (id) to distinguish it ? >>>>>> >>>>>> What's the reason that you want to distinguish default filters from >>>>>> others? >>>>>> >>>>> >>>>> The default filters will be used by COLO or MC, (In COLO, we will >>>>> use it >>>>> to control packets buffering/releasing). >>>> >>>> A question is how will you do this? >>>> >>> >>> Er, for COLO, we will enable all the default filter in the >>> initialization stage, >>> then the buffer-filter will buffer all netdev's packets, >>> after doing a checkpoint, we will release all the buffered packets >>> (Flush all default >>> filters' packets). >> >> Right, that's the point. So looks several choices here: >> >> 1) Track each default filter explicitly, generate and record the netdev >> ids for default filter by COLO. Walk through the ids list and release >> the packet in each filter. >> 2) Track the default filters implicitly. Link all buffer filters (with >> zero interval) in a list during filter buffer initialization. And export >> a helper for COLO to walk them and release packets. >> >> Either looks fine, but maybe 2 is easier. >> > > 2) is a good choice, but I'm a little worry about using zero to > distinguish if a filter is default filter or not, because users > can use qom-set to change its value. Then I see minor issues: 1) Looks like we should prevent user other than COLO from using zero interval, neither from cli nor 'qom-set'. 2) For the zero interval filter used by COLO, we should not allow user to change the value of interval. So I was thinking a new type which is based on current filter-buffer whose interval is invisible to user. > If special flag like 'is_default' is not acceptable, > then we have to use the filter ids to distinguish the default > buffer-filter > (here the rule of generation ids for default filter is > '[netdev-id][DEFAULT_FILTER_TYPE]' > > Thanks, > Hailiang > >>> If VM is failover, we will set all default filters back to disabled >>> status. >>> (This is a periodic mode for COLO, different from another mode, which >>> we will call it >>> hybrid mode, that is based on colo-proxy, which is in developing by >>> zhangchen) >>> >>> Thanks, >>> Hailiang >> >> Yes, I see. >> >> >> . >> > >