From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51750) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQ9kP-0007ix-Ea for qemu-devel@nongnu.org; Mon, 01 Feb 2016 03:22:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aQ9kM-00014h-64 for qemu-devel@nongnu.org; Mon, 01 Feb 2016 03:22:09 -0500 Received: from szxga02-in.huawei.com ([119.145.14.65]:5457) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aQ9kL-000147-Ai for qemu-devel@nongnu.org; Mon, 01 Feb 2016 03:22:06 -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> <56AF11DA.1060208@easystack.cn> From: Hailiang Zhang Message-ID: <56AF158F.7090208@huawei.com> Date: Mon, 1 Feb 2016 16:21:35 +0800 MIME-Version: 1.0 In-Reply-To: <56AF11DA.1060208@easystack.cn> 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: Yang Hongyang , Jason Wang , qemu-devel@nongnu.org Cc: peter.huangpeng@huawei.com, zhangchen.fnst@cn.fujitsu.com On 2016/2/1 16:05, Yang Hongyang 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(+) >>>>>> >>>>>> 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 Er, then we came back to the original question, 'is it necessary to add each netdev a default filter ?' If we add the a filter to netdev when doing COLO, it will be added dynamically, Here we want to add each netdev a default filter while launch QEMU (no matter if this VM will go into COLO or not), just to support hot-add NIC for VM while in COLO lifetime. > 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); >>>>> >>>>> >>>>> . >>>>> >>>> >>>> >>> >>> >>> . >>> >> >> >> >