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

[Jason Wang]

On 01/27/2016 08:37 AM, Hailiang Zhang wrote:
> On 2016/1/26 11:18, Jason Wang wrote:
>>
>>
>> On 01/25/2016 03:22 PM, Hailiang Zhang wrote:
>>> On 2016/1/25 13:18, Jason Wang wrote:
>>>>
>>>>
>>>> On 01/22/2016 04:36 PM, zhanghailiang wrote:
>>>>> We add each netdev a default buffer filter, which the name is
>>>>> 'nop', and the default buffer filter is disabled, so it has
>>>>> no side effect for packets delivering in qemu net layer.
>>>>>
>>>>> The default buffer filter can be used by COLO or Micro-checkpoint,
>>>>> The reason we add the default filter is we hope to support
>>>>> hot add network during COLO state in future.
>>>>>
>>>>> Signed-off-by: zhanghailiang <address@hidden>
>>>>> ---
>>>>>    include/net/filter.h | 11 +++++++++++
>>>>>    net/dump.c           |  2 --
>>>>>    net/filter.c         | 15 ++++++++++++++-
>>>>>    net/net.c            | 18 ++++++++++++++++++
>>>>>    4 files changed, 43 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/include/net/filter.h b/include/net/filter.h
>>>>> index c7bd8f9..2043609 100644
>>>>> --- a/include/net/filter.h
>>>>> +++ b/include/net/filter.h
>>>>> @@ -22,6 +22,16 @@
>>>>>    #define NETFILTER_CLASS(klass) \
>>>>>        OBJECT_CLASS_CHECK(NetFilterClass, (klass), TYPE_NETFILTER)
>>>>>
>>
>> [...]
>>
>>>>>
>>>>>        nf->netdev = ncs[0];
>>>>> +    nf->is_default = !strcmp(path, DEFAULT_FILTER_NAME);
>>>>> +    /*
>>>>> +    * For the default buffer filter, it will be disabled by default,
>>>>> +    * So it will not buffer any packets.
>>>>> +    */
>>>>> +    if (nf->is_default) {
>>>>> +        nf->enabled = false;
>>>>> +    }
>>>>
>>>> This seems not very elegant. Besides DEFAULT_FILTER_NAME(TYPE), we may
>>>> also want a DEFAULT_FILTER_PROPERTIES? Then you can store the "status"
>>>> into properties.
>>>>
>>>
>>> A little confused, do you mean add a 'default' property for filter ?
>>> Just like the new 'status' property which is exported to users ?
>>> Is the type of 'default' property string or bool ?
>>
>> For example, is it possible to store the default property into a string
>> and just create the filter through qemu_opts_parse_noisily() by just
>
> We still need to use some *visit* helpers to realize the capability,
> because the object_add() helper need a 'Visitor *v' parameter, and the
> codes
> will be like:
> QemuOptsList qemu_filter_opts = {
>     .name = "default-filter",
>     .head = QTAILQ_HEAD_INITIALIZER(qemu_filter_opts.head),
>     .desc = {
>         {
>             .name = "netdev",
>             .type = QEMU_OPT_STRING,
>         },{
>             .name = "status",
>             .type = QEMU_OPT_STRING,
>         },
>         { /* end of list */ }
>     },
> };
> void netdev_add_filter(const char *netdev_id,
>                        const char *filter_type,
>                        const char *id,
>                        bool is_default,
>                        Error **errp)
> {
>    sprintf(optarg, "netdev=%s,status=%s", netdev_id,
>                     is_default ? "disable" : "enable");
>     opts = qemu_opts_parse_noisily(&qemu_filter_opts,
>                                    optarg, false);
>     if (!opts) {
>         error_report("Failed to parse param '%s'", optarg);
>         exit(1);
>     }
>
>     qdict = qemu_opts_to_qdict(opts, NULL);
>     ov = opts_visitor_new(opts);
>     visit_start_struct(opts_get_visitor(ov), &dummy, NULL, NULL, 0,
> &err);
>     if (err) {
>         goto out_clean;
>     }
>     object_add(filter_type, id, qdict, opts_get_visitor(ov), &err);
>     if (err) {
>         goto out_clean;
>     }
>
>     visit_end_struct(opts_get_visitor(ov), &err);
>     if (err) {
>         qmp_object_del(id, NULL);
>         goto out_clean;
>     }
>
> }
>
> Or, we can simplify patch 4 by using qmp_object_add(), codes will be
> like:
>
> void netdev_add_filter(const char *netdev_id,
>                        const char *filter_type,
>                        const char *id,
>                        bool is_default,
>                        Error **errp)
> {
>     ... ...
>
>     qov = qmp_output_visitor_new();
>     ov = qmp_output_get_visitor(qov);
>     visit_start_struct(ov,  &dummy, NULL, NULL, 0, &err);
>     if (err) {
>         goto out;
>     }
>     visit_type_str(ov, &nc->name, "netdev", &err);
>     if (err) {
>         goto out;
>     }
>     status = is_default ? g_strdup("disable") : g_strdup("enable");
>     visit_type_str(ov, &status, "status", &err);
>     g_free(status);
>     if (err) {
>         goto out;
>     }
>     visit_end_struct(ov, &err);
>     if (err) {
>         goto out;
>     }
>     obj = qmp_output_get_qobject(qov);
>     g_assert(obj != NULL);
>     qmp_object_add(filter_type, id, true, obj, &err);
>     qmp_output_visitor_cleanup(qov);
>     qobject_decref(obj);
>
> }
>
> what's your suggestion ? :)
>

Can we just reuse object_create()? here

> Thanks,
> Hailiang