From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47246) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQ9MH-0001j5-Sr for qemu-devel@nongnu.org; Mon, 01 Feb 2016 02:57:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQ9ME-0004vg-KX for qemu-devel@nongnu.org; Mon, 01 Feb 2016 02:57:13 -0500 Received: from szxga03-in.huawei.com ([119.145.14.66]:61890) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQ9MD-0004t8-NU for qemu-devel@nongnu.org; Mon, 01 Feb 2016 02:57: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> From: Hailiang Zhang Message-ID: <56AF0FC4.7070809@huawei.com> Date: Mon, 1 Feb 2016 15:56:52 +0800 MIME-Version: 1.0 In-Reply-To: <56AF0D63.3010707@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 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(+) >>>> >>>> diff --git a/include/net/filter.h b/include/net/filter.h >>>> index af3c53c..ee1c024 100644 >>>> --- a/include/net/filter.h >>>> +++ b/include/net/filter.h >>>> @@ -55,6 +55,7 @@ struct NetFilterState { >>>> char *netdev_id; >>>> NetClientState *netdev; >>>> NetFilterDirection direction; >>>> + bool is_default; >>>> bool enabled; >>>> QTAILQ_ENTRY(NetFilterState) next; >>>> }; >>>> @@ -74,4 +75,10 @@ ssize_t >>>> qemu_netfilter_pass_to_next(NetClientState *sender, >>>> int iovcnt, >>>> void *opaque); >>>> >>>> +void netdev_add_filter(const char *netdev_id, >>>> + const char *filter_type, >>>> + const char *id, >>>> + bool is_default, >>>> + Error **errp); >>>> + >>>> #endif /* QEMU_NET_FILTER_H */ >>>> diff --git a/net/filter.c b/net/filter.c >>>> index d08a2be..dc7aa9b 100644 >>>> --- a/net/filter.c >>>> +++ b/net/filter.c >>>> @@ -214,6 +214,86 @@ static void netfilter_complete(UserCreatable >>>> *uc, Error **errp) >>>> QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next); >>>> } >>>> >>>> +QemuOptsList qemu_filter_opts = { >>>> + .name = "default-filter", >>>> + .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head), >>>> + .desc = { >>>> + { >>>> + .name = "qom-type", >>>> + .type = QEMU_OPT_STRING, >>>> + },{ >>>> + .name = "id", >>>> + .type = QEMU_OPT_STRING, >>>> + },{ >>>> + .name = "netdev", >>>> + .type = QEMU_OPT_STRING, >>>> + },{ >>>> + .name = "status", >>>> + .type = QEMU_OPT_STRING, >>>> + }, >>>> + { /* end of list */ } >>>> + }, >>>> +}; >>>> + >>>> +static void filter_set_default_flag(const char *id, >>>> + bool is_default, >>>> + Error **errp) >>>> +{ >>>> + Object *obj, *container; >>>> + NetFilterState *nf; >>>> + >>>> + container = object_get_objects_root(); >>>> + obj = object_resolve_path_component(container, id); >>>> + if (!obj) { >>>> + error_setg(errp, "object id not found"); >>>> + return; >>>> + } >>>> + nf = NETFILTER(obj); >>>> + nf->is_default = is_default; >>>> +} >>>> + >>>> +void netdev_add_filter(const char *netdev_id, >>>> + const char *filter_type, >>>> + const char *id, >>>> + bool is_default, >>>> + Error **errp) >>>> +{ >>>> + NetClientState *nc = qemu_find_netdev(netdev_id); >>>> + char *optarg; >>>> + QemuOpts *opts = NULL; >>>> + Error *err = NULL; >>>> + >>>> + /* FIXME: Not support multiple queues */ >>>> + if (!nc || nc->queue_index > 1) { >>>> + return; >>>> + } >>>> + /* Not support vhost-net */ >>>> + if (get_vhost_net(nc)) { >>>> + return; >>>> + } >>>> + >>>> + 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, Just use this global string instead ? >> >>> - 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). For COLO, we don't want to control (use) other filters that added by users. Thanks, Hailiang > Thanks > >> >> Thanks, >> Hailiang >> >>> Thoughts? >>> >>>> + opts = qemu_opts_parse_noisily(&qemu_filter_opts, >>>> + optarg, false); >>>> + if (!opts) { >>>> + error_report("Failed to parse param '%s'", optarg); >>>> + exit(1); >>>> + } >>>> + g_free(optarg); >>>> + if (object_create(NULL, opts, &err) < 0) { >>>> + error_report("Failed to create object"); >>>> + goto out_clean; >>>> + } >>>> + filter_set_default_flag(id, is_default, &err); >>>> + >>>> +out_clean: >>>> + qemu_opts_del(opts); >>>> + if (err) { >>>> + error_propagate(errp, err); >>>> + } >>>> +} >>>> + >>>> static void netfilter_finalize(Object *obj) >>>> { >>>> NetFilterState *nf = NETFILTER(obj); >>> >>> >>> . >>> >> >> > > > . >