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

[Jason Wang]

On 02/01/2016 06:40 PM, Hailiang Zhang wrote:
> 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 <address@hidden>
>>>>>>>>> ---
>>>>>>>>> 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.
>>
>
> 2) is a good choice, but I'm a little worry about using zero to
> distinguish if a filter is default filter or not, because users
> can use qom-set to change its value.

Then I see minor issues:

1) Looks like we should prevent user other than COLO from using zero
interval, neither from cli nor 'qom-set'.
2) For the zero interval filter used by COLO, we should not allow user
to change the value of interval.

So I was thinking a new type which is based on current filter-buffer
whose interval is invisible to user.

> If special flag like 'is_default' is not acceptable,
> then we have to use the filter ids to distinguish the default
> buffer-filter
> (here the rule of generation ids for default filter is
> '[netdev-id][DEFAULT_FILTER_TYPE]'
>
> 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.
>>
>>
>> .
>>
>
>