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

[Jason Wang <jasowang@redhat.com>]

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

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