Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define

[Stefan Hajnoczi <stefanha@redhat.com>]

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 <yanghy@cn.fujitsu.com>
> >>>>Reviewed-by: Thomas Huth <thuth@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.