From: Jason Wang <jasowang@redhat.com>
To: Zhang Chen <zhangchen.fnst@cn.fujitsu.com>,
qemu devel <qemu-devel@nongnu.org>
Cc: Li Zhijian <lizhijian@cn.fujitsu.com>,
Wen Congyang <wency@cn.fujitsu.com>,
"eddie . dong" <eddie.dong@intel.com>,
"Dr . David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [Qemu-devel] [PATCH] net/net: Add ReadState for reuse codes
Date: Wed, 11 May 2016 17:01:51 +0800 [thread overview]
Message-ID: <5732F4FF.5060206@redhat.com> (raw)
In-Reply-To: <1462532187-22787-1-git-send-email-zhangchen.fnst@cn.fujitsu.com>
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);
> }
next prev parent reply other threads:[~2016-05-11 9:02 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5732F4FF.5060206@redhat.com \
--to=jasowang@redhat.com \
--cc=dgilbert@redhat.com \
--cc=eddie.dong@intel.com \
--cc=lizhijian@cn.fujitsu.com \
--cc=qemu-devel@nongnu.org \
--cc=wency@cn.fujitsu.com \
--cc=zhangchen.fnst@cn.fujitsu.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).