[RFC,v2,3/5] net/filter: Introduce a helper to add a filter to the netdev

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(+)

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(+)
>
> diff --git a/include/net/filter.h b/include/net/filter.h
> index af3c53c..ee1c024 100644
> --- a/include/net/filter.h
> +++ b/include/net/filter.h
> @@ -55,6 +55,7 @@ struct NetFilterState {
>      char *netdev_id;
>      NetClientState *netdev;
>      NetFilterDirection direction;
> +    bool is_default;
>      bool enabled;
>      QTAILQ_ENTRY(NetFilterState) next;
>  };
> @@ -74,4 +75,10 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>                                      int iovcnt,
>                                      void *opaque);
>  
> +void netdev_add_filter(const char *netdev_id,
> +                       const char *filter_type,
> +                       const char *id,
> +                       bool is_default,
> +                       Error **errp);
> +
>  #endif /* QEMU_NET_FILTER_H */
> diff --git a/net/filter.c b/net/filter.c
> index d08a2be..dc7aa9b 100644
> --- a/net/filter.c
> +++ b/net/filter.c
> @@ -214,6 +214,86 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>      QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>  }
>  
> +QemuOptsList qemu_filter_opts = {
> +    .name = "default-filter",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
> +    .desc = {
> +        {
> +            .name = "qom-type",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "id",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "netdev",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "status",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        { /* end of list */ }
> +    },
> +};
> +
> +static void filter_set_default_flag(const char *id,
> +                                    bool is_default,
> +                                    Error **errp)
> +{
> +    Object *obj, *container;
> +    NetFilterState *nf;
> +
> +    container = object_get_objects_root();
> +    obj = object_resolve_path_component(container, id);
> +    if (!obj) {
> +        error_setg(errp, "object id not found");
> +        return;
> +    }
> +    nf = NETFILTER(obj);
> +    nf->is_default = is_default;
> +}
> +
> +void netdev_add_filter(const char *netdev_id,
> +                       const char *filter_type,
> +                       const char *id,
> +                       bool is_default,
> +                       Error **errp)
> +{
> +    NetClientState *nc = qemu_find_netdev(netdev_id);
> +    char *optarg;
> +    QemuOpts *opts = NULL;
> +    Error *err = NULL;
> +
> +    /* FIXME: Not support multiple queues */
> +    if (!nc || nc->queue_index > 1) {
> +        return;
> +    }
> +    /* Not support vhost-net */
> +    if (get_vhost_net(nc)) {
> +        return;
> +    }
> +
> +    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
- 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"
- no need to have a special flag like "is_default"

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);

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(+)
>>
>> diff --git a/include/net/filter.h b/include/net/filter.h
>> index af3c53c..ee1c024 100644
>> --- a/include/net/filter.h
>> +++ b/include/net/filter.h
>> @@ -55,6 +55,7 @@ struct NetFilterState {
>>       char *netdev_id;
>>       NetClientState *netdev;
>>       NetFilterDirection direction;
>> +    bool is_default;
>>       bool enabled;
>>       QTAILQ_ENTRY(NetFilterState) next;
>>   };
>> @@ -74,4 +75,10 @@ ssize_t qemu_netfilter_pass_to_next(NetClientState *sender,
>>                                       int iovcnt,
>>                                       void *opaque);
>>
>> +void netdev_add_filter(const char *netdev_id,
>> +                       const char *filter_type,
>> +                       const char *id,
>> +                       bool is_default,
>> +                       Error **errp);
>> +
>>   #endif /* QEMU_NET_FILTER_H */
>> diff --git a/net/filter.c b/net/filter.c
>> index d08a2be..dc7aa9b 100644
>> --- a/net/filter.c
>> +++ b/net/filter.c
>> @@ -214,6 +214,86 @@ static void netfilter_complete(UserCreatable *uc, Error **errp)
>>       QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>>   }
>>
>> +QemuOptsList qemu_filter_opts = {
>> +    .name = "default-filter",
>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
>> +    .desc = {
>> +        {
>> +            .name = "qom-type",
>> +            .type = QEMU_OPT_STRING,
>> +        },{
>> +            .name = "id",
>> +            .type = QEMU_OPT_STRING,
>> +        },{
>> +            .name = "netdev",
>> +            .type = QEMU_OPT_STRING,
>> +        },{
>> +            .name = "status",
>> +            .type = QEMU_OPT_STRING,
>> +        },
>> +        { /* end of list */ }
>> +    },
>> +};
>> +
>> +static void filter_set_default_flag(const char *id,
>> +                                    bool is_default,
>> +                                    Error **errp)
>> +{
>> +    Object *obj, *container;
>> +    NetFilterState *nf;
>> +
>> +    container = object_get_objects_root();
>> +    obj = object_resolve_path_component(container, id);
>> +    if (!obj) {
>> +        error_setg(errp, "object id not found");
>> +        return;
>> +    }
>> +    nf = NETFILTER(obj);
>> +    nf->is_default = is_default;
>> +}
>> +
>> +void netdev_add_filter(const char *netdev_id,
>> +                       const char *filter_type,
>> +                       const char *id,
>> +                       bool is_default,
>> +                       Error **errp)
>> +{
>> +    NetClientState *nc = qemu_find_netdev(netdev_id);
>> +    char *optarg;
>> +    QemuOpts *opts = NULL;
>> +    Error *err = NULL;
>> +
>> +    /* FIXME: Not support multiple queues */
>> +    if (!nc || nc->queue_index > 1) {
>> +        return;
>> +    }
>> +    /* Not support vhost-net */
>> +    if (get_vhost_net(nc)) {
>> +        return;
>> +    }
>> +
>> +    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 ?

> - 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 ?

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);
>
>
> .
>

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(+)
>>>
>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>> index af3c53c..ee1c024 100644
>>> --- a/include/net/filter.h
>>> +++ b/include/net/filter.h
>>> @@ -55,6 +55,7 @@ struct NetFilterState {
>>>       char *netdev_id;
>>>       NetClientState *netdev;
>>>       NetFilterDirection direction;
>>> +    bool is_default;
>>>       bool enabled;
>>>       QTAILQ_ENTRY(NetFilterState) next;
>>>   };
>>> @@ -74,4 +75,10 @@ ssize_t
>>> qemu_netfilter_pass_to_next(NetClientState *sender,
>>>                                       int iovcnt,
>>>                                       void *opaque);
>>>
>>> +void netdev_add_filter(const char *netdev_id,
>>> +                       const char *filter_type,
>>> +                       const char *id,
>>> +                       bool is_default,
>>> +                       Error **errp);
>>> +
>>>   #endif /* QEMU_NET_FILTER_H */
>>> diff --git a/net/filter.c b/net/filter.c
>>> index d08a2be..dc7aa9b 100644
>>> --- a/net/filter.c
>>> +++ b/net/filter.c
>>> @@ -214,6 +214,86 @@ static void netfilter_complete(UserCreatable
>>> *uc, Error **errp)
>>>       QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>>>   }
>>>
>>> +QemuOptsList qemu_filter_opts = {
>>> +    .name = "default-filter",
>>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
>>> +    .desc = {
>>> +        {
>>> +            .name = "qom-type",
>>> +            .type = QEMU_OPT_STRING,
>>> +        },{
>>> +            .name = "id",
>>> +            .type = QEMU_OPT_STRING,
>>> +        },{
>>> +            .name = "netdev",
>>> +            .type = QEMU_OPT_STRING,
>>> +        },{
>>> +            .name = "status",
>>> +            .type = QEMU_OPT_STRING,
>>> +        },
>>> +        { /* end of list */ }
>>> +    },
>>> +};
>>> +
>>> +static void filter_set_default_flag(const char *id,
>>> +                                    bool is_default,
>>> +                                    Error **errp)
>>> +{
>>> +    Object *obj, *container;
>>> +    NetFilterState *nf;
>>> +
>>> +    container = object_get_objects_root();
>>> +    obj = object_resolve_path_component(container, id);
>>> +    if (!obj) {
>>> +        error_setg(errp, "object id not found");
>>> +        return;
>>> +    }
>>> +    nf = NETFILTER(obj);
>>> +    nf->is_default = is_default;
>>> +}
>>> +
>>> +void netdev_add_filter(const char *netdev_id,
>>> +                       const char *filter_type,
>>> +                       const char *id,
>>> +                       bool is_default,
>>> +                       Error **errp)
>>> +{
>>> +    NetClientState *nc = qemu_find_netdev(netdev_id);
>>> +    char *optarg;
>>> +    QemuOpts *opts = NULL;
>>> +    Error *err = NULL;
>>> +
>>> +    /* FIXME: Not support multiple queues */
>>> +    if (!nc || nc->queue_index > 1) {
>>> +        return;
>>> +    }
>>> +    /* Not support vhost-net */
>>> +    if (get_vhost_net(nc)) {
>>> +        return;
>>> +    }
>>> +
>>> +    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.

>
>> - 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?

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);
>>
>>
>> .
>>
>
>

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(+)
>>>>
>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>> index af3c53c..ee1c024 100644
>>>> --- a/include/net/filter.h
>>>> +++ b/include/net/filter.h
>>>> @@ -55,6 +55,7 @@ struct NetFilterState {
>>>>        char *netdev_id;
>>>>        NetClientState *netdev;
>>>>        NetFilterDirection direction;
>>>> +    bool is_default;
>>>>        bool enabled;
>>>>        QTAILQ_ENTRY(NetFilterState) next;
>>>>    };
>>>> @@ -74,4 +75,10 @@ ssize_t
>>>> qemu_netfilter_pass_to_next(NetClientState *sender,
>>>>                                        int iovcnt,
>>>>                                        void *opaque);
>>>>
>>>> +void netdev_add_filter(const char *netdev_id,
>>>> +                       const char *filter_type,
>>>> +                       const char *id,
>>>> +                       bool is_default,
>>>> +                       Error **errp);
>>>> +
>>>>    #endif /* QEMU_NET_FILTER_H */
>>>> diff --git a/net/filter.c b/net/filter.c
>>>> index d08a2be..dc7aa9b 100644
>>>> --- a/net/filter.c
>>>> +++ b/net/filter.c
>>>> @@ -214,6 +214,86 @@ static void netfilter_complete(UserCreatable
>>>> *uc, Error **errp)
>>>>        QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>>>>    }
>>>>
>>>> +QemuOptsList qemu_filter_opts = {
>>>> +    .name = "default-filter",
>>>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
>>>> +    .desc = {
>>>> +        {
>>>> +            .name = "qom-type",
>>>> +            .type = QEMU_OPT_STRING,
>>>> +        },{
>>>> +            .name = "id",
>>>> +            .type = QEMU_OPT_STRING,
>>>> +        },{
>>>> +            .name = "netdev",
>>>> +            .type = QEMU_OPT_STRING,
>>>> +        },{
>>>> +            .name = "status",
>>>> +            .type = QEMU_OPT_STRING,
>>>> +        },
>>>> +        { /* end of list */ }
>>>> +    },
>>>> +};
>>>> +
>>>> +static void filter_set_default_flag(const char *id,
>>>> +                                    bool is_default,
>>>> +                                    Error **errp)
>>>> +{
>>>> +    Object *obj, *container;
>>>> +    NetFilterState *nf;
>>>> +
>>>> +    container = object_get_objects_root();
>>>> +    obj = object_resolve_path_component(container, id);
>>>> +    if (!obj) {
>>>> +        error_setg(errp, "object id not found");
>>>> +        return;
>>>> +    }
>>>> +    nf = NETFILTER(obj);
>>>> +    nf->is_default = is_default;
>>>> +}
>>>> +
>>>> +void netdev_add_filter(const char *netdev_id,
>>>> +                       const char *filter_type,
>>>> +                       const char *id,
>>>> +                       bool is_default,
>>>> +                       Error **errp)
>>>> +{
>>>> +    NetClientState *nc = qemu_find_netdev(netdev_id);
>>>> +    char *optarg;
>>>> +    QemuOpts *opts = NULL;
>>>> +    Error *err = NULL;
>>>> +
>>>> +    /* FIXME: Not support multiple queues */
>>>> +    if (!nc || nc->queue_index > 1) {
>>>> +        return;
>>>> +    }
>>>> +    /* Not support vhost-net */
>>>> +    if (get_vhost_net(nc)) {
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    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,
Just use this global string instead ?

>>
>>> - 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).
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);
>>>
>>>
>>> .
>>>
>>
>>
>
>
> .
>

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(+)
>>>>>
>>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>>> index af3c53c..ee1c024 100644
>>>>> --- a/include/net/filter.h
>>>>> +++ b/include/net/filter.h
>>>>> @@ -55,6 +55,7 @@ struct NetFilterState {
>>>>>        char *netdev_id;
>>>>>        NetClientState *netdev;
>>>>>        NetFilterDirection direction;
>>>>> +    bool is_default;
>>>>>        bool enabled;
>>>>>        QTAILQ_ENTRY(NetFilterState) next;
>>>>>    };
>>>>> @@ -74,4 +75,10 @@ ssize_t
>>>>> qemu_netfilter_pass_to_next(NetClientState *sender,
>>>>>                                        int iovcnt,
>>>>>                                        void *opaque);
>>>>>
>>>>> +void netdev_add_filter(const char *netdev_id,
>>>>> +                       const char *filter_type,
>>>>> +                       const char *id,
>>>>> +                       bool is_default,
>>>>> +                       Error **errp);
>>>>> +
>>>>>    #endif /* QEMU_NET_FILTER_H */
>>>>> diff --git a/net/filter.c b/net/filter.c
>>>>> index d08a2be..dc7aa9b 100644
>>>>> --- a/net/filter.c
>>>>> +++ b/net/filter.c
>>>>> @@ -214,6 +214,86 @@ static void netfilter_complete(UserCreatable
>>>>> *uc, Error **errp)
>>>>>        QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>>>>>    }
>>>>>
>>>>> +QemuOptsList qemu_filter_opts = {
>>>>> +    .name = "default-filter",
>>>>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
>>>>> +    .desc = {
>>>>> +        {
>>>>> +            .name = "qom-type",
>>>>> +            .type = QEMU_OPT_STRING,
>>>>> +        },{
>>>>> +            .name = "id",
>>>>> +            .type = QEMU_OPT_STRING,
>>>>> +        },{
>>>>> +            .name = "netdev",
>>>>> +            .type = QEMU_OPT_STRING,
>>>>> +        },{
>>>>> +            .name = "status",
>>>>> +            .type = QEMU_OPT_STRING,
>>>>> +        },
>>>>> +        { /* end of list */ }
>>>>> +    },
>>>>> +};
>>>>> +
>>>>> +static void filter_set_default_flag(const char *id,
>>>>> +                                    bool is_default,
>>>>> +                                    Error **errp)
>>>>> +{
>>>>> +    Object *obj, *container;
>>>>> +    NetFilterState *nf;
>>>>> +
>>>>> +    container = object_get_objects_root();
>>>>> +    obj = object_resolve_path_component(container, id);
>>>>> +    if (!obj) {
>>>>> +        error_setg(errp, "object id not found");
>>>>> +        return;
>>>>> +    }
>>>>> +    nf = NETFILTER(obj);
>>>>> +    nf->is_default = is_default;
>>>>> +}
>>>>> +
>>>>> +void netdev_add_filter(const char *netdev_id,
>>>>> +                       const char *filter_type,
>>>>> +                       const char *id,
>>>>> +                       bool is_default,
>>>>> +                       Error **errp)
>>>>> +{
>>>>> +    NetClientState *nc = qemu_find_netdev(netdev_id);
>>>>> +    char *optarg;
>>>>> +    QemuOpts *opts = NULL;
>>>>> +    Error *err = NULL;
>>>>> +
>>>>> +    /* FIXME: Not support multiple queues */
>>>>> +    if (!nc || nc->queue_index > 1) {
>>>>> +        return;
>>>>> +    }
>>>>> +    /* Not support vhost-net */
>>>>> +    if (get_vhost_net(nc)) {
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    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,
> Just use this global string instead ?
>
>>>
>>>> - 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).
> For COLO, we don't want to control (use) other filters that added by users.

I think Jason's point is that COLO is a manager, you can add the filter
to netdev when doing COLO, so the only difference between COLO's default
filter and other filter is that COLO's filter (for example buffer
filter) is added by COLO, and the other filter is added by user
specifing -filter.

>
> 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);
>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>
>>
>> .
>>
>
>
>

On 2016/2/1 16:05, Yang Hongyang 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(+)
>>>>>>
>>>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>>>> index af3c53c..ee1c024 100644
>>>>>> --- a/include/net/filter.h
>>>>>> +++ b/include/net/filter.h
>>>>>> @@ -55,6 +55,7 @@ struct NetFilterState {
>>>>>>        char *netdev_id;
>>>>>>        NetClientState *netdev;
>>>>>>        NetFilterDirection direction;
>>>>>> +    bool is_default;
>>>>>>        bool enabled;
>>>>>>        QTAILQ_ENTRY(NetFilterState) next;
>>>>>>    };
>>>>>> @@ -74,4 +75,10 @@ ssize_t
>>>>>> qemu_netfilter_pass_to_next(NetClientState *sender,
>>>>>>                                        int iovcnt,
>>>>>>                                        void *opaque);
>>>>>>
>>>>>> +void netdev_add_filter(const char *netdev_id,
>>>>>> +                       const char *filter_type,
>>>>>> +                       const char *id,
>>>>>> +                       bool is_default,
>>>>>> +                       Error **errp);
>>>>>> +
>>>>>>    #endif /* QEMU_NET_FILTER_H */
>>>>>> diff --git a/net/filter.c b/net/filter.c
>>>>>> index d08a2be..dc7aa9b 100644
>>>>>> --- a/net/filter.c
>>>>>> +++ b/net/filter.c
>>>>>> @@ -214,6 +214,86 @@ static void netfilter_complete(UserCreatable
>>>>>> *uc, Error **errp)
>>>>>>        QTAILQ_INSERT_TAIL(&nf->netdev->filters, nf, next);
>>>>>>    }
>>>>>>
>>>>>> +QemuOptsList qemu_filter_opts = {
>>>>>> +    .name = "default-filter",
>>>>>> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
>>>>>> +    .desc = {
>>>>>> +        {
>>>>>> +            .name = "qom-type",
>>>>>> +            .type = QEMU_OPT_STRING,
>>>>>> +        },{
>>>>>> +            .name = "id",
>>>>>> +            .type = QEMU_OPT_STRING,
>>>>>> +        },{
>>>>>> +            .name = "netdev",
>>>>>> +            .type = QEMU_OPT_STRING,
>>>>>> +        },{
>>>>>> +            .name = "status",
>>>>>> +            .type = QEMU_OPT_STRING,
>>>>>> +        },
>>>>>> +        { /* end of list */ }
>>>>>> +    },
>>>>>> +};
>>>>>> +
>>>>>> +static void filter_set_default_flag(const char *id,
>>>>>> +                                    bool is_default,
>>>>>> +                                    Error **errp)
>>>>>> +{
>>>>>> +    Object *obj, *container;
>>>>>> +    NetFilterState *nf;
>>>>>> +
>>>>>> +    container = object_get_objects_root();
>>>>>> +    obj = object_resolve_path_component(container, id);
>>>>>> +    if (!obj) {
>>>>>> +        error_setg(errp, "object id not found");
>>>>>> +        return;
>>>>>> +    }
>>>>>> +    nf = NETFILTER(obj);
>>>>>> +    nf->is_default = is_default;
>>>>>> +}
>>>>>> +
>>>>>> +void netdev_add_filter(const char *netdev_id,
>>>>>> +                       const char *filter_type,
>>>>>> +                       const char *id,
>>>>>> +                       bool is_default,
>>>>>> +                       Error **errp)
>>>>>> +{
>>>>>> +    NetClientState *nc = qemu_find_netdev(netdev_id);
>>>>>> +    char *optarg;
>>>>>> +    QemuOpts *opts = NULL;
>>>>>> +    Error *err = NULL;
>>>>>> +
>>>>>> +    /* FIXME: Not support multiple queues */
>>>>>> +    if (!nc || nc->queue_index > 1) {
>>>>>> +        return;
>>>>>> +    }
>>>>>> +    /* Not support vhost-net */
>>>>>> +    if (get_vhost_net(nc)) {
>>>>>> +        return;
>>>>>> +    }
>>>>>> +
>>>>>> +    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,
>> Just use this global string instead ?
>>
>>>>
>>>>> - 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).
>> For COLO, we don't want to control (use) other filters that added by users.
>
> I think Jason's point is that COLO is a manager, you can add the filter
> to netdev when doing COLO, so the only difference between COLO's default

Er, then we came back to the original question, 'is it necessary to add each netdev
a default filter ?'
If we add the a filter to netdev when doing COLO, it will be added dynamically,
Here we want to add each netdev a default filter while launch QEMU
(no matter if this VM will go into COLO or not),
just to support hot-add NIC for VM while in COLO lifetime.

> filter and other filter is that COLO's filter (for example buffer
> filter) is added by COLO, and the other filter is added by user
> specifing -filter.
>
>>
>> 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);
>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>
>>>
>>> .
>>>
>>
>>
>>
>

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);
>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>
>>
>> .
>>
>
>
>

On 02/01/2016 04:21 PM, Hailiang Zhang wrote:
>>>>>>
>>>>>> 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,
>>> Just use this global string instead ?
>>>
>>>>>
>>>>>> - 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).
>>> For COLO, we don't want to control (use) other filters that added by
>>> users.
>>
>> I think Jason's point is that COLO is a manager, you can add the filter
>> to netdev when doing COLO, so the only difference between COLO's default
>
> Er, then we came back to the original question, 'is it necessary to
> add each netdev
> a default filter ?'

The question could be extended to:

1) Do we need a default filter? I think the answer is yes, but of course
COLO can work even without this.
2) Do we want to implement COLO on top of default filter? If yes, as you
suggest, we may record the ids of the default filter and do what ever we
what. If not, COLO need codes to go through each netdev and add filter
itself (hotplug is not supported). Or you want management to do this,
then even hotplug could be supported.

Any thoughts?

> If we add the a filter to netdev when doing COLO, it will be added
> dynamically,
> Here we want to add each netdev a default filter while launch QEMU
> (no matter if this VM will go into COLO or not),
> just to support hot-add NIC for VM while in COLO lifetime.

Yes.

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). 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

>> 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);
>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>
>>>
>>> .
>>>
>>
>>
>>
>
>
> .
>

On 2016/2/1 17:18, Jason Wang wrote:
>
>
> On 02/01/2016 04:21 PM, Hailiang Zhang wrote:
>>>>>>>
>>>>>>> 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,
>>>> Just use this global string instead ?
>>>>
>>>>>>
>>>>>>> - 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).
>>>> For COLO, we don't want to control (use) other filters that added by
>>>> users.
>>>
>>> I think Jason's point is that COLO is a manager, you can add the filter
>>> to netdev when doing COLO, so the only difference between COLO's default
>>
>> Er, then we came back to the original question, 'is it necessary to
>> add each netdev
>> a default filter ?'
>
> The question could be extended to:
>
> 1) Do we need a default filter? I think the answer is yes, but of course
> COLO can work even without this.

Yes, after colo-proxy is realized, we can switch to colo-proxy
(It should have the capability of buffer and release packets directly).
But for now, we want to merge COLO prototype without colo-proxy, the COLO
prototype should have the basic capability. Just like Remus or
Micro-checkpointing. It is based on the default buffer-filter to control net
packets.

> 2) Do we want to implement COLO on top of default filter? If yes, as you
> suggest, we may record the ids of the default filter and do what ever we

Yes, we need it.

> what. If not, COLO need codes to go through each netdev and add filter
> itself (hotplug is not supported). Or you want management to do this,
> then even hotplug could be supported.
>

We also want to support hotplug during VM is in COLO state in the future.
(For this point, I'm not quite sure if this usage case is really exist.)

Thanks,
Hailiang

> Any thoughts?
>
>> If we add the a filter to netdev when doing COLO, it will be added
>> dynamically,
>> Here we want to add each netdev a default filter while launch QEMU
>> (no matter if this VM will go into COLO or not),
>> just to support hot-add NIC for VM while in COLO lifetime.
>
> Yes.
>
>
>
>
> .
>

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.

>  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.

On 02/01/2016 05:39 PM, Hailiang Zhang wrote:
> On 2016/2/1 17:18, Jason Wang wrote:
>>
>>
>> On 02/01/2016 04:21 PM, Hailiang Zhang wrote:
>>>>>>>>
>>>>>>>> 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,
>>>>> Just use this global string instead ?
>>>>>
>>>>>>>
>>>>>>>> - 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).
>>>>> For COLO, we don't want to control (use) other filters that added by
>>>>> users.
>>>>
>>>> I think Jason's point is that COLO is a manager, you can add the
>>>> filter
>>>> to netdev when doing COLO, so the only difference between COLO's
>>>> default
>>>
>>> Er, then we came back to the original question, 'is it necessary to
>>> add each netdev
>>> a default filter ?'
>>
>> The question could be extended to:
>>
>> 1) Do we need a default filter? I think the answer is yes, but of course
>> COLO can work even without this.
>
> Yes, after colo-proxy is realized, we can switch to colo-proxy
> (It should have the capability of buffer and release packets directly).
> But for now, we want to merge COLO prototype without colo-proxy, the COLO
> prototype should have the basic capability.

Right, I see.

> Just like Remus or
> Micro-checkpointing. It is based on the default buffer-filter to
> control net
> packets.
>
>> 2) Do we want to implement COLO on top of default filter? If yes, as you
>> suggest, we may record the ids of the default filter and do what ever we
>
> Yes, we need it.

Or just as I reply, all buffer filters (with zero interval) could be
tracked by itself. So as you see, several ways could go. It's your call
to choose one of them.

>
>> what. If not, COLO need codes to go through each netdev and add filter
>> itself (hotplug is not supported). Or you want management to do this,
>> then even hotplug could be supported.
>>
>
> We also want to support hotplug during VM is in COLO state in the future.
> (For this point, I'm not quite sure if this usage case is really exist.)
>
> Thanks,
> Hailiang

Support hotplug should be useful I think. But I'm also ok if you don't
want to consider for it now.

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.
>

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.
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.
>
>
> .
>

On 2016/2/1 17:49, Jason Wang wrote:
>
>
> On 02/01/2016 05:39 PM, Hailiang Zhang wrote:
>> On 2016/2/1 17:18, Jason Wang wrote:
>>>
>>>
>>> On 02/01/2016 04:21 PM, Hailiang Zhang wrote:
>>>>>>>>>
>>>>>>>>> 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,
>>>>>> Just use this global string instead ?
>>>>>>
>>>>>>>>
>>>>>>>>> - 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).
>>>>>> For COLO, we don't want to control (use) other filters that added by
>>>>>> users.
>>>>>
>>>>> I think Jason's point is that COLO is a manager, you can add the
>>>>> filter
>>>>> to netdev when doing COLO, so the only difference between COLO's
>>>>> default
>>>>
>>>> Er, then we came back to the original question, 'is it necessary to
>>>> add each netdev
>>>> a default filter ?'
>>>
>>> The question could be extended to:
>>>
>>> 1) Do we need a default filter? I think the answer is yes, but of course
>>> COLO can work even without this.
>>
>> Yes, after colo-proxy is realized, we can switch to colo-proxy
>> (It should have the capability of buffer and release packets directly).
>> But for now, we want to merge COLO prototype without colo-proxy, the COLO
>> prototype should have the basic capability.
>
> Right, I see.
>
>> Just like Remus or
>> Micro-checkpointing. It is based on the default buffer-filter to
>> control net
>> packets.
>>
>>> 2) Do we want to implement COLO on top of default filter? If yes, as you
>>> suggest, we may record the ids of the default filter and do what ever we
>>
>> Yes, we need it.
>
> Or just as I reply, all buffer filters (with zero interval) could be
> tracked by itself. So as you see, several ways could go. It's your call
> to choose one of them.
>

OK, got it.

>>
>>> what. If not, COLO need codes to go through each netdev and add filter
>>> itself (hotplug is not supported). Or you want management to do this,
>>> then even hotplug could be supported.
>>>
>>
>> We also want to support hotplug during VM is in COLO state in the future.
>> (For this point, I'm not quite sure if this usage case is really exist.)
>>
>> Thanks,
>> Hailiang
>
> Support hotplug should be useful I think. But I'm also ok if you don't
> want to consider for it now.
>

Thanks very much.

Hailiang

On Wed, Jan 27, 2016 at 04:29:38PM +0800, 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(+)

> +void netdev_add_filter(const char *netdev_id,
> +                       const char *filter_type,
> +                       const char *id,
> +                       bool is_default,
> +                       Error **errp)
> +{
> +    NetClientState *nc = qemu_find_netdev(netdev_id);
> +    char *optarg;
> +    QemuOpts *opts = NULL;
> +    Error *err = NULL;
> +
> +    /* FIXME: Not support multiple queues */
> +    if (!nc || nc->queue_index > 1) {
> +        return;
> +    }
> +    /* Not support vhost-net */
> +    if (get_vhost_net(nc)) {
> +        return;
> +    }
> +
> +    optarg = g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
> +            filter_type, id, netdev_id, is_default ? "disable" : "enable");
> +    opts = qemu_opts_parse_noisily(&qemu_filter_opts,
> +                                   optarg, false);

Formatting a string and then immediately parsing it again is totally
crazy, not least because you're not taking care to do escaping of
special characters like ',' in the string parameters.

> +    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;
> +    }

Don't use object_create() - use object_new_with_props() which avoids
the need to format + parse the string above. ie do

  object_new_with_props(filter_type,
                        object_get_objects_root(),
			id,
			&err,
			"netdev", netdev_id,
			"status", is_default ? "disable" : "enable",
			NULL);


Regards,
Daniel

On 2016/2/1 18:43, Daniel P. Berrange wrote:
> On Wed, Jan 27, 2016 at 04:29:38PM +0800, 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(+)
>
>> +void netdev_add_filter(const char *netdev_id,
>> +                       const char *filter_type,
>> +                       const char *id,
>> +                       bool is_default,
>> +                       Error **errp)
>> +{
>> +    NetClientState *nc = qemu_find_netdev(netdev_id);
>> +    char *optarg;
>> +    QemuOpts *opts = NULL;
>> +    Error *err = NULL;
>> +
>> +    /* FIXME: Not support multiple queues */
>> +    if (!nc || nc->queue_index > 1) {
>> +        return;
>> +    }
>> +    /* Not support vhost-net */
>> +    if (get_vhost_net(nc)) {
>> +        return;
>> +    }
>> +
>> +    optarg = g_strdup_printf("qom-type=%s,id=%s,netdev=%s,status=%s",
>> +            filter_type, id, netdev_id, is_default ? "disable" : "enable");
>> +    opts = qemu_opts_parse_noisily(&qemu_filter_opts,
>> +                                   optarg, false);
>
> Formatting a string and then immediately parsing it again is totally
> crazy, not least because you're not taking care to do escaping of
> special characters like ',' in the string parameters.
>

Got it.

>> +    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;
>> +    }
>
> Don't use object_create() - use object_new_with_props() which avoids
> the need to format + parse the string above. ie do
>
>    object_new_with_props(filter_type,
>                          object_get_objects_root(),
> 			id,
> 			&err,
> 			"netdev", netdev_id,
> 			"status", is_default ? "disable" : "enable",
> 			NULL);
>
>

Ha, that's really a good idea, i will fix it like that
in next version. Thank you very much.

Hailiang

> Regards,
> Daniel
>

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.
>
>
> .
>

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 <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.
>>
>
> 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.
>>
>>
>> .
>>
>
>

On 2016/2/5 14:19, Jason Wang wrote:
>
>
> 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 <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.
>>>
>>
>> 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:
>

Great, you are still online :)

> 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.
>

It seems that, we have prevented this case, if we set the interval
to zero through qom-set, it will report error:
"Property 'filter-buffer.interval' requires a positive value", also if
launch qemu with CLI "-object filter-buffer,id=f0,netdev=hn0,queue=rx,interval=0",
it also reported the error "Property 'filter-buffer.interval' requires a positive value".

This is because we judge this value in filter_buffer_set_interval(),
but the 'interval' property is optional, users can use ignore this property while launch
qemu and its default value will be zero, so we still can't use this value to identify default
filter.

Other choices to drop the troublesome 'is_default' flag is,
1) use a list to store all the default filters, add them to the list in
netdev_add_default_filters(). but we need add a new member
'QTAILQ_ENTRY(NetFilterState) internal_next' to struct NetFilterState,
The benefit of this scheme is, it will more easy to traverse and control
the default filters, we don't have to traverse all netfilters to identify them.
2) Use the name to identify them, more the name rule for default filter is
'[netdev->id]nop'.

Which one do you think is more acceptable ? Or any other suggestions ?

Thanks,
Hailiang

>> 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.
>>>
>>>
>>> .
>>>
>>
>>
>
>
> .
>

On 02/05/2016 03:01 PM, Hailiang Zhang wrote:
> On 2016/2/5 14:19, Jason Wang wrote:
>>
>>
>> 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 <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.
>>>>
>>>
>>> 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:
>>
>
> Great, you are still online :)

Right :)

>
>> 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.
>>
>
> It seems that, we have prevented this case, if we set the interval
> to zero through qom-set, it will report error:
> "Property 'filter-buffer.interval' requires a positive value", also if
> launch qemu with CLI "-object
> filter-buffer,id=f0,netdev=hn0,queue=rx,interval=0",
> it also reported the error "Property 'filter-buffer.interval' requires
> a positive value".
>
> This is because we judge this value in filter_buffer_set_interval(),
> but the 'interval' property is optional, users can use ignore this
> property while launch
> qemu and its default value will be zero, so we still can't use this
> value to identify default
> filter.

Cool, good to know this.

>
> Other choices to drop the troublesome 'is_default' flag is,
> 1) use a list to store all the default filters, add them to the list in
> netdev_add_default_filters(). but we need add a new member
> 'QTAILQ_ENTRY(NetFilterState) internal_next' to struct NetFilterState,
> The benefit of this scheme is, it will more easy to traverse and control
> the default filters, we don't have to traverse all netfilters to
> identify them.

This is pretty similar to what I suggested above.  The only difference
is that you propose to track this at filter layer instead of buffer
filter implementation.

Think hard about this. Consider:

1) Buffer is the only user for something like default filter and there's
no plan for exporting default filter configuration through cli (at least
in short term).
2) All default filter buffer needs to be tracked in a list for COLO to
buffer/relase packets.

Adding the ability of default filter is good but may need more thoughts.
For example, cli design and to be generic enough for all kinds of filter
(you may want to generate filename dynamically for dump as a default
filter which sounds bad). To reduce the extra efforts and converge this
soon , maybe we could do something more simpler.

- Instead of a explicit default filter, using something like
notifier/callback in net_dev_init(). Then COLO can register a callback
for this notifier. Each time a netdev is created, COLO will be notified
and can do whatever it wants (E.g adding a buffer filter with zero
interval).
- With above, there's no need to care about the zero interval setting
from cli. Then filter buffer can track all filters with zero interval,
and export helpers for COLO to buffer or release the packet.


Thoughts?

Thanks

> 2) Use the name to identify them, more the name rule for default
> filter is
> '[netdev->id]nop'.
>
> Which one do you think is more acceptable ? Or any other suggestions ?
>
> Thanks,
> Hailiang
>
>>> 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.
>>>>
>>>>
>>>> .
>>>>
>>>
>>>
>>
>>
>> .
>>
>
>

On 2016/2/5 15:40, Jason Wang wrote:
>
>
> On 02/05/2016 03:01 PM, Hailiang Zhang wrote:
>> On 2016/2/5 14:19, Jason Wang wrote:
>>>
>>>
>>> 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 <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.
>>>>>
>>>>
>>>> 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:
>>>
>>
>> Great, you are still online :)
>
> Right :)
>
>>
>>> 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.
>>>
>>
>> It seems that, we have prevented this case, if we set the interval
>> to zero through qom-set, it will report error:
>> "Property 'filter-buffer.interval' requires a positive value", also if
>> launch qemu with CLI "-object
>> filter-buffer,id=f0,netdev=hn0,queue=rx,interval=0",
>> it also reported the error "Property 'filter-buffer.interval' requires
>> a positive value".
>>
>> This is because we judge this value in filter_buffer_set_interval(),
>> but the 'interval' property is optional, users can use ignore this
>> property while launch
>> qemu and its default value will be zero, so we still can't use this
>> value to identify default
>> filter.
>
> Cool, good to know this.
>
>>
>> Other choices to drop the troublesome 'is_default' flag is,
>> 1) use a list to store all the default filters, add them to the list in
>> netdev_add_default_filters(). but we need add a new member
>> 'QTAILQ_ENTRY(NetFilterState) internal_next' to struct NetFilterState,
>> The benefit of this scheme is, it will more easy to traverse and control
>> the default filters, we don't have to traverse all netfilters to
>> identify them.
>
> This is pretty similar to what I suggested above.  The only difference
> is that you propose to track this at filter layer instead of buffer
> filter implementation.
>
> Think hard about this. Consider:
>
> 1) Buffer is the only user for something like default filter and there's
> no plan for exporting default filter configuration through cli (at least
> in short term).
> 2) All default filter buffer needs to be tracked in a list for COLO to
> buffer/relase packets.
>
> Adding the ability of default filter is good but may need more thoughts.
> For example, cli design and to be generic enough for all kinds of filter
> (you may want to generate filename dynamically for dump as a default
> filter which sounds bad). To reduce the extra efforts and converge this
> soon , maybe we could do something more simpler.
>

Totally agree, and for now, it seems that only COLO use the buffer filter.
After colo-proxy is merged, COLO will switch to colo-proxy.
So realizing it in a simple way is a good idea.

> - Instead of a explicit default filter, using something like
> notifier/callback in net_dev_init(). Then COLO can register a callback
> for this notifier. Each time a netdev is created, COLO will be notified
> and can do whatever it wants (E.g adding a buffer filter with zero
> interval).

Yes, in this way, we can still support hot-add netfilter,
and can also handle it with the later colo-proxy.

> - With above, there's no need to care about the zero interval setting
> from cli. Then filter buffer can track all filters with zero interval,
> and export helpers for COLO to buffer or release the packet.
>
>

Thank you very much for your patience :)

> Thoughts?
>
> Thanks
>
>> 2) Use the name to identify them, more the name rule for default
>> filter is
>> '[netdev->id]nop'.
>>
>> Which one do you think is more acceptable ? Or any other suggestions ?
>>
>> Thanks,
>> Hailiang
>>
>>>> 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.
>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>
>>>
>>> .
>>>
>>
>>
>
>
> .
>