Re: [Qemu-devel] [PATCH] RFC/net: Add a net filter

[Yang Hongyang <yanghy@cn.fujitsu.com>]

On 07/28/2015 12:00 PM, Yang Hongyang wrote:
> On 07/28/2015 11:28 AM, Jason Wang wrote:
>> On 07/27/2015 06:03 PM, Yang Hongyang wrote:
>>> On 07/27/2015 05:16 PM, Jason Wang wrote:
>>> [...]
>>>>>>>>> I think this won't work for the buffer case? If we want the buffer
>>>>>>>>> case
>>>>>>>>> to work under this, we should modify the generic netdev layer
>>>>>>>>> code, to
>>>>>>>>> check the return value of the filter function call.
>>>>>>>>
>>>>>>>> But checking return value is rather simpler than a new netdev type,
>>>>>>>> isn't it?
>>>>>>>
>>>>>>> But how to implement a plugin which suppose to do the actual work on
>>>>>>> the packets?
>>>>>>
>>>>>> Well, the filter get the packets, so it can do everything it wants.
>>>>>>
>>>>>>> how to configure params related to the plugin? different
>>>>>>> plugins may need different params, implement as another netdev?
>>>>>>
>>>>>> I belive qmp can do this? something like -filter dump,id=f0,len=10000?
>>>>>
>>>>> So you mean implement another object filter?
>>>>
>>>> Yes.
>>>>
>>>>> and the structure is like netdev?
>>>>
>>>> No, it is embedded in netdev.
>>>
>>> Ah, I see what you mean, thank you for the patience...
>>> does the command line looks like:
>>> -filter dump,id=f0,len=10000
>>> -netdev tap,XXX,filter=dump
>>>
>>> If I need both dump and packets buffering, how will the qmp be?
>>> -filter dump,id=f0,len=10000
>>> -filter buffer,XXX
>>> -netdev tap,XXX,filter=dump:buffer:XXX ?
>>
>> This is ok but we have several choices, e.g you may want to have a next
>> field like:
>>
>> - filter buffer,id=f0
>> - filter dump,id=f1,len=1000,next=f0
>> - netdev tap,XXX,filter_root=f1
>
> This seems better, thank you!

Sorry, when try to implement it, I found this qmp is not that good when it
comes to dynamically add/remove filters, can I use below instead:
-netdev tap,id=bn0
-netfilter dump,id=f0,netdev=bn0
-netfilter buffer,id=f1,netdev=bn0

by this, I can introduce a QTAILQ of filters to NetClentState and
netdev_{add|del}_filter to dynamically add/remove filters.

>
>>
>>> or
>>> -netdev tap,id=bn0,XXX
>>> -filter dump,id=f0,len=10000,netdev=bn0
>>> -filter buffer,XXX,netdev=bn0 ?
>>>
>>> And do you care if we add a filter list to NetClientInfo?
>>
>> I don't care, and in fact this also shows great advantages over the
>> proposal of new netdevs. In that case, if you want two filters, two
>> netdevs is needed and I can't image how to handle offloads or link
>> status in this case.
>>
>>> and modify the generic layer to deal with these filters?
>>> E.g, init/cleanup filters, go through these filters, check
>>> for return values, stop call peer's receiving.
>>
>> I think it's ok to do these. What we really need is an new layer before
>> peer's receiving not new kinds of netdevs.
>>
>>> This is our main concern at the beginning, we want to avoid
>>> modify the netdev generic layer too much, and do all things
>>> within the filter dev so that this could be a bolt-on
>>> feature. But if you don't care about this, we could simply
>>> implement it as you said.
>>
>> I don't think much will be modified. Maybe just add callbacks on
>> receive, initialization and cleanup. Most of the codes should be limited
>> to filter itself. The point is 'filter' is not a kind of device or
>> backend, so most of the fields will be unnecessary. Treating it as
>> pseudo netdev will bring burdens too. E.g: dealing with vnet headers,
>> offloads, be/le setting and link status and probably even more.
>
> Thank you for the explanation. will do as you said.
>
>>>
>>>>
>>>>> That will duplicate some of the netdev layer code.
>>>>
>>>> Not at all, it only cares about how to deal with the packet.
>>>>
>>>>> Implement it as
>>>>> a netdev can reuse the existing netdev design. And current dump is
>>>>> implemented
>>>>> as a netdev right?
>>>>
>>>> Right but it only works for hub, and that's why Thomas wrote his series
>>>> to make it work for all other backends
>>>>
>>>>> even if we simply passing the packets to the filter function before
>>>>> calling nc->info->receive{_raw}(), we might also need to implement as
>>>>> a netdev as dump dose.
>>>>
>>>> Why? The reason why we still keep -netdev dump is for backward
>>>> compatibility. If we only care about using it for new netdevs, we can
>>>> get rid of all netdev stuffs from dump.
>>>
>>> Thank you, I see the points.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>> And it is not as
>>>>>>>>> extensible as we abstract the filter function to a netdev, We can
>>>>>>>>> flexibly add/remove/change filter plugins on the fly.
>>>>>>>>
>>>>>>>> I don't see why we lose the flexibility like what I suggested.
>>>>>>>> Actually,
>>>>>>>> implement it through a netdev will complex this. E.g:
>>>>>>>>
>>>>>>>> -netdev tap,id=bn0  # you can use whatever backend as needed
>>>>>>>> -netdev filter,id=f0,backend=bn0,plugin=dump
>>>>>>>> -device e1000,netdev=f0
>>>>>>>>
>>>>>>>> How did you remove filter id=f0? Looks like you need also remove
>>>>>>>> e1000 nic?
>>>>>>>
>>>>>>> No, when remove filter, we restore the connection between network
>>>>>>> backend and
>>>>>>> NIC. Just like filter does not ever exists.
>>>>>>
>>>>>> But e1000's peer is f0. You mean you will modify the peer pointer
>>>>>> during
>>>>>> filter removing?
>>>>>
>>>>> Yes.
>>>>>
>>>>>> Sounds scary.
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> .
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>> .
>>>>>>
>>>>>
>>>>
>>>> .
>>>>
>>>
>>
>> .
>>
>

-- 
Thanks,
Yang.