Re: [Qemu-devel] [PATCH RFC v2 3/5] net/filter: Introduce a helper to add a filter to the netdev

[Hailiang Zhang <zhang.zhanghailiang@huawei.com>]

Hi Jason,

On 2016/2/1 17:42, Jason Wang wrote:
>
>
> On 02/01/2016 05:22 PM, Hailiang Zhang wrote:
>> On 2016/2/1 17:04, Jason Wang wrote:
>>>
>>>
>>> On 02/01/2016 03:56 PM, Hailiang Zhang wrote:
>>>> On 2016/2/1 15:46, Jason Wang wrote:
>>>>>
>>>>>
>>>>> On 02/01/2016 02:13 PM, Hailiang Zhang wrote:
>>>>>> On 2016/2/1 11:14, Jason Wang wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 01/27/2016 04:29 PM, zhanghailiang wrote:
>>>>>>>> We add a new helper function netdev_add_filter(), this function
>>>>>>>> can help adding a filter object to a netdev.
>>>>>>>> Besides, we add a is_default member for struct NetFilterState
>>>>>>>> to indicate whether the filter is default or not.
>>>>>>>>
>>>>>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>>>>>> ---
>>>>>>>> v2:
>>>>>>>>      -Re-implement netdev_add_filter() by re-using object_create()
>>>>>>>>       (Jason's suggestion)
>>>>>>>> ---
>>>>>>>>      include/net/filter.h |  7 +++++
>>>>>>>>      net/filter.c         | 80
>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>>>      2 files changed, 87 insertions(+)
>>>>>>>>
>>>>>>>>
>>>
>>> [...]
>>>
>>>>>>>> +
>>>>>>>> +    optarg =
>>>>>>>> g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>>>>>>>> +            filter_type, id, netdev_id, is_default ? "disable" :
>>>>>>>> "enable"
>>>>>>>
>>>>>>> Instead of this, I wonder maybe it's better to:
>>>>>>>
>>>>>>> - store the default filter property into a pointer to string
>>>>>>
>>>>>> Do you mean, pass a string parameter which stores the filter property
>>>>>> instead of
>>>>>> assemble it in this helper ?
>>>>>
>>>>> Yes. E.g just a global string which could be changed by any subsystem.
>>>>> E.g colo may change it to
>>>>> "filter-buffer,interval=0,status=disable". But
>>>>> filter ids need to be generated automatically.
>>>>>
>>>>
>>>> Got it. Then we don't need the global default_netfilter_type[] in
>>>> patch 5,
>>>
>>> Yes.
>>>
>>>> Just use this global string instead ?
>>>
>>> Right.
>>>
>>>>
>>>>>>
>>>>>>> - colo code may change the pointer to "filter-buffer,status=disable"
>>>>>>>
>>>>>>
>>>>>>> Then, there's no need for lots of codes above:
>>>>>>> - no need a "is_default" parameter in netdev_add_filter which
>>>>>>> does not
>>>>>>> scale consider we may want to have more property in the future
>>>>>>> - no need to hacking like "qemu_filter_opts"
>>>>>>
>>>>>> Yes, we can use qemu_find_opts("object") instead of it.
>>>>>>
>>>>>>> - no need to have a special flag like "is_default"
>>>>>>>
>>>>>>
>>>>>> But we have to distinguish the default filter from the common
>>>>>> filter, use the name (id) to distinguish it ?
>>>>>
>>>>> What's the reason that you want to distinguish default filters from
>>>>> others?
>>>>>
>>>>
>>>> The default filters will be used by COLO or MC, (In COLO, we will
>>>> use it
>>>> to control packets buffering/releasing).
>>>
>>> A question is how will you do this?
>>>
>>
>> Er, for COLO, we will enable all the default filter in the
>> initialization stage,
>> then the buffer-filter will buffer all netdev's packets,
>> after doing a checkpoint, we will release all the buffered packets
>> (Flush all default
>> filters' packets).
>
> Right, that's the point. So looks several choices here:
>
> 1) Track each default filter explicitly, generate and record the netdev
> ids for default filter by COLO.  Walk through the ids list and release
> the packet in each filter.
> 2) Track the default filters implicitly. Link all buffer filters (with
> zero interval) in a list during filter buffer initialization. And export
> a helper for COLO to walk them and release packets.
>
> Either looks fine, but maybe 2 is easier.
>

Danies recommended using object_new_with_props() instead of object_create(),
which will avoids the need of format + parse process. It makes the codes
more clean. I have fixed it in v3.

I kept the 'is_default' member for struct NetFilterState. Because comparing
with finding the default filter by the special name ([netdev->id][nop]),
it seems to be more easy for COLO to find/control the default filter with
this flag.
(We added a new helper qemu_foreach_netfilter() in COLO frame, and
we traverse these filters directly without passing any netdev related info.)

But if you think it is still not acceptable to add such a member, i'll remove it
in next verison ;)

Thanks,
Hailiang


>>   If VM is failover, we will set all default filters back to disabled
>> status.
>> (This is a periodic mode for COLO, different from another mode, which
>> we will call it
>> hybrid mode, that is based on colo-proxy, which is in developing by
>> zhangchen)
>>
>> Thanks,
>> Hailiang
>
> Yes, I see.
>
>
> .
>