Re: [Qemu-devel] [PATCH RFC 6/7] net/filter: Add a default filter to each netdev

[Jason Wang <jasowang@redhat.com>]

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.zhanghailiang@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;
>>>   }
>>>
>>
>>
>> .
>>
>
>