On 01/20/2016 03:14 PM, Hailiang Zhang wrote:
> On 2016/1/20 10:39, Jason Wang wrote:
>>
>>
>> On 01/19/2016 04:39 PM, Hailiang Zhang wrote:
>>> Hi Jason,
>>>
>>> Thanks for your review.
>>>
>>> On 2016/1/19 11:19, Jason Wang wrote:
>>>>
>>>>
>>>> On 12/29/2015 03:09 PM, zhanghailiang wrote:
>>>>> We add each netdev (except vhost-net) a default filter-buffer,
>>>>> which will be used for COLO or Micro-checkpoint to buffer VM's
>>>>> packets.
>>>>> The name of default filter-buffer is 'nop'.
>>>>> For the default filter-buffer, it will not buffer any packets in
>>>>> default.
>>>>> So it has no side effect for the netdev.
>>>>>
>>>>> Signed-off-by: zhanghailiang <zhang.zhanghaili...@huawei.com>
>>>>> Cc: Jason Wang <jasow...@redhat.com>
>>>>> Cc: Yang Hongyang <hongyang.y...@easystack.cn>
>>>>
>>>> This patch did three things:
>>>>
>>>> 1) the ability to enable or disable a netfilter
>>>> 2) the ability to add a default filter
>>>> 3) default filter attaching for filter-buffer
>>>>
>>>> Better to split them into separate small patches.
>>>>
>>>> And several questions:
>>>>
>>>> For 1), I'm not sure this is real needed, we can in fact disable a
>>>> filter by removing it.
>>>
>>> If we do like this, do we also need to _enable_ the buffer filter by
>>> add it dynamically instead of attaching the default filter ?
>>> Just like what we do in V10 ?
>>> (In that series, you think have a default filter may be better.
>>> The main reason for that is to support
>>> hot-add nic. Since we didn't support hot-add nic during COLO,
>>> it will be OK to add default filter dynamically)
>>
>> Aha, right. So rethinking of this, enabling/disabling a filter looks ok
>> to me.
>>
>>>
>>>> For 2), Instead of a specific code just for filter buffer, I think we
>>>> need a generic method for an arbitrary filter to be used as default.
>>>
>>> Good idea.
>>>
>>>> And if we can achieve 2), 3) is not needed any more.
>>>>
>>>>> ---
>>>>> v12:
>>>>> - Skip vhost-net when add default filter
>>>>> - Don't go through filter layer if the filter is disabled.
>>>>> v11:
>>>>> - New patch
>>>>> ---
>>>>> include/net/filter.h | 10 +++++++
>>>>> net/filter-buffer.c | 82
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>>> net/filter.c | 6 +++-
>>>>> net/net.c | 12 ++++++++
>>>>> 4 files changed, 109 insertions(+), 1 deletion(-)
>>>>>
[...]
>>>>> +/*
>>>>> +* This will be used by COLO or MC FT, for which they will need
>>>>> +* to buffer the packets of VM's net devices, Here we add a default
>>>>> +* buffer filter for each netdev. The name of default buffer
>>>>> filter is
>>>>> +* 'nop'
>>>>> +*/
>>>>> +void netdev_add_default_filter_buffer(const char *netdev_id,
>>>>> + NetFilterDirection direction,
>>>>> + Error **errp)
>>>>> +{
>>>>
>>>> Need a more generic way to add an arbitrary filter as default. E.g
>>>> during netdev init, query if there's a default and do the
>>>> initialization
>>>> there.
>>>>
>>>
>>> We call it in net_client_init1(), i don't find a better place to
>>> call it,
>>> what's your suggestion ?
>>
>> net_client_init1() is ok, just need a generic way. E.g a string which
>> stores the default filter and its parameters which could be queried by
>> default filter initialization function.
>>
>
> Hmm, i'm still confused about how to realize this, do you mean change
> this helper
> to a more generic function ? Just like :
> void netdev_add_filter(const char *netdev_id,
> const char *filter_type, char *id,
> Error **errp)
>
> Could you please give me more detail about how to realize it ? :)
I mean you need to know the type of default filter before calling
netdev_add_filter(). May need a global pointer for this, and colo may
set this during initialization. And in the future, we may allow this to
be set from cli.
>
>>>
>>>>> + QmpOutputVisitor *qov;
>>>>> + QmpInputVisitor *qiv;
>>>>> + Visitor *ov, *iv;
>>>>> + QObject *obj = NULL;
>>>>> + QDict *qdict;
>>>>> + void *dummy = NULL;
>>>>> + const char *id = "nop";
>>>>> + char *queue = g_strdup(NetFilterDirection_lookup[direction]);
>>>>> + NetClientState *nc = qemu_find_netdev(netdev_id);
>>>>> + Error *err = NULL;
>>>>> +
>>>>> + /* FIXME: Not support multiple queues */
>>>>> + if (!nc || nc->queue_index > 1) {
>>>>> + g_free(queue);
>>>>> + return;
>>>>> + }
>>>>> + /* Not support vhost-net */
>>>>> + if (get_vhost_net(nc)) {
>>>>> + g_free(queue);
>>>>> + return;
>>>>> + }
>>>>> + 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;
>>>>> + }
>>>>> + visit_type_str(ov, &queue, "queue", &err);
>>>>> + if (err) {
>>>>> + goto out;
>>>>> + }
>>>>> + visit_end_struct(ov, &err);
>>>>> + if (err) {
>>>>> + goto out;
>>>>> + }
>>>>> + obj = qmp_output_get_qobject(qov);
>>>>> + g_assert(obj != NULL);
>>>>> + qdict = qobject_to_qdict(obj);
>>>>> + qmp_output_visitor_cleanup(qov);
>>>>> +
>>>>> + qiv = qmp_input_visitor_new(obj);
>>>>> + iv = qmp_input_get_visitor(qiv);
>>>>> + object_add(TYPE_FILTER_BUFFER, id, qdict, iv, &err);
>>>>> + qmp_input_visitor_cleanup(qiv);
>>>>> + qobject_decref(obj);
>>>>> +out:
>>>>> + g_free(queue);
>>>>> + if (err) {
>>>>> + error_propagate(errp, err);
>>>>> + }
>>>>> +}
>>>>> +
>>>>> static void filter_buffer_init(Object *obj)
>>>>> {
>>>>> object_property_add(obj, "interval", "int",
>>>>> diff --git a/net/filter.c b/net/filter.c
>>>>> index 1365bad..0b1e408 100644
>>>>> --- a/net/filter.c
>>>>> +++ b/net/filter.c
>>>>> @@ -163,7 +163,8 @@ static void netfilter_complete(UserCreatable
>>>>> *uc, Error **errp)
>>>>> }
>>>>>
>>>>> nf->netdev = ncs[0];
>>>>> -
>>>>> + nf->is_default = false;
>>>>> + nf->enabled = true;
>>>>
>>>> If we really need something like enabled, need more code for user to
>>>> control this bit.
>>>>
>>>
>>> OK, i will fix it.
>>
>> Or if you want to minimize the change set. You can make something like
>> enabled locally to filter buffer, and let it to be used by colo only.
>>
>
> I have convert it to use object property to set/get the value, is that
> OK ?
>
> Thanks,
> Hailiang
It's ok.
