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

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

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!

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