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 <zhang.zhanghaili...@huawei.com>
>>>> ---
>>>> 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;
>>>> }
>>>>
>>>
>>>
>>> .
>>>
>>
>
>
>
