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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.