* [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes @ 2016-05-06 10:56 Zhang Chen 2016-05-10 2:57 ` Li Zhijian 2016-05-11 9:01 ` Jason Wang 0 siblings, 2 replies; 10+ messages in thread From: Zhang Chen @ 2016-05-06 10:56 UTC (permalink / raw) To: qemu devel, Jason Wang Cc: Zhang Chen, Li Zhijian, Wen Congyang, eddie . dong, Dr . David Alan Gilbert Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> --- 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; + +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)); 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); } } } @@ -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); } -- 2.7.4 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes 2016-05-06 10:56 [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes Zhang Chen @ 2016-05-10 2:57 ` Li Zhijian 2016-05-10 5:33 ` Zhang Chen 2016-05-11 7:01 ` Zhang Chen 2016-05-11 9:01 ` Jason Wang 1 sibling, 2 replies; 10+ messages in thread From: Li Zhijian @ 2016-05-10 2:57 UTC (permalink / raw) To: Zhang Chen, qemu devel, Jason Wang Cc: Wen Congyang, eddie . dong, Dr . David Alan Gilbert On 05/06/2016 06:56 PM, Zhang Chen wrote: > Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > --- > 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; > + > +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]; you may need to remove 'REDIRECTOR_MAX_LEN' defination too. Thanks Li Zhijian ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes 2016-05-10 2:57 ` Li Zhijian @ 2016-05-10 5:33 ` Zhang Chen 2016-05-11 7:01 ` Zhang Chen 1 sibling, 0 replies; 10+ messages in thread From: Zhang Chen @ 2016-05-10 5:33 UTC (permalink / raw) To: Li Zhijian, qemu devel, Jason Wang Cc: Wen Congyang, eddie . dong, Dr . David Alan Gilbert On 05/10/2016 10:57 AM, Li Zhijian wrote: > > > On 05/06/2016 06:56 PM, Zhang Chen wrote: >> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >> --- >> 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; >> + >> +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]; > you may need to remove 'REDIRECTOR_MAX_LEN' defination too. > OK,I will fix it in next version. Thanks Zhang Chen > Thanks > Li Zhijian > . > -- Thanks zhangchen ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes 2016-05-10 2:57 ` Li Zhijian 2016-05-10 5:33 ` Zhang Chen @ 2016-05-11 7:01 ` Zhang Chen 1 sibling, 0 replies; 10+ messages in thread From: Zhang Chen @ 2016-05-11 7:01 UTC (permalink / raw) To: Li Zhijian, qemu devel, Jason Wang Cc: Wen Congyang, eddie . dong, Dr . David Alan Gilbert On 05/10/2016 10:57 AM, Li Zhijian wrote: > > > On 05/06/2016 06:56 PM, Zhang Chen wrote: >> Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> >> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >> --- >> 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; >> + >> +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]; > you may need to remove 'REDIRECTOR_MAX_LEN' defination too. Sorry, in redirector_chr_can_read() use 'REDIRECTOR_MAX_LEN' defination too. so we can't remove it. Thanks Zhang Chen > > Thanks > Li Zhijian > . > -- Thanks zhangchen ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes 2016-05-06 10:56 [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes Zhang Chen 2016-05-10 2:57 ` Li Zhijian @ 2016-05-11 9:01 ` Jason Wang 2016-05-11 11:20 ` Zhang Chen 1 sibling, 1 reply; 10+ messages in thread From: Jason Wang @ 2016-05-11 9:01 UTC (permalink / raw) To: Zhang Chen, qemu devel Cc: Li Zhijian, Wen Congyang, eddie . dong, Dr . David Alan Gilbert On 2016年05月06日 18:56, Zhang Chen wrote: > Signed-off-by: Zhang Chen <zhangchen.fnst@cn.fujitsu.com> > Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> 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; > > +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. > + > +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? > 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? > } > } > } > @@ -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); > } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes 2016-05-11 9:01 ` Jason Wang @ 2016-05-11 11:20 ` Zhang Chen 2016-05-12 1:11 ` Jason Wang 0 siblings, 1 reply; 10+ messages in thread From: Zhang Chen @ 2016-05-11 11:20 UTC (permalink / raw) 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 <zhangchen.fnst@cn.fujitsu.com> >> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > > 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes 2016-05-11 11:20 ` Zhang Chen @ 2016-05-12 1:11 ` Jason Wang 2016-05-12 6:33 ` Zhang Chen 0 siblings, 1 reply; 10+ messages in thread From: Jason Wang @ 2016-05-12 1:11 UTC (permalink / raw) To: Zhang Chen, qemu devel; +Cc: eddie . dong, Li Zhijian, Dr . David Alan Gilbert 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 <zhangchen.fnst@cn.fujitsu.com> >>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> >>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >> >> 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. > >> >>> 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(). > >> >>> } >>> } >>> } >>> @@ -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); >>> } >> >> >> >> . >> > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes 2016-05-12 1:11 ` Jason Wang @ 2016-05-12 6:33 ` Zhang Chen 2016-05-12 8:07 ` Jason Wang 0 siblings, 1 reply; 10+ messages in thread From: Zhang Chen @ 2016-05-12 6:33 UTC (permalink / raw) 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 <zhangchen.fnst@cn.fujitsu.com> >>>> Signed-off-by: Li Zhijian <lizhijian@cn.fujitsu.com> >>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >>> >>> 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes 2016-05-12 6:33 ` Zhang Chen @ 2016-05-12 8:07 ` Jason Wang 2016-05-12 8:23 ` Zhang Chen 0 siblings, 1 reply; 10+ messages in thread From: Jason Wang @ 2016-05-12 8:07 UTC (permalink / raw) To: Zhang Chen, qemu devel; +Cc: eddie . dong, Dr . David Alan Gilbert, Li Zhijian On 2016年05月12日 14:33, Zhang Chen wrote: >>>>> + 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. > > > You mean you need to get nf? Since SocketReadState were embedded in MirrorState, so you could get the address of MirrorState, then it's not hard to get nf address? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes 2016-05-12 8:07 ` Jason Wang @ 2016-05-12 8:23 ` Zhang Chen 0 siblings, 0 replies; 10+ messages in thread From: Zhang Chen @ 2016-05-12 8:23 UTC (permalink / raw) To: Jason Wang, qemu devel; +Cc: eddie . dong, Dr . David Alan Gilbert, Li Zhijian On 05/12/2016 04:07 PM, Jason Wang wrote: > > > On 2016年05月12日 14:33, Zhang Chen wrote: >>>>>> + 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. >> >> >> > > You mean you need to get nf? Since SocketReadState were embedded in > MirrorState, so you could get the address of MirrorState, then it's > not hard to get nf address? > > Got it~~ will fix it in next version. Thanks Zhang Chen > . > -- Thanks zhangchen ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-05-12 8:24 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-05-06 10:56 [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes Zhang Chen 2016-05-10 2:57 ` Li Zhijian 2016-05-10 5:33 ` Zhang Chen 2016-05-11 7:01 ` Zhang Chen 2016-05-11 9:01 ` Jason Wang 2016-05-11 11:20 ` Zhang Chen 2016-05-12 1:11 ` Jason Wang 2016-05-12 6:33 ` Zhang Chen 2016-05-12 8:07 ` Jason Wang 2016-05-12 8:23 ` Zhang Chen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).