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

[Jason Wang <jasowang@redhat.com>]

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.