From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51733) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNbTv-0007Hc-BF for qemu-devel@nongnu.org; Mon, 25 Jan 2016 02:22:36 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aNbTs-00083I-4j for qemu-devel@nongnu.org; Mon, 25 Jan 2016 02:22:35 -0500 Received: from szxga01-in.huawei.com ([58.251.152.64]:44303) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aNbTr-00081w-1w for qemu-devel@nongnu.org; Mon, 25 Jan 2016 02:22:32 -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> From: Hailiang Zhang Message-ID: <56A5CD1D.9090303@huawei.com> Date: Mon, 25 Jan 2016 15:22:05 +0800 MIME-Version: 1.0 In-Reply-To: <56A5B031.3020707@redhat.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 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 ? >> >> 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; >> } >> > > > . >