From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43092) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b18IR-0000Ks-D2 for qemu-devel@nongnu.org; Fri, 13 May 2016 04:18:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b18IO-00048M-1o for qemu-devel@nongnu.org; Fri, 13 May 2016 04:18:07 -0400 Received: from [59.151.112.132] (port=50498 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b18IJ-00041C-TH for qemu-devel@nongnu.org; Fri, 13 May 2016 04:18:03 -0400 References: <1463044750-10424-1-git-send-email-zhangchen.fnst@cn.fujitsu.com> <573551BA.4030809@redhat.com> From: Zhang Chen Message-ID: <57356FBE.4040808@cn.fujitsu.com> Date: Fri, 13 May 2016 14:10:06 +0800 MIME-Version: 1.0 In-Reply-To: <573551BA.4030809@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH V2] net/net: Add SocketReadState for reuse codes List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang , qemu devel Cc: Li Zhijian , Wen Congyang , "eddie . dong" , "Dr . David Alan Gilbert" On 05/13/2016 12:02 PM, Jason Wang wrote: > > > On 2016年05月12日 17:19, Zhang Chen wrote: >> This function is from net/socket.c, move it to net.c and net.h. >> Add SocketReadState to make others reuse net_fill_rstate(). >> suggestion from jason. >> >> v2: >> - rename ReadState to SocketReadState >> - add SocketReadState init and finalize callback >> >> v1: >> - init patch >> >> Signed-off-by: Zhang Chen >> Signed-off-by: Li Zhijian >> Signed-off-by: Wen Congyang >> --- >> include/net/net.h | 14 +++++++++ >> net/filter-mirror.c | 72 ++++++++++++++----------------------------- >> net/net.c | 65 ++++++++++++++++++++++++++++++++++++++ >> net/socket.c | 89 >> +++++++++++++++++++++-------------------------------- >> 4 files changed, 137 insertions(+), 103 deletions(-) >> >> diff --git a/include/net/net.h b/include/net/net.h >> index 73e4c46..4f6b6bf 100644 >> --- a/include/net/net.h >> +++ b/include/net/net.h >> @@ -57,6 +57,9 @@ typedef void (SetOffload)(NetClientState *, int, >> int, int, int, int); >> typedef void (SetVnetHdrLen)(NetClientState *, int); >> typedef int (SetVnetLE)(NetClientState *, bool); >> typedef int (SetVnetBE)(NetClientState *, bool); >> +typedef struct SocketReadState SocketReadState; >> +typedef void (SocketReadStateInit)(SocketReadState *rs); >> +typedef void (SocketReadStateFinalize)(SocketReadState *rs); >> typedef struct NetClientInfo { >> NetClientOptionsKind type; >> @@ -102,6 +105,16 @@ typedef struct NICState { >> bool peer_deleted; >> } NICState; >> +struct SocketReadState { >> + int state; /* 0 = getting length, 1 = getting data */ >> + uint32_t index; >> + uint32_t packet_len; >> + uint8_t buf[NET_BUFSIZE]; >> + SocketReadStateInit *init; >> + SocketReadStateFinalize *finalize; >> +}; >> + >> +int net_fill_rstate(SocketReadState *rs, const uint8_t *buf, int size); >> char *qemu_mac_strdup_printf(const uint8_t *macaddr); >> NetClientState *qemu_find_netdev(const char *id); >> int qemu_find_net_clients_except(const char *id, NetClientState **ncs, >> @@ -160,6 +173,7 @@ ssize_t qemu_deliver_packet_iov(NetClientState >> *sender, >> void print_net_client(Monitor *mon, NetClientState *nc); >> void hmp_info_network(Monitor *mon, const QDict *qdict); >> +void net_socket_rs_init(SocketReadState *rs); >> /* NIC info */ >> diff --git a/net/filter-mirror.c b/net/filter-mirror.c >> index c0c4dc6..4e55924 100644 >> --- a/net/filter-mirror.c >> +++ b/net/filter-mirror.c >> @@ -40,10 +40,7 @@ typedef struct MirrorState { >> char *outdev; >> CharDriverState *chr_in; >> CharDriverState *chr_out; >> - int state; /* 0 = getting length, 1 = getting data */ >> - unsigned int index; >> - unsigned int packet_len; >> - uint8_t buf[REDIRECTOR_MAX_LEN]; >> + SocketReadState rs; >> } MirrorState; >> static int filter_mirror_send(CharDriverState *chr_out, >> @@ -108,50 +105,15 @@ static void redirector_chr_read(void *opaque, >> const uint8_t *buf, int size) >> { >> NetFilterState *nf = opaque; >> MirrorState *s = FILTER_REDIRECTOR(nf); >> - unsigned int l; >> - >> - while (size > 0) { >> - /* reassemble a packet from the network */ >> - switch (s->state) { /* 0 = getting length, 1 = getting data */ >> - case 0: >> - l = 4 - s->index; >> - if (l > size) { >> - l = size; >> - } >> - memcpy(s->buf + s->index, buf, l); >> - buf += l; >> - size -= l; >> - s->index += l; >> - if (s->index == 4) { >> - /* got length */ >> - s->packet_len = ntohl(*(uint32_t *)s->buf); >> - s->index = 0; >> - s->state = 1; >> - } >> - break; >> - case 1: >> - l = s->packet_len - s->index; >> - if (l > size) { >> - l = size; >> - } >> - if (s->index + l <= sizeof(s->buf)) { >> - memcpy(s->buf + s->index, buf, l); >> - } else { >> - error_report("serious error: oversized packet >> received."); >> - s->index = s->state = 0; >> - qemu_chr_add_handlers(s->chr_in, NULL, NULL, NULL, >> NULL); >> - return; >> - } >> - >> - s->index += l; >> - buf += l; >> - size -= l; >> - if (s->index >= s->packet_len) { >> - s->index = 0; >> - s->state = 0; >> - redirector_to_filter(nf, s->buf, s->packet_len); >> - } >> - break; >> + int ret; >> + >> + ret = net_fill_rstate(&s->rs, buf, size); >> + >> + if (ret == -1) { >> + qemu_chr_add_handlers(s->chr_in, NULL, NULL, NULL, NULL); >> + } else if (ret == 1) { >> + if (s->rs.finalize) { >> + s->rs.finalize(&s->rs); > > Why not simply call this in net_fill_rstate()? Good idea. I will move it in next version. > >> } >> } >> } >> @@ -258,6 +220,14 @@ static void filter_mirror_setup(NetFilterState >> *nf, Error **errp) >> } >> } >> +static void redirector_rs_finalize(SocketReadState *rs) >> +{ >> + MirrorState *s = container_of(rs, MirrorState, rs); >> + NetFilterState *nf = NETFILTER(s); >> + >> + redirector_to_filter(nf, rs->buf, rs->packet_len); >> +} >> + >> static void filter_redirector_setup(NetFilterState *nf, Error **errp) >> { >> MirrorState *s = FILTER_REDIRECTOR(nf); >> @@ -274,7 +244,11 @@ static void >> filter_redirector_setup(NetFilterState *nf, Error **errp) >> } >> } >> - s->state = s->index = 0; >> + s->rs.init = net_socket_rs_init; >> + s->rs.finalize = redirector_rs_finalize; >> + if (s->rs.init) { >> + s->rs.init(&s->rs); >> + } > > This looks odd, you can call net_socket_rs_init() directly here and > pass redirector_rs_finalize to it. Then there's no need to do the open > code here. > > OK Thanks Zhang Chen > > . > -- Thanks zhangchen