On 01/25/2016 03:22 PM, 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)
>>>
[...]
>>>
>>> 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 ?
For example, is it possible to store the default property into a string
and just create the filter through qemu_opts_parse_noisily() by just
pass a string as its parameter? (Of course, it needs some code to
generate ids). With this, there's even no need for you to duplicate
codes like what patch 4 does.
>
>>>
>>> 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' ?
Ok.
>
>>> + */
>>> +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;
>>> }
>>>
>>
>>
>> .
>>
>
>
