Re: [Qemu-devel] [PATCH 09/12] netfilter: add a netbuffer filter

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

On 07/30/2015 05:33 PM, Jason Wang wrote:
> On 07/30/2015 05:04 PM, Yang Hongyang wrote:
>>
>>
>> On 07/30/2015 04:40 PM, Jason Wang wrote:
>>>
>>>
>>> On 07/30/2015 02:47 PM, Yang Hongyang wrote:
>>>> On 07/30/2015 01:13 PM, Jason Wang wrote:
>>>> [...]
>>>>>> +
>>>>>> +#include "net/filter.h"
>>>>>> +#include "net/queue.h"
>>>>>> +#include "filters.h"
>>>>>> +#include "qemu-common.h"
>>>>>> +#include "qemu/error-report.h"
>>>>>> +
>>>>>> +typedef struct FILTERBUFFERState {
>>>>>> +    NetFilterState nf;
>>>>>> +    NetClientState dummy; /* used to send buffered packets */
>>>>>
>>>>> Why need this? Couldn't we just infer this from NetFilterState?
>>>>
>>>> Because we use existing API qemu_send_packet_async/raw to send
>>>> packet, it takes an NetClientState as the first argument sender,
>>>> and use sender->peer->incoming_queue as the dest queue, so in order to
>>>> make this API work, we need to use this dummy NC and init it's
>>>> peer to our dest(which is the network backend)
>>>> Another way is to call qemu_net_queue_send(netdev->incoming_queue,...)
>>>> directly, we still need a NetClientState *sender param, can not
>>>> use NetFilterState.
>>>
>>> I think this is my meaning. Use NetFilterState->netdev.
>>
>> Problem is NetFilterState->netdev is our destination, we need a sender...
>> if we use this, packet will be sent back to NIC...
>>
>
> I see, then NetFilterState->netdev->peer is sender. But I think it's
> better to track sender instead of destination in this case. Something
> like dummy NC is not elegant.
>
>>>
>>>> This dummy NC also been checked in filter_buffer_receive to avoid
>>>> buffering
>>>> packet been sent by ourself.
>>>>
>>>
>>> I don't get why this is needed. Who is going to queue a packet in dummy
>>> NC, consider it was not peered by any others?
>>
>> There's nothing in the dummy NC except the dummy->peer =
>> NetFilterState->netdev
>> This dummy NC only used to as a sender param of the existing APIs
>> which send
>> packets. When a buffered packet been sent, we shouldn't buffer it
>> again, we
>> cann't use any existing NC (packet->sender or NetFilterState->netdev)
>> as the sender because otherwise we can't distinguish if the packet is
>> a buffered
>> packet sent by ourself.
>
> I see, so the reason is you are using qemu_deliver_packet() for both
> enqueuing packet to filter and delivering packet to destination. How
> about something like:
>
> E.g for qemu_send_packet_async(), move the hook before
> qemu_send_packet_async_with_flags(). Then flush method can call
> qemu_send_packet_async_with_flags() without any issue?

I think we can't move the hook earlier, because filters only deal
with the packets will actually been sent. for example, a dump filter.
dump packet that probably won't been sent is wrong. calling
qemu_send_packet_async() or qemu_send_packet_async_with_flags()
doesn't mean the packet is sent, if the sent_cb is not provided and
the other peer is not able to receive, the packet will be dropped.

>
>
>
> .
>

-- 
Thanks,
Yang.