From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49142) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQ9V0-0005MT-Fb for qemu-devel@nongnu.org; Mon, 01 Feb 2016 03:06:15 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQ9Ux-0006bu-8P for qemu-devel@nongnu.org; Mon, 01 Feb 2016 03:06:14 -0500 Received: from m199-177.yeah.net ([123.58.177.199]:33190) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQ9Uw-0006aN-GK for qemu-devel@nongnu.org; Mon, 01 Feb 2016 03:06:11 -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> From: Yang Hongyang Message-ID: <56AF11DA.1060208@easystack.cn> Date: Mon, 1 Feb 2016 16:05:46 +0800 MIME-Version: 1.0 In-Reply-To: <56AF0FC4.7070809@huawei.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: Hailiang Zhang , Jason Wang , qemu-devel@nongnu.org Cc: peter.huangpeng@huawei.com, zhangchen.fnst@cn.fujitsu.com 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(+) >>>>> >>>>> 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. I think Jason's point is that COLO is a manager, you can add the filter to netdev when doing COLO, so the only difference between COLO's default filter and other filter is that COLO's filter (for example buffer filter) is added by COLO, and the other filter is added by user specifing -filter. > > 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); >>>> >>>> >>>> . >>>> >>> >>> >> >> >> . >> > > > -- Thanks, Yang