qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Yang Hongyang
Subject: Re: [Qemu-devel] [PATCH 09/12] netfilter: add a netbuffer filter
Date: Thu, 30 Jul 2015 18:28:23 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0

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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]