From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54643) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXAkZ-00014d-J0 for qemu-devel@nongnu.org; Wed, 02 Sep 2015 12:19:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZXAkW-0002I2-CW for qemu-devel@nongnu.org; Wed, 02 Sep 2015 12:19:03 -0400 Received: from [59.151.112.132] (port=28180 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZXAkK-0002Bm-V6 for qemu-devel@nongnu.org; Wed, 02 Sep 2015 12:19:00 -0400 Message-ID: <55E7214A.20803@cn.fujitsu.com> Date: Thu, 3 Sep 2015 00:18:18 +0800 From: Yang Hongyang MIME-Version: 1.0 References: <1441098383-22585-1-git-send-email-yanghy@cn.fujitsu.com> <1441098383-22585-6-git-send-email-yanghy@cn.fujitsu.com> <20150901144308.GE2407@stefanha-thinkpad.redhat.com> <55E6559E.5060305@cn.fujitsu.com> <20150902130245.GI17873@stefanha-thinkpad.redhat.com> In-Reply-To: <20150902130245.GI17873@stefanha-thinkpad.redhat.com> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v9 05/10] move out net queue structs define List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: thuth@redhat.com, zhang.zhanghailiang@huawei.com, lizhijian@cn.fujitsu.com, jasowang@redhat.com, qemu-devel@nongnu.org, mrhines@linux.vnet.ibm.com, armbru@redhat.com 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 >>>> Reviewed-by: Thomas Huth >>>> --- >>>> 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. Thanks! > > Stefan > . > -- Thanks, Yang.