From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52493) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b0Q1t-0004tU-GI for qemu-devel@nongnu.org; Wed, 11 May 2016 05:02:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b0Q1q-0004PB-06 for qemu-devel@nongnu.org; Wed, 11 May 2016 05:02:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:42885) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b0Q1p-0004OJ-OY for qemu-devel@nongnu.org; Wed, 11 May 2016 05:02:01 -0400 References: <1462532187-22787-1-git-send-email-zhangchen.fnst@cn.fujitsu.com> From: Jason Wang Message-ID: <5732F4FF.5060206@redhat.com> Date: Wed, 11 May 2016 17:01:51 +0800 MIME-Version: 1.0 In-Reply-To: <1462532187-22787-1-git-send-email-zhangchen.fnst@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: Li Zhijian , Wen Congyang , "eddie . dong" , "Dr . David Alan Gilbert" 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. > --- > 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; > =20 > +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. > + > +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 =3D getting length, 1 =3D getting data */ > - unsigned int index; > - unsigned int packet_len; > - uint8_t buf[REDIRECTOR_MAX_LEN]; > + ReadState rs; > } MirrorState; > =20 > static int filter_mirror_send(CharDriverState *chr_out, > @@ -108,51 +105,14 @@ static void redirector_chr_read(void *opaque, con= st 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 dat= a */ > - 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 received= ."); > - s->index =3D s->state =3D 0; > - qemu_chr_add_handlers(s->chr_in, NULL, NULL, NULL, NUL= L); > - 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); > } > } > =20 > @@ -274,7 +234,7 @@ static void filter_redirector_setup(NetFilterState = *nf, Error **errp) > } > } > =20 > - s->state =3D s->index =3D 0; > + s->rs.state =3D s->rs.index =3D 0; > =20 > 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 da= ta */ > + 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 recei= ved," > + "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 SOCK_= STREAM) */ > - uint8_t buf[NET_BUFSIZE]; > struct sockaddr_in dgram_dst; /* contains inet host and port dest= ination 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 =3D opaque; > int size; > - unsigned l; > + int ret; > uint8_t buf1[NET_BUFSIZE]; > const uint8_t *buf; > =20 > @@ -166,60 +163,26 @@ static void net_socket_send(void *opaque) > closesocket(s->fd); > =20 > 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? > memset(s->nc.info_str, 0, sizeof(s->nc.info_str)); > =20 > 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 recei= ved," > - "connection terminated.\n"); > - s->state =3D 0; > - goto eoc; > - } > =20 > - 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, s->packet_l= en, > - 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 on th= is? > } > } > } > @@ -229,7 +192,7 @@ static void net_socket_send_dgram(void *opaque) > NetSocketState *s =3D opaque; > int size; > =20 > - 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); > }