From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52726) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b0fAT-0005hF-RT for qemu-devel@nongnu.org; Wed, 11 May 2016 21:11:59 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b0fAO-0002BT-LA for qemu-devel@nongnu.org; Wed, 11 May 2016 21:11:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46496) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b0fAO-0002BP-CH for qemu-devel@nongnu.org; Wed, 11 May 2016 21:11:52 -0400 References: <1462532187-22787-1-git-send-email-zhangchen.fnst@cn.fujitsu.com> <5732F4FF.5060206@redhat.com> <57331577.2090206@cn.fujitsu.com> From: Jason Wang Message-ID: <5733D852.9080301@redhat.com> Date: Thu, 12 May 2016 09:11:46 +0800 MIME-Version: 1.0 In-Reply-To: <57331577.2090206@cn.fujitsu.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable 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: Zhang Chen , qemu devel Cc: "eddie . dong" , Li Zhijian , "Dr . David Alan Gilbert" On 2016=E5=B9=B405=E6=9C=8811=E6=97=A5 19:20, Zhang Chen wrote: > > > On 05/11/2016 05:01 PM, Jason Wang wrote: >> >> >> On 2016=E5=B9=B405=E6=9C=8806=E6=97=A5 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=20 >>> +++++++++++++---------------------------------------- >>> 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 =3D getting length, 1 =3D 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=20 >>> **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 =3D getting length, 1 =3D 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,=20 >>> const uint8_t *buf, int size) >>> { >>> NetFilterState *nf =3D opaque; >>> MirrorState *s =3D FILTER_REDIRECTOR(nf); >>> - unsigned int l; >>> - >>> - while (size > 0) { >>> - /* reassemble a packet from the network */ >>> - switch (s->state) { /* 0 =3D getting length, 1 =3D getting d= ata */ >>> - case 0: >>> - l =3D 4 - s->index; >>> - if (l > size) { >>> - l =3D size; >>> - } >>> - memcpy(s->buf + s->index, buf, l); >>> - buf +=3D l; >>> - size -=3D l; >>> - s->index +=3D l; >>> - if (s->index =3D=3D 4) { >>> - /* got length */ >>> - s->packet_len =3D ntohl(*(uint32_t *)s->buf); >>> - s->index =3D 0; >>> - s->state =3D 1; >>> - } >>> - break; >>> - case 1: >>> - l =3D s->packet_len - s->index; >>> - if (l > size) { >>> - l =3D size; >>> - } >>> - if (s->index + l <=3D sizeof(s->buf)) { >>> - memcpy(s->buf + s->index, buf, l); >>> - } else { >>> - error_report("serious error: oversized packet=20 >>> received."); >>> - s->index =3D s->state =3D 0; >>> - qemu_chr_add_handlers(s->chr_in, NULL, NULL, NULL,=20 >>> NULL); >>> - return; >>> - } >>> - >>> - s->index +=3D l; >>> - buf +=3D l; >>> - size -=3D l; >>> - if (s->index >=3D s->packet_len) { >>> - s->index =3D 0; >>> - s->state =3D 0; >>> - redirector_to_filter(nf, s->buf, s->packet_len); >>> - } >>> - break; >>> - } >>> + int ret; >>> + >>> + ret =3D net_fill_rstate(&s->rs, buf, size); >>> + >>> + if (ret =3D=3D -1) { >>> + qemu_chr_add_handlers(s->chr_in, NULL, NULL, NULL, NULL); >>> + } else if (ret =3D=3D 1) { >>> + redirector_to_filter(nf, s->rs.buf, s->rs.packet_len); >>> } >>> } >>> @@ -274,7 +234,7 @@ static void=20 >>> filter_redirector_setup(NetFilterState *nf, Error **errp) >>> } >>> } >>> - s->state =3D s->index =3D 0; >>> + s->rs.state =3D s->rs.index =3D 0; >>> if (s->indev) { >>> s->chr_in =3D 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 =3D { >>> { /* 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 =3D getting length, 1 =3D getting=20 >>> data */ >>> + case 0: >>> + l =3D 4 - rs->index; >>> + if (l > size) { >>> + l =3D size; >>> + } >>> + memcpy(rs->buf + rs->index, buf, l); >>> + buf +=3D l; >>> + size -=3D l; >>> + rs->index +=3D l; >>> + if (rs->index =3D=3D 4) { >>> + /* got length */ >>> + rs->packet_len =3D ntohl(*(uint32_t *)rs->buf); >>> + rs->index =3D 0; >>> + rs->state =3D 1; >>> + } >>> + break; >>> + case 1: >>> + l =3D rs->packet_len - rs->index; >>> + if (l > size) { >>> + l =3D size; >>> + } >>> + if (rs->index + l <=3D sizeof(rs->buf)) { >>> + memcpy(rs->buf + rs->index, buf, l); >>> + } else { >>> + fprintf(stderr, "serious error: oversized packet=20 >>> received," >>> + "connection terminated.\n"); >>> + rs->index =3D rs->state =3D 0; >>> + return -1; >>> + } >>> + >>> + rs->index +=3D l; >>> + buf +=3D l; >>> + size -=3D l; >>> + if (rs->index >=3D rs->packet_len) { >>> + rs->index =3D 0; >>> + rs->state =3D 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 =3D getting length, 1 =3D getting data */ >>> - unsigned int index; >>> - unsigned int packet_len; >>> + ReadState rs; >>> unsigned int send_index; /* number of bytes sent (only=20 >>> SOCK_STREAM) */ >>> - uint8_t buf[NET_BUFSIZE]; >>> struct sockaddr_in dgram_dst; /* contains inet host and port=20 >>> destination iff connectionless (SOCK_DGRAM) */ >>> IOHandler *send_fn; /* differs between=20 >>> SOCK_STREAM/SOCK_DGRAM */ >>> bool read_poll; /* waiting to receive data? */ >>> @@ -147,7 +144,7 @@ static void net_socket_send(void *opaque) >>> { >>> NetSocketState *s =3D 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 =3D -1; >>> - s->state =3D 0; >>> - s->index =3D 0; >>> - s->packet_len =3D 0; >>> + s->rs.state =3D 0; >>> + s->rs.index =3D 0; >>> + s->rs.packet_len =3D 0; >>> s->nc.link_down =3D 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 =3D 0; > + s->rs.index =3D 0; > + s->rs.packet_len =3D 0; > + memset(s->rs.buf, 0, sizeof(s->rs.buf)); > > add > s->rs->cb =3D xxx_cb; > s->rs->opxx =3D xxx; > > void xxx_cb(void *opaque) > { > NetSocketState *s =3D opaque; > > s->rs.state =3D 0; > s->rs.index =3D 0; > s->rs.packet_len =3D 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=20 pointer to SocketReadState. And another parameter of callbacks. > >> >>> memset(s->nc.info_str, 0, sizeof(s->nc.info_str)); >>> return; >>> } >>> buf =3D buf1; >>> - while (size > 0) { >>> - /* reassemble a packet from the network */ >>> - switch(s->state) { >>> - case 0: >>> - l =3D 4 - s->index; >>> - if (l > size) >>> - l =3D size; >>> - memcpy(s->buf + s->index, buf, l); >>> - buf +=3D l; >>> - size -=3D l; >>> - s->index +=3D l; >>> - if (s->index =3D=3D 4) { >>> - /* got length */ >>> - s->packet_len =3D ntohl(*(uint32_t *)s->buf); >>> - s->index =3D 0; >>> - s->state =3D 1; >>> - } >>> - break; >>> - case 1: >>> - l =3D s->packet_len - s->index; >>> - if (l > size) >>> - l =3D size; >>> - if (s->index + l <=3D sizeof(s->buf)) { >>> - memcpy(s->buf + s->index, buf, l); >>> - } else { >>> - fprintf(stderr, "serious error: oversized packet=20 >>> received," >>> - "connection terminated.\n"); >>> - s->state =3D 0; >>> - goto eoc; >>> - } >>> - s->index +=3D l; >>> - buf +=3D l; >>> - size -=3D l; >>> - if (s->index >=3D s->packet_len) { >>> - s->index =3D 0; >>> - s->state =3D 0; >>> - if (qemu_send_packet_async(&s->nc, s->buf,=20 >>> s->packet_len, >>> - net_socket_send_completed) =3D=3D 0) { >>> - net_socket_read_poll(s, false); >>> - break; >>> - } >>> - } >>> - break; >>> + ret =3D net_fill_rstate(&s->rs, buf, size); >>> + >>> + if (ret =3D=3D -1) { >>> + goto eoc; >>> + } else if (ret =3D=3D 1) { >>> + if (qemu_send_packet_async(&s->nc, s->rs.buf, >>> + s->rs.packet_len, >>> + net_socket_send_completed) =3D=3D= 0) { >>> + net_socket_read_poll(s, false); >> >> This looks not elegant, maybe we could use callback (which was=20 >> initialized by the helper I mention above) to do this. Any thoughts=20 >> on this? > > Do you mean: > > remove > + if (qemu_send_packet_async(&s->nc, s->rs.buf, > + s->rs.packet_len, > + net_socket_send_completed) =3D=3D 0= ) { > + net_socket_read_poll(s, false); > > add > > s->rs->done > > void socket_fill_rsstate_done_cb(SocketReadState *srs, void *opaque) > { > NetSocketState *s =3D opaque; > > if (qemu_send_packet_async(&s->nc, srs->buf, > srs->packet_len, > net_socket_send_completed) =3D=3D 0) { > net_socket_read_poll(s, false); > } > } Yes, but there's no need for opaque, we can infer the container by=20 container_of(). > >> >>> } >>> } >>> } >>> @@ -229,7 +192,7 @@ static void net_socket_send_dgram(void *opaque) >>> NetSocketState *s =3D opaque; >>> int size; >>> - size =3D qemu_recv(s->fd, s->buf, sizeof(s->buf), 0); >>> + size =3D qemu_recv(s->fd, s->rs.buf, sizeof(s->rs.buf), 0); >>> if (size < 0) >>> return; >>> if (size =3D=3D 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) =3D=3D 0)= { >>> net_socket_read_poll(s, false); >>> } >> >> >> >> . >> >