| Message ID | 1462532187-22787-1-git-send-email-zhangchen.fnst@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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
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 > . >
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 > . >
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); > }
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); >> } > > > > . >
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); >>> } >> >> >> >> . >> >
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); >>>> } >>> >>> >>> >>> . >>> >> > > > > . >
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?
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 > . >
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); }