From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37724) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b0kBZ-0004OF-Rq for qemu-devel@nongnu.org; Thu, 12 May 2016 02:33:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b0kBV-0002pu-LF for qemu-devel@nongnu.org; Thu, 12 May 2016 02:33:25 -0400 Received: from [59.151.112.132] (port=13087 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b0kBT-0002oc-5r for qemu-devel@nongnu.org; Thu, 12 May 2016 02:33:21 -0400 References: <1462532187-22787-1-git-send-email-zhangchen.fnst@cn.fujitsu.com> <5732F4FF.5060206@redhat.com> <57331577.2090206@cn.fujitsu.com> <5733D852.9080301@redhat.com> From: Zhang Chen Message-ID: <573423B6.2040602@cn.fujitsu.com> Date: Thu, 12 May 2016 14:33:26 +0800 MIME-Version: 1.0 In-Reply-To: <5733D852.9080301@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: "eddie . dong" , Li Zhijian , "Dr . David Alan Gilbert" On 05/12/2016 09:11 AM, Jason Wang wrote: > > > On 2016年05月11日 19:20, Zhang Chen wrote: >> >> >> 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. > > Looks good. Thanks > >> >>> >>>> --- >>>> 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)); >> } >> > > Kind of, and looks like there's no need for opaque, you can just pass > pointer to SocketReadState. And another parameter of callbacks. OK,will fix it. > >> >>> >>>> 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); >> } >> } > > Yes, but there's no need for opaque, we can infer the container by > container_of(). > But in filter-mirror.c we need do this: void redirector_fill_rsstate_done_cb(SocketReadState *srs, void *opaque) { NetFilterState *nf = opaque; redirector_to_filter(nf, srs->buf, srs->packet_len); } so,I think we have to use void *opaque. >> >>> >>>> } >>>> } >>>> } >>>> @@ -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