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

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

On 2016/1/27 13:59, Jason Wang wrote:
>
>
> 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 <zhang.zhanghailiang@huawei.com>
>>>>>> ---
>>>>>>     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
>

Yes, the codes is more clean if we reuse it.
I will fix it like that in v2, thanks.

>> Thanks,
>> Hailiang
>
>
> .
>