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 <yan...@cn.fujitsu.com> > >>>>Reviewed-by: Thomas Huth <th...@redhat.com> > >>>>--- > >>>> 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.