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

[Jason Wang <jasowang@redhat.com>]

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?

> For COLO, we don't want to control (use) other filters that added by
> users.
>
> Thanks,
> Hailiang
>
>> Thanks
>>
>>>
>>> Thanks,
>>> Hailiang
>>>
>>>> Thoughts?
>>>>
>>>>> +    opts = qemu_opts_parse_noisily(&qemu_filter_opts,
>>>>> +                                   optarg, false);
>>>>> +    if (!opts) {
>>>>> +        error_report("Failed to parse param '%s'", optarg);
>>>>> +        exit(1);
>>>>> +    }
>>>>> +    g_free(optarg);
>>>>> +    if (object_create(NULL, opts, &err) < 0) {
>>>>> +        error_report("Failed to create object");
>>>>> +        goto out_clean;
>>>>> +    }
>>>>> +    filter_set_default_flag(id, is_default, &err);
>>>>> +
>>>>> +out_clean:
>>>>> +    qemu_opts_del(opts);
>>>>> +    if (err) {
>>>>> +        error_propagate(errp, err);
>>>>> +    }
>>>>> +}
>>>>> +
>>>>>    static void netfilter_finalize(Object *obj)
>>>>>    {
>>>>>        NetFilterState *nf = NETFILTER(obj);
>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>
>>
>> .
>>
>
>
>