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

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

On 07/30/2015 06:14 PM, Jason Wang wrote:
>
>
> On 07/30/2015 05:49 PM, Yang Hongyang wrote:
>> 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.
>
> It depends on how do you define 'actually been sent' and whether or not
> we should have such accuracy. Packet could be dropped by various layers.
> Reaching receive() or receive_iov() does not mean it can be sent for
> sure. For example, lots of nics drop packet in their receive()
> implementation.

This is true, ok, I'm convinced that we might not need to be this accurate.
but Thomas might have different opinion, I saw this description in his
dump series:

+    /*
+     * Log network traffic into a dump file. Note: This should ideally
+     * be done after calling the ->receive() function below to make sure
+     * that we only log the packets that have really been sent. However,
+     * this does not work right with slirp networking since it immediately
+     * sends reply packets during the receive() function already, so we
+     * would get a wrong order of the packets in the dump file in that case.
+     */

So Thomas, what do you think of this?

> .
>

-- 
Thanks,
Yang.