From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49152) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNfBD-0001qE-LC for qemu-devel@nongnu.org; Mon, 25 Jan 2016 06:19:33 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aNfB8-0004t4-OU for qemu-devel@nongnu.org; Mon, 25 Jan 2016 06:19:31 -0500 Received: from szxga02-in.huawei.com ([119.145.14.65]:19034) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNfB7-0004sz-LW for qemu-devel@nongnu.org; Mon, 25 Jan 2016 06:19:26 -0500 References: <1453451811-11860-1-git-send-email-zhang.zhanghailiang@huawei.com> <1453451811-11860-7-git-send-email-zhang.zhanghailiang@huawei.com> <56A5B031.3020707@redhat.com> <56A5CD1D.9090303@huawei.com> From: Hailiang Zhang Message-ID: <56A604A7.4070202@huawei.com> Date: Mon, 25 Jan 2016 19:19:03 +0800 MIME-Version: 1.0 In-Reply-To: <56A5CD1D.9090303@huawei.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each 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/1/25 15:22, Hailiang Zhang wrote: > On 2016/1/25 13:18, Jason Wang wrote: >> >> >> On 01/22/2016 04:36 PM, zhanghailiang wrote: >>> We add each netdev a default buffer filter, which the name is >>> 'nop', and the default buffer filter is disabled, so it has >>> no side effect for packets delivering in qemu net layer. >>> >>> The default buffer filter can be used by COLO or Micro-checkpoint, >>> The reason we add the default filter is we hope to support >>> hot add network during COLO state in future. >>> >>> Signed-off-by: zhanghailiang >>> --- >>> include/net/filter.h | 11 +++++++++++ >>> net/dump.c | 2 -- >>> net/filter.c | 15 ++++++++++++++- >>> net/net.c | 18 ++++++++++++++++++ >>> 4 files changed, 43 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/net/filter.h b/include/net/filter.h >>> index c7bd8f9..2043609 100644 >>> --- a/include/net/filter.h >>> +++ b/include/net/filter.h >>> @@ -22,6 +22,16 @@ >>> #define NETFILTER_CLASS(klass) \ >>> OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER) >>> >>> +#define DEFAULT_FILTER_NAME "nop" >> >> Maybe DEFAULT_FILTER_TYPE? >> > >>> + >>> +#define TYPE_FILTER_BUFFER "filter-buffer" >>> +#define TYPE_FILTER_DUMP "filter-dump" >>> + >>> +#define NETFILTER_ID_BUFFER 1 >>> +#define NETFILTER_ID_DUMP 2 >>> + >>> +extern const char *const netfilter_type_lookup[]; >>> + >>> typedef void (FilterSetup) (NetFilterState *nf, Error **errp); >>> typedef void (FilterCleanup) (NetFilterState *nf); >>> /* >>> @@ -55,6 +65,7 @@ struct NetFilterState { >>> char *netdev_id; >>> NetClientState *netdev; >>> NetFilterDirection direction; >>> + bool is_default; >>> bool enabled; >>> QTAILQ_ENTRY(NetFilterState) next; >>> }; >>> diff --git a/net/dump.c b/net/dump.c >>> index 88d9582..82727a6 100644 >>> --- a/net/dump.c >>> +++ b/net/dump.c >>> @@ -229,8 +229,6 @@ int net_init_dump(const NetClientOptions *opts, const char *name, >>> >>> /* Dumping via filter */ >>> >>> -#define TYPE_FILTER_DUMP "filter-dump" >>> - >>> #define FILTER_DUMP(obj) \ >>> OBJECT_CHECK(NetFilterDumpState, (obj), TYPE_FILTER_DUMP) >>> >>> diff --git a/net/filter.c b/net/filter.c >>> index 4d96301..a126a3b 100644 >>> --- a/net/filter.c >>> +++ b/net/filter.c >>> @@ -21,6 +21,11 @@ >>> #include "qapi/qmp-input-visitor.h" >>> #include "monitor/monitor.h" >>> >>> +const char *const netfilter_type_lookup[] = { >>> + [NETFILTER_ID_BUFFER] = TYPE_FILTER_BUFFER, >>> + [NETFILTER_ID_DUMP] = TYPE_FILTER_DUMP, >>> +}; >>> + >>> ssize_t qemu_netfilter_receive(NetFilterState *nf, >>> NetFilterDirection direction, >>> NetClientState *sender, >>> @@ -200,7 +205,7 @@ static void netfilter_complete(UserCreatable *uc, Error **errp) >>> NetFilterClass *nfc = NETFILTER_GET_CLASS(uc); >>> int queues; >>> Error *local_err = NULL; >>> - >>> + char *path = object_get_canonical_path_component(OBJECT(nf)); >>> >>> if (!nf->netdev_id) { >>> error_setg(errp, "Parameter 'netdev' is required"); >>> @@ -225,6 +230,14 @@ static void netfilter_complete(UserCreatable *uc, Error **errp) >>> } >>> >>> nf->netdev = ncs[0]; >>> + nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME); >>> + /* >>> + * For the default buffer filter, it will be disabled by default, >>> + * So it will not buffer any packets. >>> + */ >>> + if (nf->is_default) { >>> + nf->enabled = false; >>> + } >> >> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may >> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status" >> into properties. >> > > A little confused, do you mean add a 'default' property for filter ? > Just like the new 'status' property which is exported to users ? > Is the type of 'default' property string or bool ? > I don't think it is necessary to add a 'default' property, because we don't want such a property to be controlled by user. It is only used internally. We have another choice to deal with this, Add a 'bool is_default' parameter for netdev_add_filter(), and handle the default filter in it, just like: void netdev_add_filter(const char *netdev_id, const char *filter_type, const char *id, bool is_default, Error **errp) { ... ... object_add(filter_type, id, qdict, iv, &err); .... ... if (is_default) { 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 = true; nf->enabled = false; } } Is this acceptable ? Thanks, Hailiang >>> >>> if (nfc->setup) { >>> nfc->setup(nf, &local_err); >>> diff --git a/net/net.c b/net/net.c >>> index ec43105..9630234 100644 >>> --- a/net/net.c >>> +++ b/net/net.c >>> @@ -76,6 +76,12 @@ const char *host_net_devices[] = { >>> >>> int default_net = 1; >>> >>> +/* >>> + * FIXME: Export this with an option for users to control >>> + * this with comand line ? >> >> This could be done in the future. >> > > Should i change the tag to 'TODO' ? > >>> + */ >>> +int default_netfilter = NETFILTER_ID_BUFFER; >> >> Why not just use a string here? >> > > No special, i will convert it to use string here. > >>> + >>> /***********************************************************/ >>> /* network device redirectors */ >>> >>> @@ -1032,6 +1038,18 @@ static int net_client_init1(const void *object, int is_netdev, Error **errp) >>> } >>> return -1; >>> } >>> + >>> + if (is_netdev) { >>> + const Netdev *netdev = object; >>> + /* >>> + * Here we add each netdev a default filter whose name is 'nop', >>> + * it will disabled by default, Users can enable it when necessary. >>> + */ >> >> If we support default properties, the above comment could be removed. >> >>> + netdev_add_filter(netdev->id, >>> + netfilter_type_lookup[default_netfilter], >>> + DEFAULT_FILTER_NAME, >> >> I believe some logic to generate id automatically is needed here. >> > > Yes, you are right, here this patch will break QEMU with multi-NICs configured, > the error report is " attempt to add duplicate property 'nop' to object". > I will fix it in next version. > > Thanks, > Hailiang > >>> + errp); >>> + } >>> return 0; >>> } >>> >> >> >> . >> >