From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57625) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b0SBq-0003Z4-4M for qemu-devel@nongnu.org; Wed, 11 May 2016 07:20:31 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b0SBl-00058x-Ol for qemu-devel@nongnu.org; Wed, 11 May 2016 07:20:28 -0400 Received: from [59.151.112.132] (port=53964 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b0SBj-00057G-Jd for qemu-devel@nongnu.org; Wed, 11 May 2016 07:20:25 -0400 References: <1462532187-22787-1-git-send-email-zhangchen.fnst@cn.fujitsu.com> <5732F4FF.5060206@redhat.com> From: Zhang Chen Message-ID: <57331577.2090206@cn.fujitsu.com> Date: Wed, 11 May 2016 19:20:23 +0800 MIME-Version: 1.0 In-Reply-To: <5732F4FF.5060206@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH] net/net: Add ReadState 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/11/2016 05:01 PM, Jason Wang wrote: > > > On 2016年05月06日 18:56, Zhang Chen wrote: >> Signed-off-by: Zhang Chen >> Signed-off-by: Li Zhijian >> Signed-off-by: Wen Congyang > > Looks good, just few nits. > > It's better to have a commit log. OK, I will add this log: 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. > >> --- >> include/net/net.h | 8 ++++++ >> net/filter-mirror.c | 60 ++++++++------------------------------------ >> net/net.c | 56 ++++++++++++++++++++++++++++++++++++++++++ >> net/socket.c | 71 >> +++++++++++++---------------------------------------- >> 4 files changed, 91 insertions(+), 104 deletions(-) >> >> diff --git a/include/net/net.h b/include/net/net.h >> index 73e4c46..1439cf9 100644 >> --- a/include/net/net.h >> +++ b/include/net/net.h >> @@ -102,6 +102,14 @@ typedef struct NICState { >> bool peer_deleted; >> } NICState; >> +typedef struct ReadState { >> + int state; /* 0 = getting length, 1 = getting data */ >> + uint32_t index; >> + uint32_t packet_len; >> + uint8_t buf[NET_BUFSIZE]; >> +} ReadState; > > I think SocketReadState is better here. > I will rename it in next version. >> + >> +int net_fill_rstate(ReadState *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, >> diff --git a/net/filter-mirror.c b/net/filter-mirror.c >> index c0c4dc6..bcec509 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]; >> + ReadState rs; >> } MirrorState; >> static int filter_mirror_send(CharDriverState *chr_out, >> @@ -108,51 +105,14 @@ 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) { >> + redirector_to_filter(nf, s->rs.buf, s->rs.packet_len); >> } >> } >> @@ -274,7 +234,7 @@ static void >> filter_redirector_setup(NetFilterState *nf, Error **errp) >> } >> } >> - s->state = s->index = 0; >> + s->rs.state = s->rs.index = 0; >> if (s->indev) { >> s->chr_in = qemu_chr_find(s->indev); >> diff --git a/net/net.c b/net/net.c >> index 0ad6217..926457e 100644 >> --- a/net/net.c >> +++ b/net/net.c >> @@ -1573,3 +1573,59 @@ QemuOptsList qemu_net_opts = { >> { /* end of list */ } >> }, >> }; >> + >> +/* >> + * Returns >> + * 0: readstate is not ready >> + * 1: readstate is ready >> + * otherwise error occurs >> + */ >> +int net_fill_rstate(ReadState *rs, const uint8_t *buf, int size) >> +{ >> + unsigned int l; >> + while (size > 0) { >> + /* reassemble a packet from the network */ >> + switch (rs->state) { /* 0 = getting length, 1 = getting data */ >> + case 0: >> + l = 4 - rs->index; >> + if (l > size) { >> + l = size; >> + } >> + memcpy(rs->buf + rs->index, buf, l); >> + buf += l; >> + size -= l; >> + rs->index += l; >> + if (rs->index == 4) { >> + /* got length */ >> + rs->packet_len = ntohl(*(uint32_t *)rs->buf); >> + rs->index = 0; >> + rs->state = 1; >> + } >> + break; >> + case 1: >> + l = rs->packet_len - rs->index; >> + if (l > size) { >> + l = size; >> + } >> + if (rs->index + l <= sizeof(rs->buf)) { >> + memcpy(rs->buf + rs->index, buf, l); >> + } else { >> + fprintf(stderr, "serious error: oversized packet >> received," >> + "connection terminated.\n"); >> + rs->index = rs->state = 0; >> + return -1; >> + } >> + >> + rs->index += l; >> + buf += l; >> + size -= l; >> + if (rs->index >= rs->packet_len) { >> + rs->index = 0; >> + rs->state = 0; >> + return 1; >> + } >> + break; >> + } >> + } >> + return 0; >> +} >> diff --git a/net/socket.c b/net/socket.c >> index 9fa2cd8..9ecdd3b 100644 >> --- a/net/socket.c >> +++ b/net/socket.c >> @@ -38,11 +38,8 @@ typedef struct NetSocketState { >> NetClientState nc; >> int listen_fd; >> int fd; >> - int state; /* 0 = getting length, 1 = getting data */ >> - unsigned int index; >> - unsigned int packet_len; >> + ReadState rs; >> unsigned int send_index; /* number of bytes sent (only >> SOCK_STREAM) */ >> - uint8_t buf[NET_BUFSIZE]; >> struct sockaddr_in dgram_dst; /* contains inet host and port >> destination iff connectionless (SOCK_DGRAM) */ >> IOHandler *send_fn; /* differs between >> SOCK_STREAM/SOCK_DGRAM */ >> bool read_poll; /* waiting to receive data? */ >> @@ -147,7 +144,7 @@ static void net_socket_send(void *opaque) >> { >> NetSocketState *s = opaque; >> int size; >> - unsigned l; >> + int ret; >> uint8_t buf1[NET_BUFSIZE]; >> const uint8_t *buf; >> @@ -166,60 +163,26 @@ static void net_socket_send(void *opaque) >> closesocket(s->fd); >> s->fd = -1; >> - s->state = 0; >> - s->index = 0; >> - s->packet_len = 0; >> + s->rs.state = 0; >> + s->rs.index = 0; >> + s->rs.packet_len = 0; >> s->nc.link_down = true; >> - memset(s->buf, 0, sizeof(s->buf)); >> + memset(s->rs.buf, 0, sizeof(s->rs.buf)); > > How about introduce a helper to do the initialization? Do you mean remove + s->rs.state = 0; + s->rs.index = 0; + s->rs.packet_len = 0; + memset(s->rs.buf, 0, sizeof(s->rs.buf)); add s->rs->cb = xxx_cb; s->rs->opxx = xxx; void xxx_cb(void *opaque) { NetSocketState *s = opaque; s->rs.state = 0; s->rs.index = 0; s->rs.packet_len = 0; memset(s->rs.buf, 0, sizeof(s->rs.buf)); } > >> memset(s->nc.info_str, 0, sizeof(s->nc.info_str)); >> return; >> } >> buf = buf1; >> - while (size > 0) { >> - /* reassemble a packet from the network */ >> - switch(s->state) { >> - 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 { >> - fprintf(stderr, "serious error: oversized packet >> received," >> - "connection terminated.\n"); >> - s->state = 0; >> - goto eoc; >> - } >> - s->index += l; >> - buf += l; >> - size -= l; >> - if (s->index >= s->packet_len) { >> - s->index = 0; >> - s->state = 0; >> - if (qemu_send_packet_async(&s->nc, s->buf, >> s->packet_len, >> - net_socket_send_completed) == 0) { >> - net_socket_read_poll(s, false); >> - break; >> - } >> - } >> - break; >> + ret = net_fill_rstate(&s->rs, buf, size); >> + >> + if (ret == -1) { >> + goto eoc; >> + } else if (ret == 1) { >> + if (qemu_send_packet_async(&s->nc, s->rs.buf, >> + s->rs.packet_len, >> + net_socket_send_completed) == 0) { >> + net_socket_read_poll(s, false); > > This looks not elegant, maybe we could use callback (which was > initialized by the helper I mention above) to do this. Any thoughts on > this? Do you mean: remove + if (qemu_send_packet_async(&s->nc, s->rs.buf, + s->rs.packet_len, + net_socket_send_completed) == 0) { + net_socket_read_poll(s, false); add s->rs->done void socket_fill_rsstate_done_cb(SocketReadState *srs, void *opaque) { NetSocketState *s = opaque; if (qemu_send_packet_async(&s->nc, srs->buf, srs->packet_len, net_socket_send_completed) == 0) { net_socket_read_poll(s, false); } } > >> } >> } >> } >> @@ -229,7 +192,7 @@ static void net_socket_send_dgram(void *opaque) >> NetSocketState *s = opaque; >> int size; >> - size = qemu_recv(s->fd, s->buf, sizeof(s->buf), 0); >> + size = qemu_recv(s->fd, s->rs.buf, sizeof(s->rs.buf), 0); >> if (size < 0) >> return; >> if (size == 0) { >> @@ -238,7 +201,7 @@ static void net_socket_send_dgram(void *opaque) >> net_socket_write_poll(s, false); >> return; >> } >> - if (qemu_send_packet_async(&s->nc, s->buf, size, >> + if (qemu_send_packet_async(&s->nc, s->rs.buf, size, >> net_socket_send_completed) == 0) { >> net_socket_read_poll(s, false); >> } > > > > . > -- Thanks zhangchen