[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define |
Date: |
Fri, 4 Sep 2015 11:32:55 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Thu, Sep 03, 2015 at 12:18:18AM +0800, Yang Hongyang wrote:
>
>
> On 09/02/2015 09:02 PM, Stefan Hajnoczi wrote:
> >On Wed, Sep 02, 2015 at 09:49:18AM +0800, Yang Hongyang wrote:
> >>On 09/01/2015 10:43 PM, Stefan Hajnoczi wrote:
> >>>On Tue, Sep 01, 2015 at 05:06:18PM +0800, Yang Hongyang wrote:
> >>>>This will be used by the next patch in this series.
> >>>>
> >>>>Signed-off-by: Yang Hongyang <address@hidden>
> >>>>Reviewed-by: Thomas Huth <address@hidden>
> >>>>---
> >>>> include/net/queue.h | 19 +++++++++++++++++++
> >>>> net/queue.c | 19 -------------------
> >>>> 2 files changed, 19 insertions(+), 19 deletions(-)
> >>>>
> >>>>diff --git a/include/net/queue.h b/include/net/queue.h
> >>>>index fc02b33..1d65e47 100644
> >>>>--- a/include/net/queue.h
> >>>>+++ b/include/net/queue.h
> >>>>@@ -31,6 +31,25 @@ typedef struct NetQueue NetQueue;
> >>>>
> >>>> typedef void (NetPacketSent) (NetClientState *sender, ssize_t ret);
> >>>>
> >>>>+struct NetPacket {
> >>>>+ QTAILQ_ENTRY(NetPacket) entry;
> >>>>+ NetClientState *sender;
> >>>>+ unsigned flags;
> >>>>+ int size;
> >>>>+ NetPacketSent *sent_cb;
> >>>>+ uint8_t data[0];
> >>>>+};
> >>>>+
> >>>>+struct NetQueue {
> >>>>+ void *opaque;
> >>>>+ uint32_t nq_maxlen;
> >>>>+ uint32_t nq_count;
> >>>>+
> >>>>+ QTAILQ_HEAD(packets, NetPacket) packets;
> >>>>+
> >>>>+ unsigned delivering:1;
> >>>>+};
> >>>>+
> >>>
> >>>Why is it necessary to expose both structs?
> >>
> >>In next patch, we add a API to pass the packet to next filter:
> >>ssize_t qemu_netfilter_pass_to_next(NetFilterState *nf, NetPacket *packet);
> >>which will access the internals of NetPacket.
> >>
> >>and in buffer filter, we need the NetQueue to Queue packets, and also to
> >>access queue->packets(pass this packets to next filter).
> >
> >Can you do these things by introducing net queue APIs instead of
> >exposing the struct?
> >
> >It's a C anti-pattern to expose structs. It makes code complex because
> >now the net queue code no longer contains all the assumptions about
> >NetQueue/NetPacket fields. People modifying the code now also need to
> >go into the net filter code to figure out how this struct works.
> >
> >Exposing the fields also leads to code duplication since every user will
> >reimplement similar stuff if they access the fields directly instead of
> >using a function interface.
>
> I can't think of a interface that no need to access the NetPacket internals
> to archive the needs. Except I do not reuse the NetPacket and NetQueue,
> implement a Queue myself. But that will be more complex.
> So in order to not expose those structs, I shall add lots of public get
> functions to use these internals, like:
> Qemu_NetQueue_get_xxx
> Qemu_NetPacket_get_xxx
>
> I'd be very happy if you could give me a better suggestion on this.
net/queue.c has logic to send/queue/flush packets but a
qemu_deliver_packet() call is hardcoded.
Maybe you can extend qemu_new_net_queue() like this:
/* Returns:
* >0 - success
* 0 - queue packet for future redelivery
* <0 - failure (discard packet)
*/
typedef ssize_t NetQueueDeliverFunc(NetClientState *sender,
unsigned flags,
const struct iovec *iov,
int iovcnt,
void *opaque);
NetQueue *qemu_new_net_queue(NetQueueDeliverFunc deliver,
void *opaque);
Now net/net.c:qemu_net_client_setup() needs to call:
nc->incoming_queue = qemu_new_net_queue(qemu_deliver_packet_iov, nc);
And the filter code can use qemu_net_queue_send_iov() and
qemu_net_queue_flush(). The filter just needs to provide its own
NetQueueDeliveryFunc.
I haven't checked the details (e.g. non-iov delivery, etc) but the idea
is to use the net/queue.c API instead of duplicating similar logic in
the filter code.
- [Qemu-devel] [PATCH v9 02/10] init/cleanup of netfilter object, (continued)
- [Qemu-devel] [PATCH v9 02/10] init/cleanup of netfilter object, Yang Hongyang, 2015/09/01
- [Qemu-devel] [PATCH v9 08/10] net/queue: export qemu_net_queue_append_iov, Yang Hongyang, 2015/09/01
- [Qemu-devel] [PATCH v9 03/10] netfilter: add netfilter_{add|del} commands, Yang Hongyang, 2015/09/01
- [Qemu-devel] [PATCH v9 09/10] netfilter: add a netbuffer filter, Yang Hongyang, 2015/09/01
- [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Yang Hongyang, 2015/09/01
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Stefan Hajnoczi, 2015/09/01
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Yang Hongyang, 2015/09/01
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Stefan Hajnoczi, 2015/09/02
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Yang Hongyang, 2015/09/02
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define,
Stefan Hajnoczi <=
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Yang Hongyang, 2015/09/07
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Markus Armbruster, 2015/09/07
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Yang Hongyang, 2015/09/07
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Stefan Hajnoczi, 2015/09/07
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Yang Hongyang, 2015/09/07
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Yang Hongyang, 2015/09/07
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Daniel P. Berrange, 2015/09/07
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Yang Hongyang, 2015/09/07
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Daniel P. Berrange, 2015/09/07
- Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define, Yang Hongyang, 2015/09/07