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

[Stefan Hajnoczi]

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.

Stefan