From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60376) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOGnw-000105-0v for qemu-devel@nongnu.org; Tue, 26 Jan 2016 22:30:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aOGnr-0003tE-VO for qemu-devel@nongnu.org; Tue, 26 Jan 2016 22:29:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:35659) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aOGnr-0003tA-L2 for qemu-devel@nongnu.org; Tue, 26 Jan 2016 22:29:55 -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> <56A604A7.4070202@huawei.com> From: Jason Wang Message-ID: <56A839A7.2020505@redhat.com> Date: Wed, 27 Jan 2016 11:29:43 +0800 MIME-Version: 1.0 In-Reply-To: <56A604A7.4070202@huawei.com> Content-Type: text/plain; charset=utf-8 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: Hailiang Zhang , qemu-devel@nongnu.org Cc: peter.huangpeng@huawei.com, zhangchen.fnst@cn.fujitsu.com, hongyang.yang@easystack.cn On 01/25/2016 07:19 PM, Hailiang Zhang wrote: > 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. If it was only used internally, then just not export the ability to configure default filter and its properties to user. > 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: This does not scale consider if somebody want to add more common properties to netfilters. > > 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; >>>> } >>>> >>> >>> >>> . >>> >> > > >