From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58873) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQDUl-0005ih-Ak for qemu-devel@nongnu.org; Mon, 01 Feb 2016 07:22:16 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQDUg-0007Hr-Gx for qemu-devel@nongnu.org; Mon, 01 Feb 2016 07:22:15 -0500 Received: from szxga01-in.huawei.com ([58.251.152.64]:24816) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQDUf-0007H3-GY for qemu-devel@nongnu.org; Mon, 01 Feb 2016 07:22:10 -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> From: Hailiang Zhang Message-ID: <56AF4DCB.2060008@huawei.com> Date: Mon, 1 Feb 2016 20:21:31 +0800 MIME-Version: 1.0 In-Reply-To: <56AF2890.4090304@redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed 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: Jason Wang , qemu-devel@nongnu.org Cc: peter.huangpeng@huawei.com, zhangchen.fnst@cn.fujitsu.com, hongyang.yang@easystack.cn Hi Jason, 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. > Danies recommended using object_new_with_props() instead of object_create(), which will avoids the need of format + parse process. It makes the codes more clean. I have fixed it in v3. I kept the 'is_default' member for struct NetFilterState. Because comparing with finding the default filter by the special name ([netdev->id][nop]), it seems to be more easy for COLO to find/control the default filter with this flag. (We added a new helper qemu_foreach_netfilter() in COLO frame, and we traverse these filters directly without passing any netdev related info.) But if you think it is still not acceptable to add such a member, i'll remove it in next verison ;) 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. > > > . >